Typing open bracket should surround the selected text with brackets #2051

lathapatil · 2024-07-08T06:45:55Z

Surround the selected text with brackets in all editors when an opening bracket is inserted. Brackets included are (,<,[,{,",',`

Fixes #865
#1141

Linked to #1718

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

laeubi · 2024-07-08T06:50:05Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

+					command.shiftsCaret= false;
+
+					// Run this in a UI thread to ensure the selection is updated correctly
+					sourceViewer.getTextWidget().getDisplay().asyncExec(new Runnable() {


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

What's wrong with the current approach?

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.

@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.

laeubi · 2024-07-08T06:50:57Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

+					});
+				}
+			} catch (BadLocationException e) {
+				SWT.error(SWT.ERROR_INVALID_ARGUMENT);


Should we really raise an SWT error here?

@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 ?

github-actions · 2024-07-08T07:40:00Z

Test Results

1 818 files + 483 1 818 suites +483 1h 53m 11s ⏱️ + 50m 49s
7 712 tests + 1 7 483 ✅ ± 0 228 💤 ± 0 1 ❌ +1
24 297 runs +4 430 23 549 ✅ +4 197 747 💤 +232 1 ❌ +1

For more details on these failures, see this check.

Results for commit 284107e. ± Comparison against base commit 54479d7.

♻️ This comment has been updated with latest results.

lathapatil · 2024-09-18T05:41:51Z

@mickaelistria @laeubi is there an issue with this approach or this issue fix is not required now?

mickaelistria · 2024-09-18T07:42:46Z

I have some concern that changing selection that way is likely to conflict with some other auto editor strategies that would run after this one for the same event and alter the command changing the size of the inserted text, or that would implement a similar strategy to also change the selection. But both are minor and I think the best way to figure it out is to merge.
Another thing I noticed, which is not related and is deeper in the editor, is that this very convenient feature wouldn't work for multiple or block text selection. But this seems out of the scope for this change.

So IMO, please just rebase to get the MANIFEST.MF compatible with current master branch and we can merge.

akurtakov · 2024-10-21T08:52:42Z

Content of this PR is plain wrong as it contains many other unrelated changes.

lathapatil · 2024-10-21T08:54:51Z

Content of this PR is plain wrong as it contains many other unrelated changes.

Its my mistake. I am checking on it.

Review points incorporated

lathapatil · 2024-10-21T09:48:25Z

this very convenient feature wouldn't work for multiple or block text selection

@mickaelistria It works in generic editor for all cases but works in Java editor only if text blocks surrounds with { and `
Is it expected or is further analysis required to fix this in java editor as well?

2024-10-21_15h12_54.mp4

mickaelistria · 2024-11-19T08:46:18Z

The Java editor has some extra configuration here and there to tweak the edition experience.
I don't mind merging it if it works for the generic editor and most text editors. The JDT part will probably have to be investigated in JDT directly anyway, without need to fix anything here.
I'm sorry we missed to merge this change earlier, since we're in RC phase, it's most likely better to wait until next stream starts (so we have time to get feedback and improve it if necessary).

mickaelistria · 2024-12-06T15:33:41Z

Now is a good time to merge. Thank you!
Please add a note to the noteworthy documentation about this new feature.

HannesWell · 2024-12-06T17:53:53Z

Thank you for this PR!
@mickaelistria the next time please make sure that such long standing PRs are rebased so that the version-bump bot can do it's duty. This is now missing, but I have created #2598, to apply it.

mickaelistria · 2024-12-06T19:33:50Z

Oops, sorry and thank you for addressing it!

szarnekow · 2024-12-08T12:10:23Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

+ * @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 {


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

@szarnekow please refer video in this comment.

Are you also asking for the same thing?

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).

szarnekow · 2024-12-08T12:12:52Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

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


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()?

A null check will be added.

szarnekow · 2024-12-08T12:14:31Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java

+	public void customizeDocumentCommand(IDocument document, DocumentCommand command) {
+		if (bracketsMap.containsKey(command.text)) {
+			try {
+				ITextSelection selection= (ITextSelection) sourceViewer.getSelectionProvider().getSelection();


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

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

szarnekow · 2024-12-08T12:20:36Z

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

-			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));


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?

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

szarnekow · 2024-12-08T12:22:44Z

@mickaelistria you suggest that JDT can tweak this, but the way it is configured doesn't allow subclasses to override the behavior. What did you have in mind when you gave green light to merge?

jukzi · 2024-12-09T07:33:44Z

please fix eclipse-jdt/eclipse.jdt.ui#1843

iloveeclipse · 2024-12-09T07:40:05Z

please fix eclipse-jdt/eclipse.jdt.ui#1843

Please also consider comments from @szarnekow and do not fix JDT UI, but improve platform code - the change here shouldn't break clients!

iloveeclipse · 2024-12-09T08:15:00Z

Just discovered that this change also broke source hover functionality in Java editor (Shift + Hover over method to see implementation) => #2600

lathapatil · 2024-12-09T08:49:57Z

I will check the root cause and fix it ASAP . Thanks for reviewing .

Fixes eclipse-platform#2600

Fixes eclipse-platform#2600 Review points implementation

Fixes #2600 Review points implementation

laeubi reviewed Jul 8, 2024

View reviewed changes

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/SurroundWithBracketsStrategy.java Outdated Show resolved Hide resolved

laeubi reviewed Jul 8, 2024

View reviewed changes

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java Outdated Show resolved Hide resolved

laeubi reviewed Jul 8, 2024

View reviewed changes

lathapatil force-pushed the Enhancements/865_SurroundTextByBrackets branch from d89042b to 6698b75 Compare July 11, 2024 08:31

lathapatil requested review from laeubi and mickaelistria July 11, 2024 08:33

lathapatil force-pushed the Enhancements/865_SurroundTextByBrackets branch from c555d19 to 6698b75 Compare October 21, 2024 09:03

Typing open bracket should surround the selected text with brackets

284107e

Review points incorporated

lathapatil force-pushed the Enhancements/865_SurroundTextByBrackets branch from 6698b75 to 284107e Compare October 21, 2024 09:07

mickaelistria added this to the 4.35 M1 milestone Nov 19, 2024

MohananRahul removed this from the 4.35 M1 milestone Nov 22, 2024

mickaelistria added this to the 4.35 M1 milestone Dec 6, 2024

mickaelistria added the noteworthy Noteworthy feature label Dec 6, 2024

mickaelistria merged commit 7adb4ef into eclipse-platform:master Dec 6, 2024
9 of 14 checks passed

ptziegler mentioned this pull request Dec 6, 2024

Restore ability to filter views by category #2579

Merged

HannesWell mentioned this pull request Dec 6, 2024

Apply missing version bump #2598

Merged

szarnekow reviewed Dec 8, 2024

View reviewed changes

jukzi mentioned this pull request Dec 9, 2024

SmokeViewsTest.testOpenSourceView fails since I20241206-1800 eclipse-jdt/eclipse.jdt.ui#1843

Closed

iloveeclipse mentioned this pull request Dec 9, 2024

Show source on hover & Declaration view broken #2600

Closed

mickaelistria mentioned this pull request Dec 9, 2024

[Feature Request] Surround a selection with brackets and quotes in all kind of text editors #1141

Closed

lathapatil added a commit to lathapatil/eclipse.platform.ui that referenced this pull request Dec 10, 2024

Fix Issues introduced by PR eclipse-platform#2051

2e548fc

Fixes eclipse-platform#2600

lathapatil added a commit to lathapatil/eclipse.platform.ui that referenced this pull request Dec 12, 2024

Fix Issues introduced by PR eclipse-platform#2051

272e7f8

Fixes eclipse-platform#2600 Review points implementation

lathapatil added a commit to lathapatil/eclipse.platform.ui that referenced this pull request Dec 12, 2024

Fix Issues introduced by PR eclipse-platform#2051

7a453c6

Fixes eclipse-platform#2600 Review points implementation

merks pushed a commit that referenced this pull request Dec 13, 2024

Fix Issues introduced by PR #2051

b7d5b14

Fixes #2600 Review points implementation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing open bracket should surround the selected text with brackets #2051

Typing open bracket should surround the selected text with brackets #2051

lathapatil commented Jul 8, 2024 •

edited

Loading

laeubi Jul 8, 2024

mickaelistria Jul 8, 2024

laeubi Jul 8, 2024

lathapatil Jul 9, 2024

laeubi Jul 8, 2024

lathapatil Jul 9, 2024

github-actions bot commented Jul 8, 2024 •

edited

Loading

lathapatil commented Sep 18, 2024

mickaelistria commented Sep 18, 2024

akurtakov commented Oct 21, 2024

lathapatil commented Oct 21, 2024

lathapatil commented Oct 21, 2024 •

edited

Loading

mickaelistria commented Nov 19, 2024

mickaelistria commented Dec 6, 2024

HannesWell commented Dec 6, 2024

mickaelistria commented Dec 6, 2024

szarnekow Dec 8, 2024

lathapatil Dec 10, 2024

mickaelistria Dec 10, 2024

szarnekow Dec 8, 2024

lathapatil Dec 10, 2024

szarnekow Dec 8, 2024

lathapatil Dec 10, 2024

szarnekow Dec 8, 2024

mickaelistria Dec 8, 2024

szarnekow commented Dec 8, 2024

jukzi commented Dec 9, 2024

iloveeclipse commented Dec 9, 2024

iloveeclipse commented Dec 9, 2024

lathapatil commented Dec 9, 2024

Typing open bracket should surround the selected text with brackets #2051

Typing open bracket should surround the selected text with brackets #2051

Conversation

lathapatil commented Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 8, 2024 • edited Loading

Test Results

lathapatil commented Sep 18, 2024

mickaelistria commented Sep 18, 2024

akurtakov commented Oct 21, 2024

lathapatil commented Oct 21, 2024

lathapatil commented Oct 21, 2024 • edited Loading

mickaelistria commented Nov 19, 2024

mickaelistria commented Dec 6, 2024

HannesWell commented Dec 6, 2024

mickaelistria commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szarnekow commented Dec 8, 2024

jukzi commented Dec 9, 2024

iloveeclipse commented Dec 9, 2024

iloveeclipse commented Dec 9, 2024

lathapatil commented Dec 9, 2024

lathapatil commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

lathapatil commented Oct 21, 2024 •

edited

Loading