Skip to content
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

Incremental Search: extract logic for disabling on RegEx-Search #1791 #1805

Conversation

Wittmaxi
Copy link

when "incremental search" is selected and "RegEx-Search" is as well, the dialog/overlay will disable the option for "whole words". This PR extracts that functionality into the Find/Replace-Logic.

Extracted from a Review-Comment in #1791

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Test Results

   921 files     921 suites   46m 9s ⏱️
 7 518 tests  7 367 ✅ 151 💤 0 ❌
23 706 runs  23 199 ✅ 507 💤 0 ❌

Results for commit 9b783e9.

♻️ This comment has been updated with latest results.

@Wittmaxi
Copy link
Author

Random Fail for Ubuntu is reported here: #1808
Random Fail for Mac is reported here: #811
Random Fail for Windows is reported here: #1427

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

In general, this looks good. I have two minor comments.

@HeikoKlare
Copy link
Contributor

And could you please also adapt the commit message, just like for the "whole words" options? (see #1791 (comment))

@Wittmaxi Wittmaxi force-pushed the MW_incremental_search_uncouple_logic_from_ui branch 3 times, most recently from c14ae90 to d654b06 Compare April 10, 2024 14:16
@Wittmaxi
Copy link
Author

@HeikoKlare Addressed in d654b06

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Minor improvement in a test name desired

@Wittmaxi Wittmaxi force-pushed the MW_incremental_search_uncouple_logic_from_ui branch from d654b06 to 859fef9 Compare April 10, 2024 15:08
@Wittmaxi Wittmaxi force-pushed the MW_incremental_search_uncouple_logic_from_ui branch from 859fef9 to df357ae Compare April 11, 2024 06:06
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The current state produces a regression, as the dialog can loose its "incremental" option when reopened. To reproduce, activate the "incremental" option, activate and deactive the "regex" option, then close and re-open the dialog. The "incremental" option is unselected but it selection should be recovered from the previous session of the dialog.

This is because the change misses an update of the functionality in FindReplaceDialog.setupFindReplaceLogic(), which still bases the activation of the "incremental" option on its selection and its enablement. The latter has to be removed, just like for the "whole word" option.

activateInFindReplaceLogicIf(SearchOptions.INCREMENTAL,
fIncrementalCheckBox.getEnabled() && fIncrementalCheckBox.getSelection());

@fedejeanne
Copy link
Contributor

The current state produces a regression ...

Now that you mention it, I'm pretty sure one can find some other weird behaviors when playing with "Incremental", "Whole words" and "Regular expressions" and closing + reopening the search dialog :-o

@HeikoKlare
Copy link
Contributor

Now that you mention it, I'm pretty sure one can find some other weird behaviors when playing with "Incremental", "Whole words" and "Regular expressions" and closing + reopening the search dialog :-o

Can you elaborate on that? Is there something specific you have in mind and is that a general negative expectation or related to this PR?

@fedejeanne
Copy link
Contributor

Now that you mention it, I'm pretty sure one can find some other weird behaviors when playing with "Incremental", "Whole words" and "Regular expressions" and closing + reopening the search dialog :-o

Can you elaborate on that? Is there something specific you have in mind and is that a general negative expectation or related to this PR?

Sorry for being unclear. I wasn't talking about this PR :-)

@Wittmaxi Wittmaxi force-pushed the MW_incremental_search_uncouple_logic_from_ui branch from df357ae to 7568c1b Compare April 12, 2024 09:31
@Wittmaxi
Copy link
Author

@HeikoKlare I have addressed your comment and added a Unit-Test which checks the behavior for "re-opening" the dialog.
A very large amount of improvements in testing-architecture were made in the Find/Replace-Overlay-PR, so once that is moved, we should test the "re-opening"-behavior of the Dialog/Overlay with the other buttons

@Wittmaxi
Copy link
Author

Windows Random Failing documented here: #1807

@HeikoKlare HeikoKlare force-pushed the MW_incremental_search_uncouple_logic_from_ui branch from 7568c1b to a767ca3 Compare April 16, 2024 07:17
@HeikoKlare HeikoKlare force-pushed the MW_incremental_search_uncouple_logic_from_ui branch 5 times, most recently from 7ec1114 to 679b224 Compare April 16, 2024 19:06
when "incremental search" is selected and "RegEx-Search" is as well,
the dialog/overlay will disable the option for "whole
words". This PR extracts that functionality into the Find/Replace-Logic.

Extracted from a Review-Comment in eclipse-platform#1791
@HeikoKlare HeikoKlare force-pushed the MW_incremental_search_uncouple_logic_from_ui branch from 679b224 to 9b783e9 Compare April 17, 2024 12:43
@HeikoKlare
Copy link
Contributor

Failing checks because of new issues are only because of an outdated reference build. Recent master build reports the same new issues: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/422

@HeikoKlare HeikoKlare merged commit f391341 into eclipse-platform:master Apr 17, 2024
13 of 16 checks passed
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.

3 participants