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.

Sign up for GitHub

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

Remove reflective access from find/replace tests #2060 #2333

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Remove reflective access from find/replace tests #2060 #2333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions ...nch.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ private final class KeyboardShortcuts {
KeyStroke.getInstance(SWT.MOD1, 'R'), KeyStroke.getInstance(SWT.MOD1, 'r'));
}

public static final String ID_DATA_KEY = "org.eclipse.ui.internal.findreplace.overlay.FindReplaceOverlay.id"; //$NON-NLS-1$

private static final String REPLACE_BAR_OPEN_DIALOG_SETTING = "replaceBarOpen"; //$NON-NLS-1$
private static final double WORST_CASE_RATIO_EDITOR_TO_OVERLAY = 0.95;
private static final double BIG_WIDTH_RATIO_EDITOR_TO_OVERLAY = 0.7;
Expand All @@ -130,9 +132,9 @@ private final class KeyboardShortcuts {
private ToolItem wholeWordSearchButton;
private ToolItem caseSensitiveSearchButton;
private ToolItem regexSearchButton;
private ToolItem searchUpButton;
private ToolItem searchDownButton;
private ToolItem searchAllButton;
private ToolItem searchBackwardButton;
private ToolItem searchForwardButton;
private ToolItem selectAllButton;
private AccessibleToolBar closeTools;
private ToolItem closeButton;

Expand Down Expand Up @@ -370,6 +372,7 @@ public int open() {
}
overlayOpen = true;
applyOverlayColors(backgroundToUse, true);
assignIDs();
updateFromTargetSelection();
searchBar.forceFocus();

Expand All @@ -391,6 +394,25 @@ private void restoreOverlaySettings() {
}
}

@SuppressWarnings("nls")
private void assignIDs() {
replaceToggle.setData(ID_DATA_KEY, "replaceToggle");
searchBar.setData(ID_DATA_KEY, "searchInput");
searchBackwardButton.setData(ID_DATA_KEY, "searchBackward");
searchForwardButton.setData(ID_DATA_KEY, "searchForward");
selectAllButton.setData(ID_DATA_KEY, "selectAll");
searchInSelectionButton.setData(ID_DATA_KEY, "searchInSelection");
wholeWordSearchButton.setData(ID_DATA_KEY, "wholeWordSearch");
regexSearchButton.setData(ID_DATA_KEY, "regExSearch");
caseSensitiveSearchButton.setData(ID_DATA_KEY, "caseSensitiveSearch");

if (replaceBarOpen) {
HeikoKlare marked this conversation as resolved.
Show resolved Hide resolved
replaceBar.setData(ID_DATA_KEY, "replaceInput");
replaceButton.setData(ID_DATA_KEY, "replaceOne");
replaceAllButton.setData(ID_DATA_KEY, "replaceAll");
}
}

private void applyOverlayColors(Color color, boolean tryToColorReplaceBar) {
closeTools.setBackground(color);
closeButton.setBackground(color);
Expand All @@ -400,9 +422,9 @@ private void applyOverlayColors(Color color, boolean tryToColorReplaceBar) {
wholeWordSearchButton.setBackground(color);
regexSearchButton.setBackground(color);
caseSensitiveSearchButton.setBackground(color);
searchAllButton.setBackground(color);
searchUpButton.setBackground(color);
searchDownButton.setBackground(color);
selectAllButton.setBackground(color);
HeikoKlare marked this conversation as resolved.
Show resolved Hide resolved
searchBackwardButton.setBackground(color);
searchForwardButton.setBackground(color);

searchBarContainer.setBackground(color);
searchBar.setBackground(color);
Expand Down Expand Up @@ -511,20 +533,20 @@ private void createSearchTools() {

searchTools.createToolItem(SWT.SEPARATOR);

searchUpButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
searchBackwardButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
.withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_FIND_PREV))
.withToolTipText(FindReplaceMessages.FindReplaceOverlay_upSearchButton_toolTip)
.withOperation(() -> performSearch(false))
.withShortcuts(KeyboardShortcuts.SEARCH_BACKWARD).build();

searchDownButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
searchForwardButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
.withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_FIND_NEXT))
.withToolTipText(FindReplaceMessages.FindReplaceOverlay_downSearchButton_toolTip)
.withOperation(() -> performSearch(true))
.withShortcuts(KeyboardShortcuts.SEARCH_FORWARD).build();
searchDownButton.setSelection(true); // by default, search down
searchForwardButton.setSelection(true); // by default, search down

searchAllButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
selectAllButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
.withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_SEARCH_ALL))
.withToolTipText(FindReplaceMessages.FindReplaceOverlay_searchAllButton_toolTip)
.withOperation(this::performSelectAll).withShortcuts(KeyboardShortcuts.SEARCH_ALL).build();
Expand Down Expand Up @@ -759,6 +781,7 @@ private void createReplaceDialog() {

updatePlacementAndVisibility();
applyOverlayColors(backgroundToUse, true);
assignIDs();
replaceBar.forceFocus();
}

Expand Down
22 changes: 22 additions & 0 deletions .../org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@
*/
class FindReplaceDialog extends Dialog {

public static final String ID_DATA_KEY = "org.eclipse.ui.texteditor.FindReplaceDialog.id"; //$NON-NLS-1$

private static final int CLOSE_BUTTON_ID = 101;
private IFindReplaceLogic findReplaceLogic;

Expand Down Expand Up @@ -275,6 +277,7 @@ public void create() {
shell.setText(FindReplaceMessages.FindReplace_Dialog_Title);

updateButtonState();
assignIDs();
}

/**
Expand Down Expand Up @@ -1353,4 +1356,23 @@ private String getCurrentSelection() {
return null;
return target.getSelectionText();
}

@SuppressWarnings("nls")
private void assignIDs() {
fFindField.setData(ID_DATA_KEY, "searchInput");
fReplaceField.setData(ID_DATA_KEY, "replaceInput");
fForwardRadioButton.setData(ID_DATA_KEY, "searchForward");
fGlobalRadioButton.setData(ID_DATA_KEY, "globalSearch");
fSelectedRangeRadioButton.setData(ID_DATA_KEY, "searchInSelection");
fCaseCheckBox.setData(ID_DATA_KEY, "caseSensitiveSearch");
fWrapCheckBox.setData(ID_DATA_KEY, "wrappedSearch");
fWholeWordCheckBox.setData(ID_DATA_KEY, "wholeWordSearch");
fIncrementalCheckBox.setData(ID_DATA_KEY, "incrementalSearch");
fIsRegExCheckBox.setData(ID_DATA_KEY, "regExSearch");

fReplaceSelectionButton.setData(ID_DATA_KEY, "replaceOne");
fReplaceFindButton.setData(ID_DATA_KEY, "replaceFindOne");
fReplaceAllButton.setData(ID_DATA_KEY, "replaceAll");
}

}
83 changes: 83 additions & 0 deletions ...orkbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/WidgetExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*******************************************************************************
* Copyright (c) 2024 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
*
* Contributors:
* Vector Informatik GmbH - initial API and implementation
*******************************************************************************/
package org.eclipse.ui.internal.findandreplace;
HeikoKlare marked this conversation as resolved.
Show resolved Hide resolved

import static org.junit.Assert.assertFalse;

import java.util.ArrayList;
import java.util.List;

import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Combo;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.ToolBar;
import org.eclipse.swt.widgets.ToolItem;
import org.eclipse.swt.widgets.Widget;

import org.eclipse.ui.internal.findandreplace.overlay.HistoryTextWrapper;

public final class WidgetExtractor {

private final Composite rootContainer;

private final String idDataKey;

public WidgetExtractor(String idDataKey, Composite container) {
this.idDataKey= idDataKey;
this.rootContainer= container;
}

public HistoryTextWrapper findHistoryTextWrapper(String id) {
return findWidget(rootContainer, HistoryTextWrapper.class, id);
}

public Combo findCombo(String id) {
return findWidget(rootContainer, Combo.class, id);
}

public Button findButton(String id) {
return findWidget(rootContainer, Button.class, id);
}

public ToolItem findToolItem(String id) {
return findWidget(rootContainer, ToolItem.class, id);
}

private <T extends Widget> T findWidget(Composite container, Class<T> type, String id) {
List<T> widgets= findWidgets(container, type, id);
assertFalse("more than one matching widget found for id '" + id + "':" + widgets, widgets.size() > 1);
return widgets.isEmpty() ? null : widgets.get(0);
}

private <T extends Widget> List<T> findWidgets(Composite container, Class<T> type, String id) {
List<Widget> children= new ArrayList<>();
children.addAll(List.of(container.getChildren()));
if (container instanceof ToolBar toolbar) {
children.addAll(List.of(toolbar.getItems()));
}
List<T> result= new ArrayList<>();
for (Widget child : children) {
if (type.isInstance(child)) {
if (id.equals(child.getData(idDataKey))) {
result.add(type.cast(child));
}
}
if (child instanceof Composite compositeChild) {
result.addAll(findWidgets(compositeChild, type, id));
}
}
return result;
}

}
4 changes: 2 additions & 2 deletions ...itor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public class FindReplaceOverlayTest extends FindReplaceUITest<OverlayAccess> {
public OverlayAccess openUIFromTextViewer(TextViewer viewer) {
Accessor actionAccessor= new Accessor(getFindReplaceAction(), FindReplaceAction.class);
actionAccessor.invoke("showOverlayInEditor", null);
Accessor overlayAccessor= new Accessor(actionAccessor.get("overlay"), "org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlay", getClass().getClassLoader());
return new OverlayAccess(getFindReplaceTarget(), overlayAccessor);
FindReplaceOverlay overlay= (FindReplaceOverlay) actionAccessor.get("overlay");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FindReplaceOverlay overlay= (FindReplaceOverlay) actionAccessor.get("overlay");
FindReplaceOverlay overlay= (FindReplaceOverlay) viewer.getControl().getData(FindReplaceOverlay.class.getName()

Just as an example on how one can "transport" an object through the data of the underlying control, of course the FindReplaceOverlay needs to set the data accordingly once it is installed in a TextViewer and remove itself once it is removed. That way it is very easy to check if a given TextViewer has a FindReplaceOverlay or not (not only for tests).

If this is needed on multiple places one can even has

static Optional<FindReplaceOverlay> FindReplaceOverlay.of(TextViewer viewer)

that encapsulates the details on how a FindReplaceOverlay know its connection to a TextViewer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like a great idea for further reducing reflective access of the tests (in this case related to the FindReplaceAction instead of the dialog/overlay itself) and maybe can also improve the FindReplaceAction itself because it could relieve the action from tracking the overlay by just attaching it to the target text editor. I would like to follow-up on this in a subsequent PR.

return new OverlayAccess(getFindReplaceTarget(), overlay);
}

@Test
Expand Down
64 changes: 32 additions & 32 deletions ...ch.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/OverlayAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import org.eclipse.swt.SWT;
Expand All @@ -31,13 +30,12 @@
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.ToolItem;

import org.eclipse.text.tests.Accessor;

import org.eclipse.jface.text.IFindReplaceTarget;
import org.eclipse.jface.text.IFindReplaceTargetExtension;

import org.eclipse.ui.internal.findandreplace.IFindReplaceUIAccess;
import org.eclipse.ui.internal.findandreplace.SearchOptions;
import org.eclipse.ui.internal.findandreplace.WidgetExtractor;

class OverlayAccess implements IFindReplaceUIAccess {
private final IFindReplaceTarget findReplaceTarget;
Expand All @@ -64,28 +62,33 @@ class OverlayAccess implements IFindReplaceUIAccess {

private ToolItem replaceAllButton;

private final Runnable closeOperation;

private final Accessor dialogAccessor;
private final FindReplaceOverlay overlay;

private final Supplier<Shell> shellRetriever;
private final Shell shell;

OverlayAccess(IFindReplaceTarget findReplaceTarget, Accessor findReplaceOverlayAccessor) {
OverlayAccess(IFindReplaceTarget findReplaceTarget, FindReplaceOverlay findReplaceOverlay) {
this.findReplaceTarget= findReplaceTarget;
dialogAccessor= findReplaceOverlayAccessor;
find= (HistoryTextWrapper) findReplaceOverlayAccessor.get("searchBar");
replace= (HistoryTextWrapper) findReplaceOverlayAccessor.get("replaceBar");
caseSensitive= (ToolItem) findReplaceOverlayAccessor.get("caseSensitiveSearchButton");
wholeWord= (ToolItem) findReplaceOverlayAccessor.get("wholeWordSearchButton");
regEx= (ToolItem) findReplaceOverlayAccessor.get("regexSearchButton");
searchForward= (ToolItem) findReplaceOverlayAccessor.get("searchDownButton");
searchBackward= (ToolItem) findReplaceOverlayAccessor.get("searchUpButton");
closeOperation= () -> findReplaceOverlayAccessor.invoke("close", null);
openReplaceDialog= (Button) findReplaceOverlayAccessor.get("replaceToggle");
replaceButton= (ToolItem) findReplaceOverlayAccessor.get("replaceButton");
replaceAllButton= (ToolItem) findReplaceOverlayAccessor.get("replaceAllButton");
inSelection= (ToolItem) findReplaceOverlayAccessor.get("searchInSelectionButton");
shellRetriever= () -> ((Shell) findReplaceOverlayAccessor.invoke("getShell", null));
overlay= findReplaceOverlay;
shell= overlay.getShell();
WidgetExtractor widgetExtractor= new WidgetExtractor(FindReplaceOverlay.ID_DATA_KEY, shell);
find= widgetExtractor.findHistoryTextWrapper("searchInput");
caseSensitive= widgetExtractor.findToolItem("caseSensitiveSearch");
wholeWord= widgetExtractor.findToolItem("wholeWordSearch");
regEx= widgetExtractor.findToolItem("regExSearch");
inSelection= widgetExtractor.findToolItem("searchInSelection");
searchForward= widgetExtractor.findToolItem("searchForward");
searchBackward= widgetExtractor.findToolItem("searchBackward");
openReplaceDialog= widgetExtractor.findButton("replaceToggle");
extractReplaceWidgets();
}

private void extractReplaceWidgets() {
if (!isReplaceDialogOpen() && Objects.nonNull(openReplaceDialog)) {
WidgetExtractor widgetExtractor= new WidgetExtractor(FindReplaceOverlay.ID_DATA_KEY, shell);
replace= widgetExtractor.findHistoryTextWrapper("replaceInput");
replaceButton= widgetExtractor.findToolItem("replaceOne");
replaceAllButton= widgetExtractor.findToolItem("replaceAll");
}
}

private void restoreInitialConfiguration() {
Expand All @@ -100,12 +103,12 @@ private void restoreInitialConfiguration() {
public void closeAndRestore() {
restoreInitialConfiguration();
assertInitialConfiguration();
closeOperation.run();
overlay.close();
}

@Override
public void close() {
closeOperation.run();
overlay.close();
}

@Override
Expand Down Expand Up @@ -234,15 +237,13 @@ public void performReplace() {
}

public boolean isReplaceDialogOpen() {
return dialogAccessor.getBoolean("replaceBarOpen");
return replace != null;
}

public void openReplaceDialog() {
if (!isReplaceDialogOpen() && Objects.nonNull(openReplaceDialog)) {
openReplaceDialog.notifyListeners(SWT.Selection, null);
replace= (HistoryTextWrapper) dialogAccessor.get("replaceBar");
replaceButton= (ToolItem) dialogAccessor.get("replaceButton");
replaceAllButton= (ToolItem) dialogAccessor.get("replaceAllButton");
extractReplaceWidgets();
}
}

Expand Down Expand Up @@ -309,15 +310,14 @@ public void assertEnabled(SearchOptions option) {

@Override
public boolean isShown() {
return shellRetriever.get() != null && shellRetriever.get().isVisible();
return !shell.isDisposed() && shell.isVisible();
}

@Override
public boolean hasFocus() {
Shell overlayShell= shellRetriever.get();
Control focusControl= overlayShell.getDisplay().getFocusControl();
Control focusControl= shell.getDisplay().getFocusControl();
Shell focusControlShell= focusControl != null ? focusControl.getShell() : null;
return focusControlShell == overlayShell;
return focusControlShell == shell;
}

}
Loading
Loading