Skip to content
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

Implemented sub-word cursor movement #2665

Merged
merged 3 commits into from
May 22, 2024
Merged

Conversation

masmu
Copy link
Contributor

@masmu masmu commented Dec 18, 2022

Hello zyedidia,

I implemented the following bindable actions and would be happy about a merge:

  • SubWordRight
  • SubWordLeft
  • SelectSubWordRight
  • SelectSubWordLeft
  • DeleteSubWordRight
  • DeleteSubWordLeft

About the implementation:

  • I payed attention to not change any pre-existing application logic. So for instance clicking on a variable still selects the entire 'word', as you would expect.
  • No new action have been bound to any default keybindings
  • The algorithm works like vscode or sublime do sub-words. I cannot not tell any difference in cursor behavior.
  • It was mostly but extensively tested with python and go source code
  • The approach is based on function-based set theory rather than regular expressions. Therefore I had to add quite some new internal/utils/utils.go functions, but it should be fast.
  • Documentation has been updated

This branch resolves #1197

Keep up the amazing work!
Cheers

@masmu
Copy link
Contributor Author

masmu commented Apr 1, 2023

Is there any news on this one?

@zyedidia
Copy link
Owner

zyedidia commented Apr 2, 2023

Cool, looks pretty good. Could you give a bit more detail about how subwords are determined? Is it via camel case, or symbols like numbers or _?

I think the code needs be formatted by gofmt -s.

@masmu
Copy link
Contributor Author

masmu commented Apr 3, 2023

Good catch. I auto-formatted the modified files using gofmt. Actually just internal/buffer/cursor.go needed some polishing.

According the determination of sub-words. I could not find a single spec of that. In general you can say it is a combination of camel case, snake case and pascal case, which I guess makes sense so sub-wording works for all languages.
Due to the missing spec, there was no other option then replicating the exact behavior of vscode and sublime with excessive testing.
Using this for months now, mainly with python and go, never noticed a single bad jump.

@masmu
Copy link
Contributor Author

masmu commented Jul 2, 2023

Any chance this is gonna make it into the next release? I would love to see this feature.

@zyedidia
Copy link
Owner

zyedidia commented Jul 8, 2023

I am reviewing PRs for the next release today and tomorrow -- I plan to merge this soon and include it in the next release.

@masmu
Copy link
Contributor Author

masmu commented Feb 18, 2024

Its been a while and I kindly want to ask if there is any chance of getting this merged?

@masmu
Copy link
Contributor Author

masmu commented Apr 14, 2024

@JoeKar , @dmaluka maybe here?

internal/buffer/cursor.go Show resolved Hide resolved
internal/buffer/cursor.go Outdated Show resolved Hide resolved
internal/action/actions.go Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
@dmaluka
Copy link
Collaborator

dmaluka commented May 16, 2024

Looks good to me. @JoeKar what do you think?

Copy link
Collaborator

@JoeKar JoeKar left a comment

Choose a reason for hiding this comment

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

Definitely 👍 for proposing this feature and thanks to you guys for taking care of creation and review.
I've just these two small remarks.

Currently I've unfortunately very limited time and wasn't able to test it yet, but was already in the situation to miss something like that. :)

internal/util/util.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@JoeKar JoeKar left a comment

Choose a reason for hiding this comment

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

I tried...

  • SubWordRight
  • SubWordLeft
  • SelectSubWordRight
  • SelectSubWordLeft
  • DeleteSubWordLeft

...and it matched the expectation. Furthermore I can't find a blocking point.
So, why not? :)

@dmaluka:
Shall we already merge it or schedule it after the next release?

@dmaluka
Copy link
Collaborator

dmaluka commented May 21, 2024

I see no reason why not merge it now.

@JoeKar JoeKar merged commit 35630aa into zyedidia:master May 22, 2024
3 checks passed
@masmu
Copy link
Contributor Author

masmu commented May 23, 2024

Thanks again guys, for the review and your support.

