-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
extend selection should include leading indentation #5231
Comments
This seems like a nice QOL improvement, thank you for sharing your expertise here. We probably want to cover this right inside Ideally we would do something similar for textobjects aswell so The first step would probably some kind of I would like to see this but sadly don't have the bandwith to work on a PR myself right now but I think with these instructions somebody else might be able to pick this up. Maybe you feel up to sending a PR yourself @matklad? |
Maybe you feel up to sending a PR yourself
No promises one way or another, but pointers to code much appreciated,
thanks!
…On Tuesday, 20 December 2022, Pascal Kuthe ***@***.***> wrote:
This seems like a nice QOL improvement, thank you for sharing your
expertise here.
We probably want to cover this right inside select_node_impl in
helix-core/src/objects.rs so it applied to all functions. Treesitter
doesn't keep explicit whitespace nodes around as RA does. the best way to
implement seems to be just look at the RopeSlice between the two child
nodes that surround the selection start and use the same logic as in RA
(although RopeSlice sadly lacks many of the convenience methods str has).
This should happen before select_fn is called.
Ideally we would do something similar for textobjects aswell so maf also
works for initial indentation. That would need to be added to
textobject_treesitter in helix-core/src/textobject.rs. Probably inside
the filter of the QueryCursor.
The first step would probably some kind of extend_ts_node_to_indent
function that would implement logic similar to that in RA (but using a
RopeSlice instead of SyntaxNode).
I would like to see this but sadly don't have the bandwith to work on a PR
myself right now but I think with these instructions somebody else might be
able to pick this up. Maybe you feel up to sending a PR yourself @matklad
<https://github.com/matklad>?
—
Reply to this email directly, view it on GitHub
<#5231 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANB3M6O6YV3NE4VJXAWHITWOIVWHANCNFSM6AAAAAATEU2ERM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I could be biased by the implementation but I think IntelliJ's motion behaviors are confusing (also see #3785). For languages where the indentation is used for scoping this behavior is fine since the indentation is syntactically important, but here the indentation whitespace belongs to the There might be room for an intelligent command like expand_selection that can incorporate this behavior and maybe #3785 but I wouldn't want to see this change to the existing expand_selection command. |
@the-mikedavis Thanks for the pointers. Sorry for jumping the gun here this seemed like a improvement to me as it only applies to start of line indentation which is (at least to me) always part of the text that follows it (especially as it's automatically inserted). I think keeping this behavior behind a flag would be easy to achieve. we could enable that flag based on whether |
I'd obviously defer to @the-mikedavis to do the judgement call here, but my personal level of confidence that IJ behavior is better is pretty high (0.8), for thee following reasons
EDIT: a nice example would be the task of duplicating a function. Today, doing that with |
Playing around with IntelliJ's C-w, I think this would be implementation-wise a bit complicated because the leading/trailing whitespace is selected for statements but not expressions. For example...LOGGER.debug(
"message",
value1, value2
); If you repeatedly hit C-w anywhere on line 2 ( I'm guessing that IntelliJ has language-specific code to do this since it doesn't work on non-Java/Kotlin languages (I tried Rust and Erlang). You could use tree-sitter queries to control which syntax nodes should have their leading/trailing whitespace selected, though this would be a lot of query code for a somewhat small behavior change. So I'm tempted to say that this should be a plugin. For that example you can make any selection linewise with |
I wasn't able to quickly find the relevant impl in the source code, but in my testing the logic seems to be simpler: we don't consider indentation to be part of the element, if there are several elements on the line. In the logger example,
Ah, I didn't realize that
Yeah, though, to my eyes, indentation is a part of a specific object (the same way as, eg, An |
Consider this code and cursor placement:
In helix, extend selection (
alt+o
) would select the following:whereas in IntelliJ the behavior is well, more intelligent :)
It considers leading indentation to be a part of the
match
expression and selects it. That is, selection splits whitespace token in two.I'd argue is that IntelliJ's behavior is the correct one here:
This is the logic implemented by rust-analyzer's selection ranges (source), though VS Code messes that up a bit by adding its own non-syntactic ranges to the hierarchy.
The text was updated successfully, but these errors were encountered: