Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*******************************************************************************
* Copyright (c) ETAS GmbH 2024, all rights reserved.
*
* 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:
* ETAS GmbH - initial API and implementation
*******************************************************************************/
package org.eclipse.jface.text;

import java.util.Map;

import org.eclipse.swt.SWT;

import org.eclipse.jface.text.source.ISourceViewer;

/**
* @since 3.26 This strategy supports surrounding the selected text with similar opening and closing
* brackets when the text is selected and an opening bracket is inserted.
*/
public class SurroundWithBracketsStrategy implements IAutoEditStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with block selections, too? E.g., when the user has multiple selections in the current document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szarnekow please refer video in this comment.

Are you also asking for the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's a different thing (which is likely unsupported yet). For example, in a text abababa , try to search for a and click "Select All", then you get a multi-selection. In such case, the expectation would be that if someone hits ( the resulting text becomes (a)b(a)b(a)b(a).


private ISourceViewer sourceViewer;

@SuppressWarnings("nls")
private final Map<String, String> bracketsMap= Map.of("(", ")", "[", "]", "{", "}", "<", ">", "\"", "\"", "'", "'", "`", "`");

public SurroundWithBracketsStrategy(ISourceViewer sourceViewer) {
this.sourceViewer= sourceViewer;
}

@Override
public void customizeDocumentCommand(IDocument document, DocumentCommand command) {
if (bracketsMap.containsKey(command.text)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see quite a bunch of null-checks when command.text is accessed in other context.
Wouldn't this throw an NPE since bracketsMap is null-hostile cause it's created via Map.of()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A null check will be added.

try {
ITextSelection selection= (ITextSelection) sourceViewer.getSelectionProvider().getSelection();
Copy link
Contributor

Choose a reason for hiding this comment

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

DocumentCommand has a package visible field fSelection - is that the same or something else then the selection obtained here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works for both the ITextSelection obtained from the sourceViewer and the fSelection from the DocumentCommand

if (selection != null && selection.getLength() > 0) {
String selectedText= document.get(selection.getOffset(), selection.getLength());
String closingBracket= bracketsMap.get(command.text);
command.text= command.text + selectedText + closingBracket;
command.offset= selection.getOffset();
command.length= selection.getLength();

// Set the caret offset after the opening bracket but before the closing bracket
command.caretOffset= command.offset + command.text.length() - closingBracket.length();
command.shiftsCaret= false;

// Run this in a UI thread asynchronously to ensure the selection is updated correctly
sourceViewer.getTextWidget().getDisplay().asyncExec(new Runnable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Display#execute to ensure it is run in the UI-Thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with the current approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says to "Run this in a UI thread to ensure the selection is updated correctly" but in fact it schedules an async execution in the UI thread, this might be okay / desired but the comment does not reflect that.

In general such "patterns" have several problems for example if you are already in an UI thread, this will possibly execute an arbitrary number of other UI actions that possibly alter your current state. Display#execute on the other hand will execute your command in-place if you are already on an UI thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laeubi
If I do not run this in async execution, the selected text is unselected as soon as the text is surrounded by braces, making the selection unavailable for further operations of surrounding with braces.

Maybe I will update the comment to: 'Run this in a UI thread asynchronously to ensure the selection is updated correctly.

@Override
public void run() {
sourceViewer.setSelectedRange(command.offset + 1, selectedText.length());
}
});
}
} catch (BadLocationException e) {
SWT.error(SWT.ERROR_INVALID_ARGUMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really raise an SWT error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laeubi I referred DefaultDocumentAdapter class raising SWT error in methods getTextRange and replaceTextRange for BadLocationException

I even see catch block is empty in many other cases .
What do you Suggest ?

}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corporation and others.
* Copyright (c) 2000, 2024 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -14,10 +14,12 @@
* Tom Hofmann (Perspectix AG) - bug 297572
* Sergey Prigogin (Google) - bug 441448
* Angelo Zerr <[email protected]> - [CodeMining] Add CodeMining support in SourceViewer - Bug 527515
* Latha Patil (ETAS GmbH) - Issue 865 - Surround the selected text with brackets on inserting a opening bracket
*******************************************************************************/
package org.eclipse.jface.text.source;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Stack;
Expand Down Expand Up @@ -57,6 +59,7 @@
import org.eclipse.jface.text.ITextViewerLifecycle;
import org.eclipse.jface.text.Position;
import org.eclipse.jface.text.Region;
import org.eclipse.jface.text.SurroundWithBracketsStrategy;
import org.eclipse.jface.text.TextViewer;
import org.eclipse.jface.text.codemining.ICodeMiningProvider;
import org.eclipse.jface.text.contentassist.IContentAssistant;
Expand Down Expand Up @@ -543,7 +546,12 @@ public void configure(SourceViewerConfiguration configuration) {
String[] types= configuration.getConfiguredContentTypes(this);
for (String t : types) {

doSetAutoEditStrategies(configuration.getAutoEditStrategies(this, t), t);
IAutoEditStrategy[] autoEditStrategies= configuration.getAutoEditStrategies(this, t);
List<IAutoEditStrategy> autoEditStrategiesList= new ArrayList<>(Arrays.asList(autoEditStrategies));
autoEditStrategiesList.add(new SurroundWithBracketsStrategy(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the strategy here makes it incredibly hard to disable. Why did you opt for this solution instead of returning it from org.eclipse.jface.text.source.SourceViewerConfiguration.getAutoEditStrategies(ISourceViewer, String) as a default?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good suggestion.
@lathapatil can you please consider the suggested change in a next PR ?

IAutoEditStrategy[] newStrategies= autoEditStrategiesList.toArray(new IAutoEditStrategy[0]);

doSetAutoEditStrategies(newStrategies, t);
setTextDoubleClickStrategy(configuration.getDoubleClickStrategy(this, t), t);

int[] stateMasks= configuration.getConfiguredTextHoverStateMasks(this, t);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2014, 2019 Google, Inc and others.
* Copyright (c) 2014, 2024 Google, Inc and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -12,6 +12,7 @@
* Sergey Prigogin (Google) - initial API and implementation
* Mickael Istria (Red Hat Inc.) - [Bug 544708] Ctrl+Home
* Paul Pazderski - [Bug 545530] Test for TextViewer's default IDocumentAdapter implementation.
* Latha Patil (ETAS GmbH) - Issue 865 - Test for Surround the selected text with brackets
*******************************************************************************/
package org.eclipse.jface.text.tests;

Expand Down Expand Up @@ -69,6 +70,7 @@
import org.eclipse.jface.text.hyperlink.URLHyperlink;
import org.eclipse.jface.text.hyperlink.URLHyperlinkDetector;
import org.eclipse.jface.text.source.SourceViewer;
import org.eclipse.jface.text.source.SourceViewerConfiguration;
import org.eclipse.jface.text.tests.util.DisplayHelper;

/**
Expand Down Expand Up @@ -454,4 +456,20 @@ public void testSelectionFromViewerState() {
textSelection= (ITextSelection) textViewer.getSelection();
assertEquals(113, textSelection.getOffset());
}
}

@Test
public void testSurroundwithBracketsStrategy() {
fShell= new Shell();
final SourceViewer sourceViewer= new SourceViewer(fShell, null, SWT.NONE);
sourceViewer.configure(new SourceViewerConfiguration());
sourceViewer.setDocument(new Document("Test sample to surround the selected text with brackets"));
StyledText text= sourceViewer.getTextWidget();
text.setText("Test sample to surround the selected text with brackets");
text.setSelection(15, 23);
assertEquals(23, text.getCaretOffset());
assertEquals("surround", text.getSelectionText());
text.insert("[");
assertEquals("Test sample to [surround] the selected text with brackets", text.getText());
assertEquals(24, text.getCaretOffset());
}
}
Loading