Find/Replace overlay: move components into internal package

[Wittmaxi]

moves helper classes for the find/replace overlay into an internal package.

[Wittmaxi]

Extracted from #1990

[HeikoKlare]

Wouldn't it make sense to also move the FindReplaceOverlay to the org.eclipse.ui.internal.findandreplace.overlay package? Then the other classes could remain package protected, as they are only supposed to be used by the overlay class (and not any other class in the bundle).

[Wittmaxi]

@HeikoKlare

FindReplaceOverlay uses package-private methods of StatusTextEditor and for now, I don't want to touch StatusTextEditor. Also, since the Dialog is also in ui.texteditor, I believe it is best if (for now!) we keep the Overlay in ui.texteditor

[laeubi]

@Wittmaxi by the way a good way to decouple things is usually using the Eclipse Adapters (e.g. implements IAdaptable) that way one often can get access to some things (e.g. a SourceViewer) by doing

Adapters.adapt(whatever, SourceViewer.class)

without need to call any methods directly on the target.

[HeikoKlare]

I see. Then, however, I am not sure about the value of the proposed refactoring. What's the pain point in keeping the classes where they currently are? In particular, placing the overlay class in a different package than its first-time usage popup and it's images provider class does not look sound to me.

[Wittmaxi]

@HeikoKlare I am fine with moving these specific classes back. The reason I want to refactor these classes is that I am expecting many helper classes for the Overlay (For example in the PR adding a search-history) which are not really related to the texteditor-ui.
I understand that by putting the overlay away from it's helpers we reduce cohesion. I will try to use @laeubi 's advice using the adapters to call methods of the SourceViewer

[HeikoKlare]

The reason I want to refactor these classes is that I am expecting many helper classes for the Overlay (For example in the PR adding a search-history) which are not really related to the texteditor-ui.

Thanks for the explanation. Now I understand.

Following the advice from @laeubi sounds good! The following should probably already do the trick:

   Control targetWidget = textEditor.getAdapter(ITextViewer.class).getTextWidget();

[Wittmaxi]

@HeikoKlare yes! This works :)

[github-actions]

Test Results

 1 815 files  ±0   1 815 suites  ±0   1h 35m 28s ⏱️ - 1m 15s
 7 663 tests ±0   7 435 ✅ ±0  228 💤 ±0  0 ❌ ±0 
24 150 runs  ±0  23 401 ✅ ±0  749 💤 ±0  0 ❌ ±0 

Results for commit 4276de4. ± Comparison against base commit 769d5d1.

This pull request removes 16 and adds 16 tests. Note that renamed tests count towards both.

org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testActivateDialogWithSelectionActive
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testActivateWholeWordsAndSearchForTwoWords
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testCantOpenReplaceDialogInReadOnlyEditor
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testChangeInputForIncrementalSearch
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testDirectionalSearchButtons
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testDisableWholeWordIfNotWord
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testDisableWholeWordIfRegEx
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testFindWithWholeWordEnabledWithMultipleWords
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testIncrementalSearchUpdatesAfterChangingOptions
org.eclipse.ui.workbench.texteditor.tests.FindReplaceOverlayTest ‑ testInitialButtonState
…

org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testActivateDialogWithSelectionActive
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testActivateWholeWordsAndSearchForTwoWords
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testCantOpenReplaceDialogInReadOnlyEditor
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testChangeInputForIncrementalSearch
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testDirectionalSearchButtons
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testDisableWholeWordIfNotWord
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testDisableWholeWordIfRegEx
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testFindWithWholeWordEnabledWithMultipleWords
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testIncrementalSearchUpdatesAfterChangingOptions
org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest ‑ testInitialButtonState
…

♻️ This comment has been updated with latest results.

[HeikoKlare]

  Control targetWidget = textEditor.getAdapter(ITextViewer.class).getTextWidget();

Access should of course be made via Adapters. Sorry for the "inadequate" proposal:

   Control targetWidget = Adapters.adapt(textEditor, ITextViewer.class).getTextWidget();

[Wittmaxi]

@HeikoKlare what is the difference between your proposals and why use the second one instead of the first one? The first one already works perfectly!

[HeikoKlare]

Maybe in this case it is even fine to call getAdapter() directly. The Javadoc states:

I guess the reason is that there may be more adapters than delivered by getAdapters() (as there is an adapter manager that is also queried by Adapters.adapt()). In addition, you can call Adapters.adapt() for objects of which you do not know whether they implement IAdaptable or not. But none of these cases applies here.
@laeubi do you see a reason for using Adapters.adapt() instead of directly calling IAdaptable.getAdapter() in this case?

[laeubi]

@laeubi do you see a reason for using Adapters.adapt() instead of directly calling IAdaptable.getAdapter() in this case?

Adapters.adapt() is just more general form and you don't need to bother if it implements IAdaptable, is null and so on ... but its not forbidden to call IAdaptable directly. Beside that there is a convenient Adapters#of that even returns an Optional ...