FindReplaceOverlay: share search settings per editor stack by vlakn · Pull Request #1 · vi-eclipse/eclipse.platform.ui

vlakn · 2026-06-25T08:33:52Z

Editors in the same tab group now share a common find/replace state. Switching between tabs in a stack keeps the overlay in sync: it auto-opens on the newly activated editor and auto-closes when another stack-mate closes it.

Independent stacks (e.g. side-by-side editors) maintain their own separate settings.

Fixes: eclipse-platform#2055

Editors in the same tab group now share a common find/replace state. Switching between tabs in a stack keeps the overlay in sync: it auto-opens on the newly activated editor and auto-closes when another stack-mate closes it. Independent stacks (e.g. side-by-side editors) maintain their own separate settings. Fixes: eclipse-platform#2055

r-mennig · 2026-06-25T11:05:03Z

I've found some issues with the current implementation in regard to marker resets / selection changes:

r-mennig · 2026-06-25T13:42:08Z

+		IWorkbenchPartSite site = targetPart.getSite();
+		if (site == null) {
+			return null;
+		}


getSite() sollte zu diesem Zeitpunkt nicht mehr null sein dürfen, da die Initialisierung vom Part schon abgeschlossen sein muss

r-mennig · 2026-06-25T14:16:42Z

+		if (isRestoringSettings) {
+			return;
+		}


Das hier finde ich nicht so schön gelöst.

Dieser isRestoringSettings-Guard ist ja nach meinem Verständnis nur notwendig ist, weil dieser in applyStackSettings von searchBar.setText(...) sonst nochmal aufgerufen werden würde (hier wäre übrigens ein Kommentar hilfreich, ohne zu Debuggen ist sonst kaum zu erkennen, wofür das gedacht ist).

Den Check dafür würde ich daher nicht hier, sondern im ModifyListener der Searchbar einbauen (mit Kommentar).

Am besten wäre es natürlich den Textinhalt der Searchbar wiederherzustellen, ohne dabei ein Update auszulösen. @HeikoKlare Hast du noch Ideen, was man hier machen könnte?

r-mennig · 2026-06-25T14:23:41Z

+	private void openForStack() {
+		if (!containerControl.isVisible()) {
+			containerControl.setVisible(true);
+			bindListeners();
+			restoreOverlaySettings();
+			setOverlayOpenInStack(true);
+			registerStackMemberActivationListener();
+		}
+		assignIDs();
+		containerControl.layout();
+		containerControl.moveAbove(null);
+		updatePlacementAndVisibility();
+		updateContentAssistAvailability();
+
+		searchBar.setFocus();
+	}


Der Code ist fast identisch mit der open()-Methode -> als open(boolean forStack) kombinieren

r-mennig · 2026-06-25T14:24:18Z

+	private void silentClose() {
+		if (containerControl.isDisposed() || !containerControl.getVisible()) {
+			return;
+		}
+		setOverlayOpenInStack(false);
+		storeOverlaySettings();
+		findReplaceLogic.activate(SearchOptions.GLOBAL);
+		unbindListeners();
+		containerControl.setVisible(false);
+	}


Der Code ist fast identisch mit der close()-Methode -> als close(boolean silently) kombinieren

r-mennig · 2026-06-25T14:29:44Z

