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

xreadline by word movement #1971

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doremiyeon
Copy link
Contributor

is this useful? added by-word navigation and deletion bindings for xreadline()

  • extend xreadline() with some by-word movement
  • xreadline ctrl-w memmove less

@jarun
Copy link
Owner

jarun commented Dec 15, 2024

Can you explain each of the behavioural changes for me to take a call?

Copy link
Collaborator

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

Untested, but some cursory review.

src/nnn.c Outdated Show resolved Hide resolved
src/nnn.c Outdated Show resolved Hide resolved
src/nnn.c Show resolved Hide resolved
@N-R-K N-R-K force-pushed the xreadline_by_word_movement branch 2 times, most recently from 1a1580a to 1c1211b Compare December 16, 2024 01:20
@N-R-K
Copy link
Collaborator

N-R-K commented Dec 16, 2024

I've pushed some changes:

  1. Removed the superfluous checks. Just make the code gracefully work with 0.
  2. isalnum (and all the other ctype.h functions) only accept argument in the rage [0, 255], we can't feed it wchar_t arguments. Added custom macro for it instead.
  3. We might be dealing with unicode chars, so instead of isalnum I changed the checks to isspace which would work on unicode chars instead of just ascii.
  4. I notice a bug in the current Ctrl+W handling, we only delete a single space if there's multiple before the cursor while readline will remove all. I've fixed it, but it also no longer treats / as a boundary (neither does readline).

@jarun do you want xreadline to follow readline's behavior or do you want to keep the custom / as word boundary behavior? If you want the custom behavior, do you want it just for Ctrl+w or the rest as well?

@N-R-K N-R-K force-pushed the xreadline_by_word_movement branch 2 times, most recently from c667a3a to c945f85 Compare December 16, 2024 01:40
@doremiyeon
Copy link
Contributor Author

Can you explain each of the behavioural changes for me to take a call?

I added Meta-b, Meta-f conforming to https://www.gnu.org/software/bash/manual/bash.html#index-forward_002dword-_0028M_002df_0029 as well as the corresponding deletion commands, M-d and M-bspc.

They move cursor by word boundary, where a word is defined to be composed of letters and digits.

@doremiyeon
Copy link
Contributor Author

@N-R-K

I've pushed some changes:
3. We might be dealing with unicode chars, so instead of isalnum I changed the checks to isspace which would work on unicode chars instead of just ascii.

I hadn't considered unicode, but could isspace be turned into something to include all the other non-word (non alphanum) characters within ascii range as well?

@N-R-K
Copy link
Collaborator

N-R-K commented Dec 16, 2024

but could isspace be turned into something to include all the other non-word (non alphanum) characters within ascii range as well?

What meaningful difference does it make?

@doremiyeon
Copy link
Contributor Author

doremiyeon commented Dec 16, 2024

but could isspace be turned into something to include all the other non-word (non alphanum) characters within ascii range as well?

What meaningful difference does it make?

the word boundaries differ, do they not? many filenames have underscores in them, or dashes. navigating between those is the whole point of these movements. paths contain slashes, same thing.

also: strictly speaking, Ctrl-w explicitly differs from Meta-bspc by what it considers a word boundary. whitespace vs non-alphanumerical (including unicode, not sure if it distinguishes between unicode "special characters" and the rest off the top of my head).

edit: there's also iswalnum() that "should just work."

added M-b, M-f, M-d, M-bspc according to GNU readline specifications.
@N-R-K N-R-K force-pushed the xreadline_by_word_movement branch from c945f85 to d6b1b80 Compare December 16, 2024 08:53
@N-R-K
Copy link
Collaborator

N-R-K commented Dec 16, 2024

the word boundaries differ, do they not? many filenames have underscores in them, or dashes. navigating between those is the whole point of these movements.

Ok, I see. I've switched to iswalnum. Hopefully it doesn't contain any weird pitfalls like the ctype.h counterparts do.

strictly speaking, Ctrl-w explicitly differs from Meta-bspc by what it considers a word boundary.

I've left out ctrl-w change for now, I'll look into it later. But first I'm going to wait for @jarun to reply on whether he wants to keep the custom / behavior.

edit: there's also iswalnum() that "should just work."

Yes, I thought about it just now as well.

@jarun
Copy link
Owner

jarun commented Dec 16, 2024

do you want xreadline to follow readline's behavior or do you want to keep the custom / as word boundary behavior? If you want the custom behavior, do you want it just for Ctrl+w or the rest as well?

We can keep it the same as readline.

when there's multiple spaces, the previous logic didn't erase
them, e.g:

    a word   |   < before
    a word  |    < after Ctrl-w

this patch brings the behavior closer to readline's:

    a word   |   < before
    a |          < after Ctrl-w

this also slightly changes the behavior since '/' is no longer
considered a boundary.
@N-R-K
Copy link
Collaborator

N-R-K commented Dec 16, 2024

We can keep it the same as readline.

Okay, done.


@doremiyeon can you test it out and make sure everything still works as intended?

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

Successfully merging this pull request may close these issues.

3 participants