From 23e5c8a7431bc44b3a76a1f21461d2938f7f2791 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Tue, 1 Sep 2026 11:40:59 +0200 Subject: [PATCH] Find/Replace overlay: make the overlay the active part while focused The overlay's control is parented into the editor's widget tree, so focusing an input field does not change the active part. The editor's key bindings and command handlers therefore stayed in effect and consumed keys meant for the input fields, which was worked around by reflectively disabling the editor's action activation and by nulling out its global action handlers. That workaround reached into private API, applied only to AbstractTextEditor, and covered only the six retargetable actions rather than the full set of conflicting commands. Instead of suppressing the editor's commands one by one, the overlay now takes the editor out of the resolution path while an input field has focus. Both conditions that decide whether one of the editor's handlers wins have to be addressed, because the editor's commands are spread over both: its key binding scopes and part-level handlers are reachable through the editor's part context, while its retargetable actions live in the window context and are guarded by an expression over the active part id. Activating a context that is a sibling of the editor's part context removes the former, and declaring that context's own id as the active part id makes the latter evaluate to false. The context is placed below the window context rather than below the application context, so that window-scoped commands and services remain available. Only the active part id is overridden, not the active part itself, so the overlay and anything invoked from it still operate on the editor, and so do contributions keyed on the active part, the active editor or the selection. With no editor handler left in the resolution path, keys the platform does not otherwise handle reach the native text widget, and the workbench-wide default handlers for cut, copy, paste and select all act on the focused input field, so those also work from the Edit menu again. The overlay's key binding scopes are activated in that same context rather than at the workbench context service. Scopes are collected along the chain between the active leaf and the root, so a scope activated there is active exactly while the context is the active leaf: the overlay's shared scope is activated once and never deactivated, and only the per-field scope is switched as focus moves between the input fields. Both kinds of context are consequently owned by one class, leaving the command support with handler activation and shortcut hints. Since the overlay reports itself as the active part, that is also what scopes its own command handlers, which are activated once at the workbench. The active part id changes at exactly the moments focus enters and leaves an input field, and it says which overlay is focused, so the overlays of different editors no longer need to be told apart by inspecting the focus control's widget hierarchy and no focus tracking has to be registered for the input fields at all. The overlay's shared scope no longer declares the text editor scope as its parent. Parent scopes are resolved when building the set a binding lookup runs against, so that parent would have reintroduced the editor's bindings regardless of the context topology, and it also tied the overlay to text editors. The active leaf is handed back when an input field loses focus only while the overlay still holds it. Losing the focus to another part is not such a case: the workbench activates the part under the mouse before the focus leaves the field, so that part already owns the leaf by then and has to keep it. Handing it to the editor anyway would raise the editor's key binding scope beside the one the part coming up brings, and while both are up every stroke the two scopes have in common is an unresolvable binding conflict, as observed for the zoom commands the console and the text editor both bind. None of this changes what the overlay owes its users, so the existing end-to-end tests pass unchanged before and after. The one behaviour not covered by them is that leaving the overlay for another part reports no binding conflict, which could not be reproduced outside a running IDE: it needs the other part to hold its key binding scope across the switch, which a part whose scope follows the active leaf does not do, so every fixture tried settles on a single scope and never conflicts. It is described in the architecture decision record instead of being pinned by a test that would pass either way. Assisted-by: Claude Opus 5 --- .../META-INF/MANIFEST.MF | 5 +- .../plugin.xml | 2 +- .../overlay/FindReplaceOverlay.java | 4 +- .../FindReplaceOverlayCommandSupport.java | 197 ++--------- .../FindReplaceOverlayContextSupport.java | 244 +++++++++++++ .../0001-find-replace-overlay-key-handling.md | 321 ++++++++++++++++++ 6 files changed, 595 insertions(+), 178 deletions(-) create mode 100644 bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayContextSupport.java create mode 100644 docs/adr/0001-find-replace-overlay-key-handling.md diff --git a/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF index fae838ba10d..002a49ef574 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF @@ -33,7 +33,10 @@ Require-Bundle: org.eclipse.jface.text;bundle-version="[3.19.0,4.0.0)", org.eclipse.swt;bundle-version="[3.133.0,4.0.0)", org.eclipse.ui;bundle-version="[3.210.0,4.0.0)", - org.eclipse.jface.notifications + org.eclipse.jface.notifications, + org.eclipse.e4.core.contexts;bundle-version="[1.13.0,2.0.0)", + org.eclipse.e4.ui.model.workbench;bundle-version="[2.4.0,3.0.0)", + org.eclipse.e4.ui.services;bundle-version="[1.6.0,2.0.0)" Bundle-RequiredExecutionEnvironment: JavaSE-21 Automatic-Module-Name: org.eclipse.ui.workbench.texteditor Require-Capability: eclipse.swt;filter:="(image.format=svg)" diff --git a/bundles/org.eclipse.ui.workbench.texteditor/plugin.xml b/bundles/org.eclipse.ui.workbench.texteditor/plugin.xml index 47d2f1a1a34..a7fcb1aca25 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/plugin.xml +++ b/bundles/org.eclipse.ui.workbench.texteditor/plugin.xml @@ -1561,7 +1561,7 @@ + parentId="org.eclipse.ui.contexts.window"> commandSupport.dispose()); customFocusOrder.install(); updateReplaceVisibility(false); containerControl.setVisible(false); @@ -484,9 +484,7 @@ private void createContentsContainer() { GridDataFactory.fillDefaults().grab(true, true).align(GridData.FILL, GridData.FILL).applyTo(contentGroup); createSearchContainer(); - commandSupport.trackFocusControl(searchBar.getTextBar()); createReplaceContainer(); - commandSupport.trackFocusControl(replaceBar.getTextBar()); } private void createSearchTools() { diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayCommandSupport.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayCommandSupport.java index f2dce606d26..5cd16887072 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayCommandSupport.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayCommandSupport.java @@ -10,51 +10,32 @@ *******************************************************************************/ package org.eclipse.ui.internal.findandreplace.overlay; -import java.lang.reflect.Method; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import org.eclipse.swt.widgets.Composite; -import org.eclipse.swt.widgets.Control; -import org.eclipse.swt.widgets.Text; - -import org.eclipse.core.expressions.EvaluationResult; import org.eclipse.core.expressions.Expression; -import org.eclipse.core.expressions.ExpressionInfo; -import org.eclipse.core.expressions.IEvaluationContext; - -import org.eclipse.core.runtime.ILog; - -import org.eclipse.jface.action.IAction; -import org.eclipse.ui.IActionBars; -import org.eclipse.ui.ISources; import org.eclipse.ui.IWorkbenchPart; import org.eclipse.ui.PlatformUI; -import org.eclipse.ui.contexts.IContextActivation; -import org.eclipse.ui.contexts.IContextService; import org.eclipse.ui.handlers.IHandlerActivation; import org.eclipse.ui.handlers.IHandlerService; -import org.eclipse.ui.part.MultiPageEditorSite; -import org.eclipse.ui.swt.IFocusService; - -import org.eclipse.ui.texteditor.AbstractTextEditor; -import org.eclipse.ui.texteditor.ITextEditorActionConstants; /** - * Owns the Find/Replace overlay's command infrastructure, including context - * activation, handler activation, and key-binding hint updates. + * Owns the Find/Replace overlay's command infrastructure: handler activation and + * key-binding hint updates. + *

+ * The overlay's own commands are activated as handlers once, rather than + * imperatively activated and deactivated on every focus change. They are scoped + * by {@link FindReplaceOverlayContextSupport#overlayFocusedExpression()}, which + * both limits them to the time an input field has focus and tells them apart + * from the handlers of the overlays of other editors, since all of those are + * activated at the workbench for the same commands. *

- * The overlay's own commands are activated as handlers once, scoped by - * {@link #overlayFocusedExpression}, rather than imperatively - * activated/deactivated on every focus change. That expression relies on - * {@link IFocusService} tracking the search/replace bar controls so that - * {@code ACTIVE_FOCUS_CONTROL} reflects them. Context activation (which - * drives key binding resolution and has no expression-based equivalent) - * remains imperative and is updated directly from the overlay's focus - * listeners. + * Everything context related, both the overlay's key binding scopes and keeping + * the editor's own commands from consuming keys meant for the input fields, is + * owned by {@link FindReplaceOverlayContextSupport}. This class only forwards + * the overlay's focus changes to it, because the shortcut hints have to be + * refreshed whenever the active scopes change. */ class FindReplaceOverlayCommandSupport { @@ -81,70 +62,20 @@ class FindReplaceOverlayCommandSupport { static final String CMD_REPLACE_ALL = "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.replaceAll"; //$NON-NLS-1$ - private static final String OVERLAY_CONTEXT_ID = - "org.eclipse.ui.workbench.texteditor.findReplaceOverlay"; //$NON-NLS-1$ - private static final String OVERLAY_SEARCH_CONTEXT_ID = - "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.searchFocused"; //$NON-NLS-1$ - private static final String OVERLAY_REPLACE_CONTEXT_ID = - "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.replaceFocused"; //$NON-NLS-1$ - - private Composite containerControl; - private final IWorkbenchPart targetPart; - private DeactivateGlobalActionHandlers globalActionHandlerDeaction; - - private final List contextActivations = new ArrayList<>(); private final Expression overlayFocusedExpression; + private final FindReplaceOverlayContextSupport contextSupport; private final List registeredActions = new ArrayList<>(); private final List actionActivations = new ArrayList<>(); FindReplaceOverlayCommandSupport(IWorkbenchPart targetPart) { - this.targetPart = targetPart; - this.overlayFocusedExpression = createOverlayFocusedExpression(); - } - - private Expression createOverlayFocusedExpression() { - return new Expression() { - @Override - public EvaluationResult evaluate(IEvaluationContext context) { - Object focusControl = context.getVariable(ISources.ACTIVE_FOCUS_CONTROL_NAME); - if (focusControl instanceof Control control) { - Control current = control; - while (current != null) { - if (current == containerControl) { - return EvaluationResult.TRUE; - } - current = current.getParent(); - } - } - return EvaluationResult.FALSE; - } - - @Override - public void collectExpressionInfo(ExpressionInfo info) { - info.addVariableNameAccess(ISources.ACTIVE_FOCUS_CONTROL_NAME); - } - }; + this.contextSupport = new FindReplaceOverlayContextSupport(targetPart); + this.overlayFocusedExpression = contextSupport.overlayFocusedExpression(); } - void trackFocusControl(Text text) { - IFocusService focusService = PlatformUI.getWorkbench().getService(IFocusService.class); - if (focusService != null) { - focusService.addFocusTracker(text, "" + text.hashCode()); //$NON-NLS-1$ - } - } - - void setContainerControl(Composite containerControl) { - this.containerControl = containerControl; - containerControl.addDisposeListener(__ -> { - deregisterActionActivations(); - // Safety net: normally already done by the focus-lost handling that runs - // while the overlay is closed via close(), but disposal is not guaranteed - // to be preceded by a focus-lost event, so repeat it here defensively. Both - // calls are idempotent if that cleanup already ran. - deactivateContexts(); - setTextEditorActionsActivated(true); - }); + void dispose() { + deregisterActionActivations(); + contextSupport.dispose(); } void registerAction(FindReplaceOverlayAction action) { @@ -178,104 +109,24 @@ private static IHandlerService getWorkbenchHandlerService() { } void searchBarActivated() { - searchOrReplaceBarActivated(OVERLAY_SEARCH_CONTEXT_ID); + contextSupport.searchBarFocused(); + refreshShortcutHints(); } void replaceBarActivated() { - searchOrReplaceBarActivated(OVERLAY_REPLACE_CONTEXT_ID); - } - - private void searchOrReplaceBarActivated(String barContextId) { - setTextEditorActionsActivated(false); - // Defensively clear any contexts still active from a previous activation, - // making this method idempotent instead of relying on a focus-lost event - // always having deactivated them first. - deactivateContexts(); - activateContext(OVERLAY_CONTEXT_ID); - activateContext(barContextId); + contextSupport.replaceBarFocused(); refreshShortcutHints(); } - private void activateContext(String context) { - IContextService contextService = getWorkbenchContextService(); - if (contextService != null) { - contextActivations.add(contextService.activateContext(context)); - } - } - - private static IContextService getWorkbenchContextService() { - return PlatformUI.getWorkbench().getService(IContextService.class); - } - void searchOrReplaceBarDeactivated() { - deactivateContexts(); - setTextEditorActionsActivated(true); + contextSupport.fieldsLostFocus(); refreshShortcutHints(); } - private void deactivateContexts() { - IContextService contextService = getWorkbenchContextService(); - if (contextService != null) { - for (IContextActivation activation : contextActivations.reversed()) { - contextService.deactivateContext(activation); - } - } - contextActivations.clear(); - } - private void refreshShortcutHints() { for (FindReplaceOverlayAction action : registeredActions) { action.updateHint(); } } - /* - * Adapted from - * org.eclipse.jdt.internal.ui.javaeditor.JavaEditor#setActionsActivated(boolean) - */ - private void setTextEditorActionsActivated(boolean state) { - if (!(targetPart instanceof AbstractTextEditor) || targetPart.getSite().getWorkbenchWindow().isClosing()) { - return; - } - if (targetPart.getSite() instanceof MultiPageEditorSite multiEditorSite) { - if (!state && globalActionHandlerDeaction == null) { - globalActionHandlerDeaction = new DeactivateGlobalActionHandlers(multiEditorSite.getActionBars()); - } else if (state && globalActionHandlerDeaction != null) { - globalActionHandlerDeaction.reactivate(); - globalActionHandlerDeaction = null; - } - } - try { - Method method = AbstractTextEditor.class.getDeclaredMethod("setActionActivation", boolean.class); //$NON-NLS-1$ - method.setAccessible(true); - method.invoke(targetPart, Boolean.valueOf(state)); - } catch (IllegalArgumentException | ReflectiveOperationException ex) { - ILog.of(FindReplaceOverlayCommandSupport.class).error("cannot (de-)activate actions for text editor", ex); //$NON-NLS-1$ - } - } - - private static final class DeactivateGlobalActionHandlers { - private static final List ACTIONS = List.of(ITextEditorActionConstants.CUT, - ITextEditorActionConstants.COPY, ITextEditorActionConstants.PASTE, - ITextEditorActionConstants.DELETE, ITextEditorActionConstants.SELECT_ALL, - ITextEditorActionConstants.FIND); - - private final Map deactivatedActions = new HashMap<>(); - private final IActionBars actionBars; - - DeactivateGlobalActionHandlers(IActionBars actionBars) { - this.actionBars = actionBars; - for (String actionID : ACTIONS) { - deactivatedActions.putIfAbsent(actionID, actionBars.getGlobalActionHandler(actionID)); - actionBars.setGlobalActionHandler(actionID, null); - } - } - - void reactivate() { - for (String actionID : deactivatedActions.keySet()) { - actionBars.setGlobalActionHandler(actionID, deactivatedActions.get(actionID)); - } - } - } - } diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayContextSupport.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayContextSupport.java new file mode 100644 index 00000000000..e6ed9b30d8e --- /dev/null +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayContextSupport.java @@ -0,0 +1,244 @@ +/******************************************************************************* + * Copyright (c) 2026 Vector Informatik GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.ui.internal.findandreplace.overlay; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.eclipse.core.expressions.EvaluationResult; +import org.eclipse.core.expressions.Expression; +import org.eclipse.core.expressions.ExpressionInfo; +import org.eclipse.core.expressions.IEvaluationContext; + +import org.eclipse.e4.core.contexts.IEclipseContext; +import org.eclipse.e4.ui.model.application.ui.basic.MBasicFactory; +import org.eclipse.e4.ui.model.application.ui.basic.MPart; +import org.eclipse.e4.ui.services.EContextService; + +import org.eclipse.ui.ISources; +import org.eclipse.ui.IWorkbenchPart; +import org.eclipse.ui.IWorkbenchPartSite; +import org.eclipse.ui.IWorkbenchWindow; +import org.eclipse.ui.PlatformUI; + +/** + * Owns the Find/Replace overlay's contexts, of which there are two kinds: an + * {@link IEclipseContext} that makes the overlay rather than its host editor the + * active part while an input field has focus, and the overlay's key binding + * scopes. The second kind lives inside the first, because scopes are collected + * along the chain between the active leaf and the root: a scope activated here is + * active exactly while this context is the active leaf, so the shared scope is + * activated once and only the per-field scope is switched. + *

+ * Two conditions decide whether one of the editor's handlers wins, and the + * editor's commands are spread over both. Its key binding scopes and part-level + * handlers are only reachable through the editor's part context, which + * activating a sibling of that context takes off the chain. Its retargetable + * actions live in the window context instead, guarded by an expression + * over {@link ISources#ACTIVE_PART_ID_NAME}, which publishing this context's own + * id as the active part id makes false. Together they leave no editor handler in + * the resolution path, so no command has to be suppressed individually. The + * context sits below the window context rather than the application context, + * which would detach window-scoped commands and services as well. + *

+ * The {@link MPart} looks superfluous, since nothing reads it, but removing it + * changes behaviour: it is what lets {@code ActivePartLookupFunction} resolve an + * active part here. Without it that lookup yields {@code null}, and + * {@code PartServiceImpl} answers a null active part by firing part deactivation + * and clearing the active selection. With it, the same code path finds a part + * outside the application model and returns early. That early return is also why + * the part is neither added to the model nor rendered nor activated through + * {@code EPartService}. + *

+ * The alternatives considered, and what was measured about them, are recorded in + * {@code docs/adr/0001-find-replace-overlay-key-handling.md}. + */ +class FindReplaceOverlayContextSupport { + + static final String OVERLAY_PART_ID_PREFIX = "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.part."; //$NON-NLS-1$ + + /** + * Tells the overlays of different editors apart: their command handlers are all + * activated at the workbench, so the id must not be shared. + */ + private static final AtomicInteger PART_ID_SEQUENCE = new AtomicInteger(); + + private static final String OVERLAY_CONTEXT_ID = "org.eclipse.ui.workbench.texteditor.findReplaceOverlay"; //$NON-NLS-1$ + + private static final String OVERLAY_SEARCH_CONTEXT_ID = "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.searchFocused"; //$NON-NLS-1$ + + private static final String OVERLAY_REPLACE_CONTEXT_ID = "org.eclipse.ui.workbench.texteditor.findReplaceOverlay.replaceFocused"; //$NON-NLS-1$ + + private final IWorkbenchPart targetPart; + + private final String overlayPartId = OVERLAY_PART_ID_PREFIX + PART_ID_SEQUENCE.incrementAndGet(); + + private IEclipseContext overlayContext; + + /** + * The leaf that was active before the overlay took over, restored when it gives + * focus back. Remembered rather than derived from the target part, which also + * works without a part and does not need the part's site to still be there. Held + * only while the overlay owns the active leaf, so that handing it back does not + * keep the context of an editor closed in the meantime reachable. + */ + private IEclipseContext contextToRestore; + + private boolean overlayContextActive; + + private final Expression overlayFocusedExpression = createOverlayFocusedExpression(); + + /** + * The per-field scope currently activated in {@link #overlayContext}. Only ever + * switched: while that context is off the active chain its scopes are inert. + */ + private String activeFieldContextId; + + FindReplaceOverlayContextSupport(IWorkbenchPart targetPart) { + this.targetPart = targetPart; + this.overlayContext = createOverlayContext(); + } + + private IEclipseContext createOverlayContext() { + IEclipseContext windowContext = getWindowContext(); + if (windowContext == null) { + return null; + } + MPart overlayPart = MBasicFactory.INSTANCE.createPart(); + overlayPart.setElementId(overlayPartId); + + IEclipseContext context = windowContext.createChild(overlayPartId); + context.set(MPart.class, overlayPart); + // Only the id is overridden, not ACTIVE_PART_NAME: the overlay operates on the + // editor, and so does anything invoked while the overlay has focus, so the + // active part itself must keep pointing at the editor. + context.set(ISources.ACTIVE_PART_ID_NAME, overlayPartId); + overlayPart.setContext(context); + + context.get(EContextService.class).activateContext(OVERLAY_CONTEXT_ID); + return context; + } + + /** + * Holds exactly while one of this overlay's input fields has focus, for scoping + * activations to this overlay alone. It tests the active part id this context + * publishes, which is unique per overlay. Declaring that variable in + * {@code collectExpressionInfo} is what gives such activations their source + * priority, so the expression belongs next to the code setting the variable. + */ + Expression overlayFocusedExpression() { + return overlayFocusedExpression; + } + + private Expression createOverlayFocusedExpression() { + return new Expression() { + @Override + public EvaluationResult evaluate(IEvaluationContext context) { + return EvaluationResult + .valueOf(overlayPartId.equals(context.getVariable(ISources.ACTIVE_PART_ID_NAME))); + } + + @Override + public void collectExpressionInfo(ExpressionInfo info) { + info.addVariableNameAccess(ISources.ACTIVE_PART_ID_NAME); + } + }; + } + + void searchBarFocused() { + fieldFocused(OVERLAY_SEARCH_CONTEXT_ID); + } + + void replaceBarFocused() { + fieldFocused(OVERLAY_REPLACE_CONTEXT_ID); + } + + private void fieldFocused(String fieldContextId) { + if (overlayContext == null) { + return; + } + if (!fieldContextId.equals(activeFieldContextId)) { + EContextService contextService = overlayContext.get(EContextService.class); + if (activeFieldContextId != null) { + contextService.deactivateContext(activeFieldContextId); + } + contextService.activateContext(fieldContextId); + activeFieldContextId = fieldContextId; + } + if (!overlayContextActive) { + contextToRestore = overlayContext.getParent().getActiveLeaf(); + overlayContextActive = true; + } + overlayContext.activate(); + } + + void fieldsLostFocus() { + if (!overlayContextActive) { + return; + } + overlayContextActive = false; + IEclipseContext toRestore = contextToRestore; + contextToRestore = null; + if (toRestore != null) { + handBackActiveLeaf(toRestore); + } + } + + /** + * Gives the active leaf back to what held it before the overlay took over, but + * only while the overlay still holds it. Losing the focus to another part does + * not come through here as a hand-back: the workbench activates the part under + * the mouse before the focus leaves the field, so that part already owns the leaf + * by now and has to keep it. + *

+ * Handing it to the editor anyway would put the editor's key binding scope up + * beside the one the part coming up brings, and while both are up every key the + * two scopes have in common is an unresolvable binding conflict. Whether that + * surfaces depends on how the other part holds its scope, which is why it is + * observed with the console but not with an ordinary view. + *

+ * Activating the overlay's context replaced the window context's active child, so + * the whole chain down to the context to restore has to be re-established, not + * just its last link. + */ + private void handBackActiveLeaf(IEclipseContext toRestore) { + if (overlayContext != null && overlayContext.getParent().getActiveLeaf() == overlayContext) { + toRestore.activateBranch(); + } + } + + void dispose() { + if (overlayContext != null) { + // The context is about to go, and disposing it while it holds the active leaf + // would leave the window context itself as the leaf, with the editor's scopes + // on no chain at all. + fieldsLostFocus(); + overlayContext.dispose(); + overlayContext = null; + } + } + + /** + * The window the overlay belongs to. Only when there is no part at all, which + * the find/replace UI tests exercise, does the overlay fall back to the active + * window rather than guessing one for a part whose site is unavailable. + */ + private IEclipseContext getWindowContext() { + IWorkbenchWindow window; + if (targetPart == null) { + window = PlatformUI.getWorkbench().getActiveWorkbenchWindow(); + } else { + IWorkbenchPartSite site = targetPart.getSite(); + window = site == null ? null : site.getWorkbenchWindow(); + } + return window == null ? null : window.getService(IEclipseContext.class); + } + +} diff --git a/docs/adr/0001-find-replace-overlay-key-handling.md b/docs/adr/0001-find-replace-overlay-key-handling.md new file mode 100644 index 00000000000..323de78623b --- /dev/null +++ b/docs/adr/0001-find-replace-overlay-key-handling.md @@ -0,0 +1,321 @@ +# 1. Find/Replace overlay: key event and editor command handling + +How the Find/Replace overlay makes sure that the right commands are in effect while one of its +input fields has focus: its own, and those of the surrounding workbench, but not those of the +editor it is drawn on, whose key bindings and handlers would otherwise carry out edits on the +document rather than in the field the user is typing into. The overlay is not a workbench part, +so the framework does not separate the two on its own. This decision covers how that separation +is achieved, and how it is achieved without tying the overlay to a particular kind of editor. + +## Status + +Accepted. + +## Context + +The Find/Replace overlay shows text input fields on top of a text editor. Its `containerControl` +is a plain SWT `Composite` parented into the editor's widget tree. + +Being embedded rather than living in its own `Shell` is a hard requirement: a shell has to +manually follow the target widget's move, resize and hide/show operations, always lags behind by +some milliseconds, and cannot be positioned at all under Wayland (see commit `78c9a1c60a`, which +introduced the embedded composite for these reasons). **A separate shell is therefore out of +scope**, which constrains the whole design space below. + +The consequence of being embedded is the root of the problem: focus in the overlay's fields does +not change the active workbench part. The editor stays active, so its key-binding scopes and its +command handlers stay in effect and compete with the overlay for every keystroke. + +### Goals + +1. Standard text editing keys (Ctrl+C/V/X/Z/A, Ctrl+Backspace, word navigation, Home/End, + arrows, Delete) must operate on the overlay's input fields, not on the editor's document. +2. The retargetable global actions (Edit > Cut/Copy/Paste/Select All) must be bound to the + overlay's fields while they have focus, so the Edit menu and toolbar act on them. +3. Commands in the "In Windows" scope (Save, Close, Next Editor, Preferences, ...) must stay + executable while the overlay has focus. +4. The overlay must not depend on `AbstractTextEditor` or `StatusTextEditor`, so it can later + serve non-text editors. +5. No reflective access to private platform API. + +### How Eclipse decides which handler runs + +Two independent gates decide whether a handler activation wins. + +**Gate 1, reachability.** `HandlerServiceImpl#lookUpHandler` resolves +`context.getActiveLeaf().get("handler::" + commandId)`, and the `HandlerSelectionFunction` +installed there walks from that leaf up to the root. An activation in a context that is not an +ancestor of the active leaf is never considered. Key-binding scopes work the same way: +`ActiveContextsFunction` unions the `localContexts` of every context from the active leaf +upwards, and that union is what `ContextManager` and the E4 `BindingService` use. + +**Gate 2, expression.** Among reachable activations, only those whose `activeWhen` expression +evaluates to true participate. The winner is chosen by `HandlerActivation#compareTo` on source +priorities computed by `SourcePriorityNameMapping#computeSourcePriority` from the variables the +expression accesses. There is no part/window layering in this comparison; `getDepth()` is +always `0`. Note that `activeFocusControl` maps to `ISources.ACTIVE_MENU` (`1 << 31`), which +`compareTo` normalises into `1 << 30`, higher than any other source. That, and not any notion +of part-level versus window-level, is why an `ACTIVE_FOCUS_CONTROL`-scoped activation outranks +the editor's. + +Legacy `ISources` variables are resolved by `ExpressionContext#getVariable` through +`IEclipseContext#getActive(name)`, that is, **from the active leaf upwards**, so a context +nearer the leaf can shadow them. + +A key reaches the widget when the winning handler reports `isHandled() == false`: +`KeyBindingDispatcher#executeCommand` computes `commandHandled` from the handler, `press()` +returns false, `processKeyEvent` leaves `event.doit` untouched, and SWT delivers the key +natively. A command with no handler at all behaves the same way. + +### Where the editor registers what + +The editor's commands do not all live in one place, which is what makes the problem non-obvious: + +| Registered by | Into which context | Guarded by | +|---|---|---| +| Editor's `KeyBindingService` (scopes such as `org.eclipse.ui.textEditorScope`) | the **part** context | n/a | +| Editor's own `IHandlerService.activateHandler` calls | the **part** context | `ActivePartExpression` | +| Retargetable actions (cut, copy, paste, delete, select all, undo, redo) via `IActionBars.setGlobalActionHandler` | the **window** context, because `EditorReference` constructs `EditorActionBars` with `page.getWorkbenchWindow()` as service locator | `LegacyEditorActionBarExpression`, comparing `activePartId` against the editor id | + +Any approach that only detaches the part context addresses the first two rows and leaves the +third untouched. The third row holds exactly the commands users notice most. + +### Context inheritance of the overlay's own scope + +`ContextSet` resolves parent contexts when building the set a binding lookup runs against. If +the overlay's own key-binding context declares `parentId="org.eclipse.ui.textEditorScope"`, the +editor scope is pulled back in **even when the editor's part context has been taken off the +active chain**, and Ctrl+Delete still resolves to `deleteNextWord`. Giving that context a parent +outside the editor scope is therefore a precondition for the chosen alternative, not cosmetic +tidying towards goal 4. + +## Decision + +While one of the overlay's input fields has focus, the overlay activates an `IEclipseContext` of +its own below the window context and publishes its own id as the active part id. That takes the +editor's key binding scopes and part-level handlers off the active context chain, and makes the +expression guarding its retargetable actions evaluate to false, so no editor handler is left in +the command resolution path and no individual command has to be suppressed. The overlay's own key +binding scopes are activated in that same context, which also gives them their lifetime, and its +shared scope is given a parent outside `org.eclipse.ui.textEditorScope`. + +### Alternatives + +**Suppress the editor's commands individually.** The editor stays the active part, and for every +command whose key an input field needs, its handler is neutralised or replaced while the field +has focus, so that the key falls through to the widget. This leaves the workbench's view of the +world untouched, since the editor remains the active part throughout. It needs a set of affected +command ids, which is either maintained by hand, and then misses commands contributed by other +plugins, or derived from the current key bindings, and then unbounded. Suppression works per +command id, so a command bound to both a native and a non-native key loses both. It also only +takes the keys away from the editor: nothing binds them to the input field, so cut, copy and +paste stay unbound and the menu entries appear enabled while doing nothing, leaving goal 2 unmet. + +**Make the overlay a workbench element of its own.** Give the overlay its own shell, or model it +as a real part and activate it, and the framework's own activation takes the editor's contexts +and handlers off the active chain. This needs no per-command work at all, and is how content +assist, Quick Access and dialogs already avoid the problem. A shell is excluded by the embedding +requirement stated in the context. A real part changes the part service's notion of the active +part, so part listeners, the selection service, Outline, link-with-editor and the editor's tab +all react, and it needs a home in the application model that a floating overlay does not +naturally have. + +**Give the overlay a context of its own (adopted).** Create an `IEclipseContext` for the overlay +and activate it while an input field has focus, without the overlay becoming a part as far as the +part service is concerned. This addresses both gates at once and needs no command set: the +editor's part-level registrations become unreachable, and its window-level ones evaluate to +false. The platform's own default handlers for cut, copy, paste and select all then act on the +focused field, which meets goal 2 without registering anything, while window-scoped commands stay +reachable. Its cost is that it relies on a divergence between the active-leaf chain and +`EPartService`'s notion of the active part which E4 does not promise to preserve, that the +`MPart` it carries exists only in a context and not in the application model, and that shadowing +`activePartId` also switches off contributions keyed on that variable against the host editor. + +### Observed behaviour + +With the search field focused in a text editor. The columns are variants described in the +appendix. + +| | Non-handling overrides | Own context, window | Own context, application | **Adopted** | +|---|---|---|---|---| +| `textEditorScope` active | yes | no | no | **no** | +| Ctrl+Delete resolves | yes | no | no | **no** | +| `edit.undo` / `edit.delete` handler | non-handling override | editor | none | **none** | +| `edit.copy` / `edit.selectAll` handler | non-handling override | editor | platform default | **platform default** | +| `deleteNextWord` handler | non-handling override | none | none | **none** | +| Save / Close / Next Editor / Preferences | ok | ok | ok | **ok** | +| `IWorkbenchWindow` from leaf | ok | ok | **null** | **ok** | +| `ISources.activePart` / `activeEditor` | editor | editor | **undefined** / editor | **editor** | +| Overlay's own bindings and handlers | ok | ok | ok | **ok** | + +## Consequences + +- Nothing in the key handling depends on the editor type, and the overlay does not reference + `AbstractTextEditor`, as goal 4 requires. +- Cut, copy, paste and select all work on the overlay's fields through the platform's default + handlers, including from the Edit menu. +- Undo, delete and the word-wise editing and navigation commands resolve to no handler while the + overlay has focus, so the native `Text` widget handles those keys. +- Taking the editor out of the resolution path does not take the bindings of its commands with it: + those declared without a `contextId` live in the window scope, which stays active, so such a key + still resolves to its command and then finds nothing to run. Commands that remain meaningful + inside the overlay are therefore adopted, by activating a handler of the overlay's own for the + editor's command id and implementing the operation in terms of the overlay's state. Adopting the + id rather than the key sequence keeps this working when the user rebinds the command. Which + commands are adopted is decided one by one, since it only makes sense where the overlay has a + meaning of its own for the command. +- Multi-page editors need no handling of their own, and the mechanism that used to provide it is + removed rather than replaced. `LegacyEditorActionBarExpression` compares `activePartId` against + the editor id it was constructed with and does nothing else, so shadowing that variable with the + overlay's id makes it false whichever editor id it carries and whichever of a multi-page editor's + two registration paths put the handler there. The gap that made the contributor variant fail, and + that `DeactivateGlobalActionHandlers` was introduced to close, therefore cannot arise here. This + follows from the expression's implementation; no test pins it, since the existing find/replace + tests do not exercise multi-page editors. +- `org.eclipse.ui.workbench.texteditor` gains a dependency on the E4 context and model bundles. +- Per-focus imperative state is limited to activating and deactivating that one context plus + switching which of the two per-field scopes is activated inside it. The overlay's shared scope + is activated once, because scopes activated in the overlay's context are active exactly while + that context is the active leaf. +- Handing the active leaf back when an input field loses focus must happen only while the overlay + still holds that leaf. A click activates the part under the mouse before the focus leaves the + field, so on that path the new part already owns the leaf and has to keep it. Handing it to the + editor anyway raises the editor's key-binding scope beside the one the new part brings, and while + both are up every stroke the two scopes share is an unresolvable binding conflict. Observed by + clicking from the overlay into the console, which reports conflicting zoom bindings; it is + reported from `PartServiceImpl#activate`, when the context updates deferred for the part switch + are flushed. Whether it surfaces depends on how the other part holds its scope: a scope that + rides the active-leaf chain drops as the leaf moves, which is why an ordinary view does not show + it. Clicking into the console without the overlay involved does not produce it either. + +## Appendix: variants and dead ends + +### Suppressing the editor's commands + +**Reflection into `AbstractTextEditor#setActionActivation`, plus nulling the action bars.** On +focus gained, call the private `setActionActivation(false)` reflectively, and for multi-page +editors additionally null out the `IActionBars` global action handlers for +cut/copy/paste/delete/select-all/find. Reverse both on focus lost. Addresses both registration +sites, but uses reflective private API against goal 5, is hard-coded to `AbstractTextEditor` and +therefore a no-op for any other editor against goal 4, and covers only the six global action ids, +not word navigation or undo. + +**Non-handling handler overrides.** Collect the commands bound to the ~40 key sequences that SWT +`Text` handles natively and, for each, activate a handler at the target part's `IHandlerService` +that returns `isHandled() == false`, scoped by an expression on `ACTIVE_FOCUS_CONTROL`, whose +rogue-bit priority outranks the editor's activations. Needs no reflection and is not tied to an +editor class, but each `activateHandler` also sets `handler::` in the part context through +`EHandlerService`, and `deactivateHandlers` only clears the `legacy::handler::` list, so that +entry outlives the overlay and shadows any pure-E4 handler for that command for the rest of the +editor's life. Deriving the command set from `BindingManager#getBindings()` returns every +declared binding across all schemes, platforms and locales, may return `null`, and is a snapshot +that ignores later changes in Preferences > Keys; a fixed list avoids that but misses third-party +commands. + +**`IEditorActionBarContributor#setActiveEditor(null)` on focus.** Makes the contributor call +`IActionBars.setGlobalActionHandler(id, null)` for each action it manages, removing the +window-level activations, restored on focus lost. Public API, and correct for single-page +editors, but `MultiPageEditorSite#getActionBarContributor()` returns `null` by specification and +the fallback via the outer editor does not reliably cover handlers the inner editor registered. +Addresses only the global action bar subset. + +The reason the fallback does not suffice is that a multi-page editor's global action handlers are +registered twice over the same shared `SubActionBars`. The platform calls +`outerContributor.setActiveEditor(outerEditor)` when the multi-page editor becomes active, and +then focusing an inner `AbstractTextEditor` page runs its `setActionActivation(true)`, whose +private `setActiveEditor` resolves the very same outer contributor through +`MultiPageEditorSite#getMultiPageEditor()` and calls it again with the *inner* editor, overwriting +the first registration. Asking that contributor to forget its active editor clears only what it +itself tracks, which need not match what the inner editor put there, so a generic +`MultiPageEditorActionBarContributor` leaves the inner editor's paste handler active. That the two +registration sites really are distinct is on record: commit `69f6e45a` had to add +`DeactivateGlobalActionHandlers` for multi-page editors *in addition to* the reflection call of +the previous variant, which would have been unnecessary had addressing the contributor been +enough. + +**Explicit pass-through handlers.** Register real handlers calling the corresponding `Text` +method. Expresses intent directly and avoids the enablement problem, but SWT `Text` exposes only +`cut()`, `copy()`, `paste()` and `selectAll()`, so most of the ~30 affected operations cannot be +implemented this way, let alone correctly across platforms and locale-sensitive word boundaries. + +### Making the overlay a workbench element of its own + +**Own `Shell`.** `ShellActivationListener` gives an unmodelled shell a child context of the +application context, activates it, and activates `org.eclipse.ui.contexts.dialog`. Complete +isolation for free, but reintroduces exactly what the embedded composite exists to avoid. + +**Real `MPart` activated through `EPartService`.** The threshold is +`PartServiceImpl#isInContainer`, true once the part is found by a `PRESENTATION`-scoped model +search or tagged as a hosted element in the window's shared elements. Crossing it has two +observable effects: `PartServiceImpl` activates the part, and since it carries no compatibility +wrapper `IWorkbenchPage#getActivePart()` then reports `null` while the overlay has focus and part +deactivation is broadcast for the editor; and placing the part in the editor area additionally +renders it, which takes focus away from the input field altogether. + +### Giving the overlay a context of its own + +**Parented on the window context.** Removes `textEditorScope` and the editor's part-level +handlers with no per-command work, and does not disturb the part service. Insufficient on its +own: the retargetable actions are registered at the window handler service, the window context +remains an ancestor, and `activePartId` still resolves to the editor, so undo, copy, delete and +select all keep resolving to the editor's handlers. + +**Parented on the application context.** The topology `ShellActivationListener` gives a dialog +shell, which additionally takes the window context off the chain. Full isolation, and the +platform's default handlers surface as described in the decision, but window-scoped services stop +being injectable (`IWorkbenchWindow` resolves to `null`) and the legacy `activePart` variable +becomes undefined, so `HandlerUtil#getActivePart` and any `activeWhen` keyed on the active part +go false, putting goal 3 at risk. + +**Parented on the window context, shadowing `activePartId` (adopted).** Setting +`ISources.ACTIVE_PART_ID_NAME` to the overlay's own id in that context makes +`LegacyEditorActionBarExpression` evaluate to false, since legacy variables resolve from the +active leaf upwards. The editor's window-level activations stay reachable but stop participating, +so gate 2 does the work that detaching the window context does in the previous variant, without +its cost. Only the id is shadowed, not `ACTIVE_PART_NAME`, so anything needing a part still sees +the editor. The context activation is also stable under ordinary interaction: clicking into the +fields, moving focus to an overlay toolbar button and back, and switching between editor and +overlay by mouse all leave the overlay's context as the active leaf, despite the `SWT.Activate` +listener `ContributedPartRenderer` installs on the editor's composite. + +The `MPart` carried by the context is what lets `ActivePartLookupFunction` resolve an active part +for it. Without it that lookup yields `null`, and `PartServiceImpl` answers a null active part by +firing part deactivation and clearing the active selection. With it, the same code path finds a +part outside the application model and returns early, leaving the part service untouched. + +### Context manipulations that do not remove the editor from resolution + +- **`EContextService#deactivateContext("org.eclipse.ui.textEditorScope")` on the part's + context** removes the id from that context's `localContexts`, but the editor's + `KeyBindingService` owns and re-establishes those activations. +- **`IContextService#deactivateContext` (3.x)** needs the `IContextActivation` token, which + belongs to the editor's `KeyBindingService`. +- **`ContextManager#setActiveContextIds`** is not exposed through `IContextService`; reaching it + means casting to the internal implementation, which goal 5 excludes. +- **`IContextService#activateContext(id, falseExpression)`** cannot help, because context + activations are additive. +- **A child `IEclipseContext` of the *part* context** leaves the part context an ancestor, so + `ActiveContextsFunction` still finds `textEditorScope`. Only a context that is not a descendant + of the part context helps. + +### The focus service + +`IFocusService#addFocusTracker(control, id)` makes `ACTIVE_FOCUS_CONTROL` and +`ACTIVE_FOCUS_CONTROL_ID` reflect the overlay's fields when they have focus. It is the documented +way to attach `WidgetMethodHandler`-based cut/copy/paste to a widget outside the part lifecycle, +and it is the source whose priority lets the suppression variants outrank the editor. It changes +nothing on its own, and `FocusControlSourceProvider` clears the variables on focus lost of a +tracked control, so anything keyed on them is inactive while an overlay toolbar button has focus. +For the adopted alternative it is redundant, because `activePartId` already changes at exactly +the same moments and additionally identifies *which* overlay is focused. + +## Related + +- `FindReplaceOverlayCommandSupport`, `FindReplaceOverlayContextSupport` +- Commit `78c9a1c60a`, which replaced the shell-based overlay with the embedded composite +- Commit `69f6e45a` and issue + , the multi-page editor + paste defect that motivated `DeactivateGlobalActionHandlers` +- Issue +- Issue , the workbench-part + proposal