+		@Override
+		public void partDeactivated(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partOpened(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partClosed(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partHidden(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partVisible(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partBroughtToTop(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partInputChanged(IWorkbenchPartReference partRef) {
+			/* unused */ }
+	}


Evtl. könnte hier ein separates Issue erzeugt werden, um eine generische PartListenerAdapter-Klasse zu definieren.

@fedejeanne Was meinst du? Ich sehe keinen Grund, warum ein Verwender hier immer auf alle Part-Events reagieren sollte

Man kann einfach die leere Methoden löschen, IPartListener2 hat bereits leere default-Methoden

r-mennig · 2026-06-25T14:31:20Z

+ * always show the same search term and options, while independent editor stacks
+ * (e.g. side-by-side layouts) can carry different, unrelated search sessions.
+ */
+public class OverlaySearchSettings {


Das würde sich als Record meiner Meinung nach schöner abbilden lassen und ein HashCode/Equals gäbs dann auch noch gratis dazu :)

r-mennig · 2026-06-25T14:35:25Z


+	private static final WeakHashMap<MPartStack, OverlaySearchSettings> STACK_SETTINGS = new WeakHashMap<>();
+
+	private static final WeakHashMap<IWorkbenchPart, FindReplaceOverlay> PART_TO_OVERLAY = new WeakHashMap<>();


Diese Felder würde ich nicht im Overlay selbst halten, sondern in eine Registry-Klasse auslagern

r-mennig · 2026-06-25T14:40:35Z

+		settings.setReplaceBarOpen(false);
+
+		Accessor actionAccessor= new Accessor(getFindReplaceAction(), FindReplaceAction.class);
+		FindReplaceOverlay overlay= (FindReplaceOverlay) actionAccessor.get("overlay");


Wenn du noch eine createForPart()-Methode ergänzen würdest, bei der die initialen Settings übergeben werden können, könnten die Reflection-Calls hier vermutlich vermieden werden

r-mennig · 2026-06-25T14:50:24Z

 				shellToUse = fShell;
 			}
-			overlay = new FindReplaceOverlay(shellToUse, fWorkbenchPart, fTarget);
+			overlay = FindReplaceOverlay.getOrCreateForPart(shellToUse, fWorkbenchPart, fTarget);


Jetzt habe ich schon ziemlich viele Einzelkommentare gemacht, aber meiner Meinung nach könnte der Codefluss ab hier schon anders sein.

Design-Idee:
Die Action frägt bei einer FindReplaceOverlayRegistry an, ob zu einem bestimmten PartStack schon ein Overlay offen ist oder mal offen war. Wenn ja, wird die existierende Instanz zurückgegeben oder ein neue Instanz mit den gespeicherten Settings gebaut.

Wird eine solche Instanz jetzt aktiv vom Nutzer geschlossen (oder andersweitig disposed, z.B. durch Schließen des Editor-Tabs), werden die derzeitigen Settings für den PartStack gespeichert (falls dieser dann noch exisitiert).

Aus meiner Sicht gibts keinen Grund, dann noch bei jeder einzelnen Modifikation immer wieder die Settings zu speichern oder übersehe ich da etwas?

fedejeanne

Ich habe nicht getestet, nur den Code überflogen. Ich glaube mit den Kommentaren von Robin gibt es einiges zu tun bis zum nächsten Review.

Eine Sache nur: wir sollten uns abstimmen, ob wir in vi-eclipse auf Deutsch oder auf Englisch reviewen. Diese Orga ist public ;-)

fedejeanne · 2026-06-26T09:54:32Z

 				.withToolTipText(FindReplaceMessages.FindReplaceOverlay_upSearchButton_toolTip)
-				.withOperation(() -> performSearch(false))
-				.withShortcuts(KeyboardShortcuts.SEARCH_BACKWARD).build();
+				.withOperation(() -> performSearch(false)).withShortcuts(KeyboardShortcuts.SEARCH_BACKWARD).build();


Auto-formattierung aktiv? Wenn möglich, bitte solche unnötige Änderungen vermeiden.

fedejeanne · 2026-06-26T10:50:28Z

+		@Override
+		public void partDeactivated(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partOpened(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partClosed(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partHidden(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partVisible(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partBroughtToTop(IWorkbenchPartReference partRef) {
+			/* unused */ }
+
+		@Override
+		public void partInputChanged(IWorkbenchPartReference partRef) {
+			/* unused */ }
+	}


Man kann einfach die leere Methoden löschen, IPartListener2 hat bereits leere default-Methoden

r-mennig requested changes Jun 25, 2026

View reviewed changes

r-mennig reviewed Jun 25, 2026

View reviewed changes

fedejeanne reviewed Jun 26, 2026

View reviewed changes


		private static final WeakHashMap<MPartStack, OverlaySearchSettings> STACK_SETTINGS = new WeakHashMap<>();

		private static final WeakHashMap<IWorkbenchPart, FindReplaceOverlay> PART_TO_OVERLAY = new WeakHashMap<>();

Uh oh!

Conversation

vlakn commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r-mennig commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedejeanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vlakn commented Jun 25, 2026 •

edited

Loading

r-mennig commented Jun 25, 2026 •

edited

Loading