Community
Participate
Working Groups
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
*** Bug 305057 has been marked as a duplicate of this bug. ***
Created attachment 202271 [details] proposed patch pls review
Created attachment 202272 [details] proposed patch pls review
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.
Created attachment 202276 [details] revised patch revised patch - incorporated sebastians improvements
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?
Created attachment 202277 [details] revised patch refactored and inlined NodeSelectionUtils into ANSP
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?
pushed to master
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)?
Created attachment 202724 [details] revised patch to be discussed
Looks good on a first glance. Please push, we can work out the details in the repository.
pushed. (In reply to comment #12) > Looks good on a first glance. Please push, we can work out the details in the > repository.
Thank you, Michael.
I broke the undo feature
pushed fix
Closing all bugs that were set to RESOLVED before Neon.0
