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

Bigword movement doesn't handle non-print non-blank characters very well #7328

Closed
lilyball opened this issue Sep 11, 2020 · 5 comments
Closed

Comments

@lilyball
Copy link
Contributor

Bigword movement uses std::iswblank(c) and std::iswgraph(c) in its implementation, and seems to make the assumption that any character is either blank or graph (or a control character which it doesn't care about). But in the BMP alone there are 6801 codepoints (on my machine running macOS 10.15.6) that are neither blank nor graph or control. Some of these are reserved codepoints, but many are not. For example, Σ (U+03A3 GREEK CAPITAL LETTER SIGMA). Given this, the implementation should probably be using !std::iswblank(c) anywhere it uses std::iswgraph(c).

For that matter, newlines are whitespace but they aren't blank, which means bigword movement stops on every newline, which seems odd. So it should probably be using std::iswspace instead of std::iswblank (the former includes 8 characters the latter doesn't).

lilyball referenced this issue Sep 11, 2020
With a commandline like

```
a b c d
```

and the cursor at the beginning, this would eat "a b", which isn't a
sensible bigword.

Bigword should be "a word, with optional leading whitespace".

This was caused by an overly zealous state-machine that always ate one
char and only *then* started eating leading whitespace.

Instead eat *a character*, and if it was whitespace go on eating
whitespace, and if it was a printable go straight to only eating
printables.

Fixes #7325.
@faho faho closed this as completed in 8cf389b Sep 11, 2020
@faho
Copy link
Member

faho commented Sep 11, 2020

For that matter, newlines are whitespace but they aren't blank, which means bigword movement stops on every newline

Given how important newlines are, I'd call that correct.

@faho faho added this to the fish 3.2.0 milestone Sep 11, 2020
@lilyball
Copy link
Contributor Author

Bigword movement should not stop on newlines. It should stop on words. If I have several blank lines in a row, bigword movement should not step one newline at a time.

@faho
Copy link
Member

faho commented Sep 27, 2020

I don't think it's a problem, but if you think it is, feel free to change it.

@krobelus
Copy link
Member

krobelus commented Sep 27, 2020

Bigword movement should not stop on newlines.

On current master, forward-bigword never stops on newlines. However this is also inconsistent, because it doesn't stop if there is a word at the beginning of a line.

For example, on this commandline, forward-bigword will only stop at indented:

'


at-start-of-line

at-start-of-line

  indented

'

krobelus added a commit that referenced this issue Sep 27, 2020
Improves on #7328.

I believe this is the correct behavior, simply skip all whitespace before
a word. Try with

	./fish -C 'bind \ef forward-bigword; bind \eb backward-bigword; bind \ed kill-bigword; bind \cw backward-kill-bigword'

Also unrelated formatting fixes. I don't think a CI failure on unformatted
code is warranted but I wish it could do that behind the scenes.
@krobelus
Copy link
Member

Fixed that, it seems better now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants