-
Notifications
You must be signed in to change notification settings - Fork 196
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
Quick Search - History for "search in" #2334
base: master
Are you sure you want to change the base?
Quick Search - History for "search in" #2334
Conversation
remove commented-out code thus improving code quality.
Test Results 1 815 files ±0 1 815 suites ±0 1h 32m 38s ⏱️ - 5m 47s For more details on these failures, see this check. Results for commit f23bbd0. ± Comparison against base commit f35ef3a. |
@@ -811,6 +791,7 @@ public void getName(AccessibleEvent e) { | |||
|
|||
pattern.addModifyListener(e -> { | |||
applyFilter(false); | |||
searchIn.storeHistory(); |
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.
on every character typed?
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.
Yes, because every new character typed starts a new search. Storing history is cheap, so I don't think that's a problem
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.
compare to #2291
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 believe you refer to
Since the overlay has incremental search enabled by default (other than the existing dialog), adding every searched term to the history is inappropriate, as typing "test" would result in history entries "t", "te", "tes" and "test".
However, here we are doing something slightly differently: the search history is applied to the "search in" field - and we store the "search in" history every time some search is performed - while typing in the search bar, there is no change of the "search in" field. Thus, we save the same string to history for every keystroke. However, since we also remove all other occurrences of that exact string in the history, the operation does nothing.
Anyway, I don't want to invest too much into this PR. It is a good model for a new contributor wanting to play with this issue.
The main problems are the dependencies; I am not so sure about importing two internal packages
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.
please mark as draft
This PR can be a start for anybody willing to take on this issue - I think it is good enough "as is", but I would like somebody to double-check if adding the dependencies I added is okay