-
Notifications
You must be signed in to change notification settings - Fork 73
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
Conversation
rem/DB.swift
Outdated
do { | ||
let count = try db.scalar(query.count) | ||
if count == 0 { | ||
print("insert") |
There was a problem hiding this comment.
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]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
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. |
There was a problem hiding this 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!
Awesome! Thanks! |
(fixed conflicts that I caused in my follow-up) |
.onHover(perform: { hovering in | ||
updateAppFilterData() | ||
}) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
Co-authored-by: Jason McGhee <mcghee.j@gmail.com>
There was a problem hiding this 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
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