Skip to content

New database table for unique app names #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 5, 2024

Conversation

caetano-dev
Copy link
Contributor

This PR adds a new table for the app names that are used in the search filters along with improvements in the dropdown menu.

It is a sequence of #56

@jasonjmcghee jasonjmcghee mentioned this pull request Jan 4, 2024
rem/DB.swift Outdated
do {
let count = try db.scalar(query.count)
if count == 0 {
print("insert")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use logger.debug everywhere instead of print

rem/DB.swift Outdated
let query = allText
.join(frames, on: frames[id] == allText[frameId])
.join(videoChunks, on: frames[chunkId] == videoChunks[id])
.join(uniqueAppNames, on: uniqueAppNames[activeApplicationName] == frames[activeApplicationName])
Copy link
Owner

@jasonjmcghee jasonjmcghee Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this is the only line that is changed from the above function - can we just add this conditionally in both search and getRecentResults specified by an optional parameter instead?

I think it would be this single line and a change to the function signature instead - saving like 50 lines of code

Copy link
Owner

@jasonjmcghee jasonjmcghee Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like:

var partialQuery = allText
  .join(frames, on: frames[id] == allText[frameId])
  .join(videoChunks, on: frames[chunkId] == videoChunks[id])

if appName != "" {
  partialQuery = partialQuery.join(uniqueAppNames, on: uniqueAppNames[activeApplicationName] == frames[activeApplicationName])
}

let query = partialQuery
  .filter(text.match("*\(searchText)*") && uniqueAppNames[activeApplicationName].lowercaseString == appName.lowercased())
  .select(allText[frameId], text, frames[activeApplicationName], frames[timestamp], videoChunks[filePath], frames[offsetIndex])
  .limit(limit, offset: offset)

rem/Search.swift Outdated
@@ -136,6 +183,7 @@ class SearchResult: ObservableObject, Identifiable {
// .replacingOccurrences(of: "\\s+", with: "\\s*", options: .regularExpression)
.replacingOccurrences(of: "(", with: "\\(")
.replacingOccurrences(of: ")", with: "\\)")
.replacingOccurrences(of: #"!([^ ]*) "#, with: "", options: .regularExpression)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have the dropdown, should we remove this?

@caetano-dev
Copy link
Contributor Author

caetano-dev commented Jan 5, 2024

Changes to the functions applied. That was a great idea, thanks for pointing that out!

I also made some changes to the snippet you provided, since it was not returning results if there was a filter with no text to be searched.

Copy link
Owner

@jasonjmcghee jasonjmcghee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Haven’t had a chance to test the code on either pr yet, but will make time tonight. Awesome changes!

@caetano-dev
Copy link
Contributor Author

Awesome! Thanks!

@jasonjmcghee
Copy link
Owner

(fixed conflicts that I caused in my follow-up)

Comment on lines +104 to +106
.onHover(perform: { hovering in
updateAppFilterData()
})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why onHover out of curiosity? This means it's blank until you hover. Could we instead do onAppear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had initially implemented it with onAppear, but it would cause the filter to only update on the first time the user clicked on "Search". It means that, if you left the search screen and went back to it again, the filter would stay the same until you restarted the program.

I made a new change adding both onAppear and onHover, which solves the issue of it being blank at first and not updating after the first search.

caetano-dev and others added 2 commits January 5, 2024 09:55
Copy link
Owner

@jasonjmcghee jasonjmcghee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caetano-dev Approving and will merge, but thinking we should do a follow-up to do an order by activeApplicationName asc. Right now dropdown application name results are arbitrary

@jasonjmcghee jasonjmcghee merged commit b696809 into jasonjmcghee:main Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants