[Modern Find/Replace] Refactor FindReplaceDialog

[Wittmaxi]

The FindReplaceDialog's business-logic is now handled in the FindReplacer.
The FindReplacer communicates using a FindReplaceStatus object which is polled by FindReplaceDialog as needed and allows for displaying correct messages in the Interface.

This Refactoring is required so that the business-logic can be reused in the new FindReplace overlay, as proposed in issue #1090

[Wittmaxi]

I expect that there is still a lot to change - especially since this is a rather intrusive change.
This is the first step in a milestone of changes that, as proposed here: #1091 (comment)

[mickaelistria]

We should avoid adding new API nor take risk of changing existing ones if there is not clear other consumer showing it's needed. I would rather delay the merge of those refactorings to when we can see how they contribute to providing value.
To rephrase it, I think it would be better to start by introducing the new UI (the find/replace bar) alternative to FindReplaceDialog and then reorganize the existing code so that it can serve both alternatives.

[Wittmaxi]

@mickaelistria Thank you for looking into this! 😊
I will need this refactoring in order to provide the new UI (it will reuse FindReplacer instead of reimplementing the business-logic). However, I agree with you that this PR should be merged together with the other branch that contains the UI-Changes.

I don't intend on adding anything to the API, everything should be under-the-hood. Are you referring to the package-visibility of the classes only?

I will update this PR to be easier to diff (removing unneeded changes, etc...) and move on to branch from this branch, implementing the UI-Changes there

[github-actions]

Test Results

   903 files  ±0     903 suites  ±0   1h 5m 10s ⏱️ - 7m 4s
 7 473 tests +2   7 319 ✅ +1  153 💤 ±0  1 ❌ +1 
23 568 runs  +6  23 056 ✅ +5  511 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit ccc3203. ± Comparison against base commit 2ff9d30.

♻️ This comment has been updated with latest results.

[vogella]

@mickaelistria have your concerns be addressed? Looks like it to me...

[mickaelistria]

Thanks for the heads-up. I'll try to review it later today.

[HeikoKlare]

It's great to separate the business functionality from the UI. The business code should be kept internal (i.e., be package private or in an internal package) for now, as proposed in https://github.com/eclipse-platform/eclipse.platform.ui/pull/1132/files#r1329665672. Then the current errors/warnings with respect to version bumps and warnings about non-API return type will become obsolete.

Currently, the internal status object of the FindReplacer can be modified by consumers. This seems to be a bug. My other comments rather concern code design/style.

[vogella]

@mickaelistria anything open here from your side?

[mickaelistria]

As mentioned earlier on the initial issue, I find pointless to merge such refactorings if there is not a change demonstrating it creates value for it (ie another consumer). Having a 2nd consumer included in the same change allows to validate that the refactoring serves a goal well; without it I won't spend more time in the review.

ok

[HeikoKlare]

I see the point of @mickaelistria, so some additional comments on this PR:

• First of all, refactorings can be valuable on their own, as they improve quality attributes such as comprehensibility and maintainability. I also see that in this refactoring, as it separates business logic from UI, which detangles these separate concerns.
• We may wait for the first WiP PR of the modernized find and replace integration that @Wittmaxi is working on to see a second consumer of the business logic. @Wittmaxi is currently working towards a state that can reasonably be shared with the community. I expect this to happen within the next weeks. For those who are interested, development happens on this branch: https://github.com/Wittmaxi/eclipse.platform.ui/tree/MW_FindReplace_Overlay
• I agreed with @Wittmax that we will enhance this PR with proper tests for the extracted find and replace business logic. This gives the PR more value than only the refactoring. Maybe you consider this equally valuable than a second code consumer, @mickaelistria. We can choose whether we want to wait for those tests to be added to this PR or whether it will be a subsequent one.

[vogella]

@Wittmaxi please look into the build errors, these need definitely be adressed.

[Wittmaxi]

Thank you for your interest @vogella, @mickaelistria, @HeikoKlare

I personally still see the need for change in this PR:

• JavaDocs need to be maintained (!!)
• Variable and Method names can be rethought
• UnitTests need to be written
• I am still not 100% convinced that this refactoring completely preserves behaviour

