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

[FindNextAction] synchronize search history with FindReplaceOverlay #2405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wittmaxi
Copy link
Contributor

@Wittmaxi Wittmaxi commented Oct 15, 2024

synchronize search history with the FindReplaceOverlay and FindReplaceDialog.

37

This fix is not as extensive as a prior attempt (#2308) and thus does not require as much testing. Since this is a high-priority issue, we want this to be fixed ASAP.

  1. search histories and options are now stored at a centralized place - in the options for the FindReplaceAction class.
  2. the name of the search history settings section is now unified
  3. the search history component of the Find/Replace Overlay now updates it's history on the fly to accomodate.

Copy link
Contributor

github-actions bot commented Oct 15, 2024

Test Results

 1 821 files  ±0   1 821 suites  ±0   2h 5m 25s ⏱️ + 13m 42s
 7 714 tests ±0   7 486 ✅ ±0  228 💤 ±0  0 ❌ ±0 
24 303 runs  ±0  23 556 ✅ ±0  747 💤 ±0  0 ❌ ±0 

Results for commit 0cd7720. ± Comparison against base commit 5c9af91.

♻️ This comment has been updated with latest results.

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.

Sharing the settings between the dialog and the overlay is good. The current proposal will produce a regression in the dialog, which needs to be resolved.

Comment on lines 345 to 347
fDialogSettings= settings.getSection(FindReplaceDialog.class.getName());
if (fDialogSettings == null)
fDialogSettings= settings.addNewSection(FindReplaceDialog.class.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the FindReplaceDialog, as it still uses the subsection according to its class name. Shouldn't we use a subsection in the dialog settings of the bundle, e.g., using the class name of the FindReplaceAction class, like done with the FindReplaceDialog before?

Copy link
Contributor Author

@Wittmaxi Wittmaxi Oct 16, 2024

Choose a reason for hiding this comment

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

I have made it so that FindReplaceDialog, FindReplaceOverlay and FindNextAction have the exact same method for getting the Dialog Settings in 5847b3d
I have contemplated adding this as a public method to the FindReplaceAction, however I chose not to in order not to pollute API

Copy link
Contributor

Choose a reason for hiding this comment

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

@Wittmaxi Now you extract different settings sections, as each class (the action, overlay and dialog) retrieve the one according their own class instead of the one for the same class (such as the action):

IDialogSettings dialogSettings = settings.getSection(getClass().getName());

@Wittmaxi
Copy link
Contributor Author

random fails documented in

#195
#294
#751

…ipse-platform#2285

Synchronize search history with the FindReplaceOverlay,
FindReplaceDialog and FindNextAction.

Fixes eclipse-platform#2285
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