Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 354353

Summary:

[UI] "Select Element" actions

Product:

[Modeling] TMF

Reporter:

Sven Efftinge <sven.efftinge>

Component:

Xtext

Assignee:

Project Inbox <tmf.xtext-inbox>

Status:

CLOSED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

clay, lieven.lemiengre, sebastian.zarnekow

Version:

2.0.0

Flags:

sven.efftinge: indigo+

Target Milestone:

SR2

Hardware:

OS:

Mac OS X - Carbon (unsup.)

Whiteboard:

Attachments:

Description	Flags
proposed patch pls review	none
proposed patch pls review	none
revised patch	none
revised patch	none
revised patch to be discussed	sven.efftinge: iplog+

Description Sven Efftinge

2011-08-10 06:19:05 EDT

In JDT is you press CTRL+SHIFT and one of the cursor keys, you are able to make textual selections based on the underlying AST.

As the needed information is there in every Xtext language as well, we can provide this feature by default.

The semantics seem to be:
CTRL+SHIFT+LEFT - select left sibling as well
CTRL+SHIFT+RIGHT - select right sibling as well
CTRL+SHIFT+UP - select parent sibling
CTRL+SHIFT+DOWN - restore previous selection

Comment 1 Michael Clay

2011-08-10 06:57:06 EDT

*** Bug 305057 has been marked as a duplicate of this bug. ***

Comment 2 Michael Clay

2011-08-27 15:03:43 EDT

Created attachment 202271 [details]
proposed patch pls review

Comment 3 Michael Clay

2011-08-27 15:14:51 EDT

Created attachment 202272 [details]
proposed patch pls review

Comment 4 Sebastian Zarnekow

2011-08-27 15:55:14 EDT

Hi Michael,

thanks for the patch. Would it be possible to remove the INodeSelection and rely on plain IRegions instead? I've got the impression that the INodeSelection is unnecessary and restricts the actions to the boundaries nodes. For example, it's not possible to do something like this (where | indicates the cursor position and [] the selection):

"my CamelC|aseValue"
"my Camel[Case]Value"
"my [CamelCaseValue]“
"[my CamelCaseValue]“
["my String value"]

Another thing that looks suspicious to me is the INodeSelectionProvider + Factory + Acceptor. The SelectionActionContributor should simply use a number of injected providers, e.g. Provider<MyNodeSelectionProvider> firstProvider, Provider<MyOtherNodeSelectionProvider> secondProvider. Furthermore it doesn't hurt if the INodeSelectionProvider has the method #initialize(XtextEditor) on it's interface.

One thing really looked like a bug to me: The read-only transaction returned a composite node and thereby left the save context. Instead, all the computation should happen inside the transaction and the newly selected region should be returned.

Could you please update the patch accordingly and please don't hesitate to ask if something's unclear.

Comment 5 Michael Clay

2011-08-28 04:34:33 EDT

Created attachment 202276 [details]
revised patch

revised patch - incorporated sebastians improvements

Comment 6 Sebastian Zarnekow

2011-08-28 05:40:10 EDT

Hi Michael,

thanks for the very fast response. One last minor thing that I wonder about is NodeSelectionUtils. It contains a bunch of static methods that cannot be overridden / specialized in a straight forward way. Since the only clients for these methods seem to be the implementations of INodeSelectionProvider, I wonder if it would hurt to move the utitlity methods into the AbstractNodeSelectionProvider and mark them as protected / non-static?

Comment 7 Michael Clay

2011-08-28 06:24:29 EDT

Created attachment 202277 [details]
revised patch

refactored and inlined NodeSelectionUtils into ANSP

Comment 8 Sebastian Zarnekow

2011-08-28 06:45:09 EDT

Hi Michael,

the patch looks good to me. Please feel free to push it.

Could you take some time and write a few unit tests for the selection providers?

Comment 9 Michael Clay

2011-08-28 16:44:39 EDT

pushed to master

Comment 10 Sebastian Zarnekow

2011-08-29 05:05:15 EDT

Hi Michael,

after a brief offline discussion with Sven, we found something that we'd like to change in the default implementation. Instead of using nodes, the actual EObjects (obtained by the EObjectAtOffsetHelper) should be used to determine the left/right/parent selection in the default implementation.

Please use the significant region provided ILocationInFileProvider for the first selection level (if the current selection is part of the significant region) and the full region for the following selections. This will improve the navigation experience when actions are involved. 

Do you have the time to update the implementation (and provide some tests)?

Comment 11 Michael Clay

2011-09-04 17:05:39 EDT

Created attachment 202724 [details]
revised patch to be discussed

Comment 12 Sven Efftinge

2011-09-05 02:57:07 EDT

Looks good on a first glance. Please push, we can work out the details in the repository.

Comment 13 Michael Clay

2011-09-05 03:47:16 EDT

pushed. 

(In reply to comment #12)
> Looks good on a first glance. Please push, we can work out the details in the
> repository.

Comment 14 Sven Efftinge

2011-09-20 05:05:11 EDT

Thank you, Michael.

Comment 15 Sven Efftinge

2011-09-20 05:26:47 EDT

I broke the undo feature

Comment 16 Sven Efftinge

2011-09-20 06:26:21 EDT

pushed fix

Comment 17 Karsten Thoms

2017-09-19 17:06:25 EDT

Closing all bugs that were set to RESOLVED before Neon.0

Comment 18 Karsten Thoms

2017-09-19 17:18:17 EDT

Closing all bugs that were set to RESOLVED before Neon.0