@Neko-Box-Coder
Copy link
Contributor

Neko-Box-Coder commented May 26, 2024

@masmu
Hey, I am using this already and it is great. Thanks a lot for the feature.

One thing I have noticed is that it is treating continuous space as a subword which is actually what I want for the word version as well.

However, it seems like the subword doesn't work quite how I would expected it to be on lines with trailing spaces.
Going from the beginning of the trailing spaces landed me on the next line.
I would have expected it to stop right before the next line instead of going onto the next line.

Doing the reverse doesn't behave the same way, going from the end of the trailing spaces landed me at the beginning of the line with trailing spaces.

Is this an expected behavior?

@masmu
Copy link
Contributor Author

masmu commented May 28, 2024

Hey, thanks. And good catch!

Is this an expected behavior?

Nope.

However, it seems like the subword doesn't work quite how I would expected it to be on lines with trailing spaces.
Going from the beginning of the trailing spaces landed me on the next line.
I would have expected it to stop right before the next line instead of going onto the next line.

The line-break is suppose to act like as an additional jump position, right? I can confirm that this is not the case.
Well, I checked that this issue also was preexisting for WordRight and WordLeft. So this is something that probably needs a manicure anyway. And it is indeed a problem in some cases: When not using line-breaks as additional jumps, you can easily jump over trailing whitespaces into the next line, not even realizing that those trailing whitespaces are there.
You first realize that when you accidentally pressed the end key (⇲) instead.

One thing I have noticed is that it is treating continuous space as a subword which is actually what I want for the word version as well.

Can you please explain this a little bit more?

@Neko-Box-Coder
Copy link
Contributor

Neko-Box-Coder commented May 28, 2024

The line-break is suppose to act like as an additional jump position, right?

Yes

Well, I checked that this issue also was preexisting for WordRight and WordLeft.

Yeah, I am not a huge fan of how WordRight and WordLeft works at the moment where it will go to nextline/previous line.

One thing I have noticed is that it is treating continuous space as a subword which is actually what I want for the word version as well.

Can you please explain this a little bit more?

So basically in normal WordRight (or left), it will merge multiple spaces and a word together if multiple spaces come first. So if you have

   some     words here
^

Doing a WordRight here will go to the end of some instead of the beginning of some. So ends up like this

   some     words here
      ^

However, if you have a word before multiple spaces, it stops at the multiple spaces, which is what I want but different from the behavior I said previously where it is treating spaces and a word together. So if you have

    some    words here
    ^

and do WordRight, it will land at the spaces like this

    some    words here
        ^

The SubWordRight (and left) do not have this problem (apart from the newline mentioned previously)
If I have the same thing as beginning and do SubWordRight 3 times, I would get

    some     words here
^   ^   ^    ^
1   2   3    4

which is exactly what I want from WordRight and WordLeft but they don't. Plus I use a lot of camel and pascal case so this kills two birds with one stone for me. Thanks :)

@dmaluka
Copy link
Collaborator

dmaluka commented May 28, 2024

When not using line-breaks as additional jumps, you can easily jump over trailing whitespaces into the next line, not even realizing that those trailing whitespaces are there. You first realize that when you accidentally pressed the end key (⇲) instead.

Micro has the hltrailingws option, I highly recommend using it, it helps noticing trailing whitespaces even without any jumping.

@Neko-Box-Coder
Copy link
Contributor

Yes, I have it on already. But it is quite annoying if you want to delete continuous spaces with delete word and it deletes both the spaces and the word,

@masmu
Copy link
Contributor Author

masmu commented May 29, 2024

I guess we should address the trailing spaces issue non the less. It is really confusing especially for standard users.
I can provide a patch.

Do I force push / update this PR? Or create another one?

@dmaluka
Copy link
Collaborator

dmaluka commented May 29, 2024

Do I force push / update this PR? Or create another one?

This one is already merged, so I guess you'd need to create another one anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bindable actions for moving by subwords
5 participants