Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Fix includeNonWordCharacters regression in Cursor #16544

Merged
merged 1 commit into from
Jan 29, 2018

Conversation

rosston
Copy link
Contributor

@rosston rosston commented Jan 12, 2018

Description of the Change

43aa3c7 (#15776) broke includeNonWordCharacters functionality of Cursor#getBeginningOfCurrentWordBufferPosition and Cursor#getEndOfCurrentWordBufferPosition. I fixed that regression.

NOTE: There are no tests here. There are no tests of Cursor that I can see. I would be happy to add some tests to cover the includeNonWordCharacters functionality, but I could use some guidance (or an example?) on how to use/clean up after using atom.workspace, etc.

Alternate Designs

Alternate designs would require needless refactoring. I just made the call to #wordRegExp look like it did before (i.e., it should be passed options).

Why Should This Be In Core?

It was broken in core.

Benefits

The regression of includeNonWordCharacters functionality will be resolved.

Possible Drawbacks

I don't see any possible drawbacks of the change.

Verification Process

I used sublime-word-navigation to verify that the bug was fixed. That package uses includeNonWordCharacters for both beginning and end of word movement. With that package installed, I:

  • typed foo-bar-baz-qux into a buffer
  • set the cursor at the end of the buffer: foo-bar-baz-qux|
  • twice hit alt+left arrow (on macOS, adjust your word-by-word navigation shortcut by platform)

Without this fix in place, the cursor is after baz: foo-bar-baz|-qux
With this fix in place (and in Atom < 1.23.0), the cursor is in front of baz: foo-bar-|baz-qux

I did not verify that this change doesn't introduce any regressions. I am reverting a call back to what it was previously (passing in options), so I'm not very worried about regressions.

Applicable Issues

None

@50Wliu
Copy link
Contributor

50Wliu commented Jan 12, 2018

/cc @maxbrunsfeld

@rosston
Copy link
Contributor Author

rosston commented Jan 29, 2018

Sorry, I know this probably isn't very high priority, but any update on this? I'm happy to make whatever changes need to be made to get this closer to merge.

@50Wliu
Copy link
Contributor

50Wliu commented Jan 29, 2018

I'll try to get someone more familiar to take a look.

@maxbrunsfeld maxbrunsfeld merged commit 669b22c into atom:master Jan 29, 2018
@maxbrunsfeld
Copy link
Contributor

Thanks @rosston! Sorry for the breakage.

@rosston rosston deleted the fix-cursor-movement branch January 29, 2018 18:27
@rosston
Copy link
Contributor Author

rosston commented Jan 29, 2018

No problem! Stuff happens in software. 🙂 Thanks for taking a look!

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

Successfully merging this pull request may close these issues.

4 participants