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

Make soft-wrap break words before a slash or space and after a dash #17949

Merged
merged 1 commit into from Oct 1, 2018

Conversation

Projects
None yet
4 participants
@ariasuni
Contributor

ariasuni commented Aug 29, 2018

Description of the Change

Wrapping now includes wrap on dash and slash. Remake of #17141, which was in response to issue #16904, in which the user wanted more "aggressive" wrapping. To implement this change, the isWrapBoundary variable was given character checks for spaces, slashes, and dashes as wrap boundaries.

Compared to the previous iteration of this PR, it checks if the previousCharacter (not the current character) is a dash, to cut words after it, so that it is consistent with how word-wrapping works in most software.

Any help to fix tests is appreciated since I do not have experience with those.

Alternate Designs

The proposed version was selected because it produced the simplest fix to this problem.

Why Should This Be In Core?

This should be in the core because it has to do with editor functionality.

Benefits

A much more intuitive soft-wrap functionality that doesn't breakup words in the middle if it doesn't have to.

Possible Drawbacks

This is not a new behavior, but it will be more visible:

The cursor can’t be after the last character, because it’s the same position than the beginning of the following line, and so “Move to end of line” puts the cursor before the last character of the line visually.

Verification Process

The new soft-wrap was tested with tests such as these:
screen shot 2018-04-14 at 11 34 11 pm

@ariasuni

This comment has been minimized.

Contributor

ariasuni commented Sep 24, 2018

Hi, it’s been almost a months. Thoughts about this PR?

@rsese

This comment has been minimized.

Member

rsese commented Sep 24, 2018

Sorry about that @ariasuni - even though this is a continuation of #17141, can you copy over/fill out the pull request template?

@ariasuni

This comment has been minimized.

Contributor

ariasuni commented Sep 24, 2018

@rsese done!

@rsese

This comment has been minimized.

Member

rsese commented Sep 25, 2018

Oh sorry my comment wasn't clear, I meant to copy over #17141 (comment) here in this pull request along with mentioning the adjustments you made?

@ariasuni

This comment has been minimized.

Contributor

ariasuni commented Sep 25, 2018

@rsese I updated my comment, is that it?

@rsese

This comment has been minimized.

Member

rsese commented Sep 25, 2018

Yes, thank you 🙇!

Oh also, about this comment:

Any help to fix tests is appreciated since I do not have experience with those.

You mean the CI failure yes?

@ariasuni

This comment has been minimized.

Contributor

ariasuni commented Sep 26, 2018

@rsese yes

@ariasuni

This comment has been minimized.

Contributor

ariasuni commented Sep 26, 2018

I rebased my code, partly to trigger the CI and be sure my change didn’t break anything.

@daviwil

This comment has been minimized.

Member

daviwil commented Sep 26, 2018

Seems that CI failed this time due to a known test flake on Windows, I'm restarting it again.

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 1, 2018

Sorry for the delay! We've been having some flaky CI issues lately so I re-ran this again and everything is green now. Merging this, thanks a lot @ariasuni!

@daviwil daviwil merged commit af54b63 into atom:master Oct 1, 2018

3 checks passed

Atom Pull Requests #11592 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ariasuni ariasuni deleted the ariasuni:more-agressive-word-wrapping branch Oct 1, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 19, 2018

The way this change influences the wrapping of slashes is also unexpected to me. In my opinion, they should behave like spaces and dashes. Here's what TextEdit (the builtin macOS text editor) does:

wrapping

@daviwil daviwil referenced this pull request Oct 19, 2018

Merged

Don't soft-wrap spaces and '/' to new lines #18287

2 of 2 tasks complete

maxbrunsfeld added a commit that referenced this pull request Oct 20, 2018

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