In order to get feedback in as quickly as possible (=> Eclipse Con!), I am currently pushing to get the Find/Replace-Dialog in a state that is good enough for us to get valuable and helpful feedback as well as some excitement in the community of users (no hard commitment, but I'm hoping to have it out by next wednesday). Once that is ready, I will get back to the nitty-gritties of this issue.

I have created a local pr of the new FindReplace Overlay, that makes reviewing easier. If anybody already wants to look into what is happening, I'm very happy to hearing first thoughts!

The build-errors come from a problem in version numbers.

Since I want to merge this PR together with the other branch, I'm not sure how I best proceed in increasing the version numbers. Any ideas?

[vogella]

Just send another PR with the version increase, I can merge it and afterwards you rebase this change onto master.

[Wittmaxi]

This push addressed all warnings present in the files of this PR and updates the JavaDocs

[HeikoKlare]

I really like the separation of UI and business code, but the business code still looks rather unstructured to me. In particular, I would propose to extract an interface that captures the "publicly" available and accordingly documented functionality of some find and replace implementation. I see this interface (that is currently implicitly represented via the public methods of the FindReplaceLogic implementation) as one of the most important outcomes of this PR. There are already some public methods in FindReplaceLogic that should actually not be there (like isEditable()).

[vogella]

@Wittmaxi please have a look at the test errors.

[HeikoKlare]

I have tested with the recent changes again and only found one remaining regression:
When search wraps, the old implementation produced a beep because no result was found before wrapping. The new implementation does, however, not produce a beep in that case.

[HeikoKlare]

I was not able to find any regressions in behavior anymore 👍 only one in message colors (see comment). I only see two minor things left as documented in the comments.

[HeikoKlare]

Now you removed the parameter evaluation.

[HeikoKlare]

Thanks for addressing all my previous comments have all been addressed. Unfortunately I find further behavior changes everytime I test the PR. Now I see these differences to the old behavior remaining:

• Invalid regex expressions are printed as info, not as error (i.e., not in red font).
• Beep is only performed for find operation, not for other operations.

[HeikoKlare]

Beep also needs to be performed in these cases:

• "Select All" without a match
• "Replace All" without a match
• Any operation with "regular expression" activated and the input is not a valid regular expression (e.g., "(" without ")")

[Wittmaxi]

In my current setup, I could not reproduce beep on

• Select all with 0 matches (not regex)
• Replace all with 0 matches (not regex)

I could reproduce the beep on faulty RegEx.

I have nevertheless implemented all of your suggestions: although I believe they introduce a difference between current behaviour and new behaviour, they still make sense.

[Wittmaxi]

116106a

[Wittmaxi]

When searching for "([", I receive a Stack Overflow error (This one is very important - "([" is a sensible string to want to search for and the error completely breaks the workbench)

I tested many more combinations extensively and I am starting to be very confident in matching the previous behaviour well.

[Wittmaxi]

Another bug I found: "replace all" and "select all" don't work in backwards mode

[Wittmaxi]

I could resolve the regressions I found - I also implemented @HeikoKlare 's suggestions even though I cannot reproduce the behaviour mentioned in the comment on my local machine.

[HeikoKlare]

Thanks for verifying and addressing my findings. I cannot reproduce the behavior for "Select All" and "Replace All" either. I am not sure which configuration I used there, maybe it was only related to an invalid regex.
Still I agree with you that the changed behavior makes sense. I think after disentangling the distributed behavior for message color and beeps, we now have a better understanding when they are supposed to apply. My understanding is as follows:

• There are two "error cases" for a find/replace operation:
  • "Invalid input": any of the input values is invalid. This currently applies to a read-only target and to an invalid regex pattern
  • "Unapplicable": Nothing matching the search pattern is found in the target, so the operation cannot be applied (showing a find result, applying a replace etc.)
• Invalid inputs are currently represented by a red-colored message, whereas unapplicability is confirmed with a beep.

The proposed implementation seems to almost properly reflect this (at least as properly as the existing ones does). Still, I propose to improve the code design in terms of replacing switch logic in the beep handling to a proper representation of this behavior in the search operation status objects (like done with the isError() method for invalid input). Doing that, we can also properly reflect this behavior at all, because, e.g. "Select All" executed on read-only targets currently gives invalid results. But since this PR now seems to be almost fine from a functional perspective, let's keep it is as it is and not mix it up with further optimizations that can be conducted independently.

Still, there are now some things broken (again).

• Shift+Enter with backwards search enabled does not work (which is why a test fails)
• When changing the input in incremental search mode, the result always moves to the next result, even if the current one is fitting.

In addition, the added isError parameter for the FindStatus constructor is unnecessary due to above considerations: It can be extracted from the messageCode field, as error state depends on whether the target is read only. One nitpicky thing I also noticed is that there are still things named "FindReplacer" rather than "FindReplaceLogic".

To be able to merge this soon, I contribute a fix for these things together with a regression test for the inverse search behavior in a separate commit (1d6bc22) on this PR rather than doing further iterations.

[HeikoKlare]

I have now intensively validated the behavior and only found a minor regression related to checkbox disablements not being considered properly: in case the "whole word" checkbox becomes disabled because more than a single word is interted into the input field (or because regex search is enabled), the "whole word" option stays enabled if the checkbox was ticked before it became disabled. I fixed this and provided an according regression test as an amendment in ccc3203. Everything else worked for me.

From my side, this one is ready to merge. I think we have enough time left in the current release cycle to find potential regressions in case we have missed anything during our validations. Since this PR is a prerequisite for the modernized find/replace UI (#1192), not merging it would complicate further development of that contribution. And there are also other issues regarding the find functionality (like #1029) which make more sense to be addressed in refactored code rather than fixing the current code and then rebasing the fix onto the refactored one.
@vogella Anything from your side that speaks against merging this?

[vogella]

No concerns from my side, the improved search UI is the top development for the Eclipse IDE I'm really waiting for as a user so I'm really happy that you @HeikoKlare did this detailed review and check of the development from @Wittmaxi

@Wittmaxi please continue with the UI improvement, IMHO this improvement is huge for our user base.

[Wittmaxi]

@vogella thank you for merging the PR and for the kind words :)

[vogella]

@vogella thank you for merging the PR and for the kind words :)

All the thanks goes to you @Wittmaxi and @HeikoKlare for doing the actual work. I just act as an impatient user who really, really wants this new search experience. :-o