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

Enable MAX_SCREEN_LINE_LENGTH to now be set via a config option #14495

Merged
merged 4 commits into from Sep 12, 2017

Conversation

Projects
None yet
7 participants
@warrenpnz
Contributor

warrenpnz commented May 19, 2017

Description of the Change

This change enables the maximum softWrapColumn in the display layer object to be set using a configuration parameter rather than using the MAX_SCREEN_LINE_LENGTH constant. This value is returned by the getSoftWrapColumn function.

The default and minimum is still set at 500 to satisfy the design decision to address performance issues for those opening minified files, however it can be adjusted upwards for those who require a more flexible option.

Alternate Designs

As the getSoftWrapColumn function used a constant as the default return value, there are no alternate design options.

Why Should This Be In Core?

This needs to be in core as it is affecting the editor display layer parameters directly

Benefits

Benefits are that the number of people who work with long line files that are not necessarily minified are able to customise their maximum width by the settings editor instead of using the constant value

Possible Drawbacks

Possible drawbacks are that a user sets the width high enough to cause performance issues with minified files, however this can be mitigated by reverting the setting to defaults in the editor settings

Applicable Issues

Related to: #13820

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu
Member

50Wliu commented May 23, 2017

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld May 23, 2017

Contributor

👍 Thanks for doing this; this is a good feature. We will definitely merge it if you add a test for it.

I think there should be two simple tests:

  • one in text-editor-spec.coffee that calls TextEditor.update with the maxScreenLineLength parameter.
  • one in text-editor-registry-spec that checks that the config setting works correctly.
Contributor

maxbrunsfeld commented May 23, 2017

👍 Thanks for doing this; this is a good feature. We will definitely merge it if you add a test for it.

I think there should be two simple tests:

  • one in text-editor-spec.coffee that calls TextEditor.update with the maxScreenLineLength parameter.
  • one in text-editor-registry-spec that checks that the config setting works correctly.
@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm May 23, 2017

Member

Not something that needs to be addressed in this PR ... but Safe Mode should probably force this configuration value to the default.

Member

lee-dohm commented May 23, 2017

Not something that needs to be addressed in this PR ... but Safe Mode should probably force this configuration value to the default.

@warrenpnz

This comment has been minimized.

Show comment
Hide comment
@warrenpnz

warrenpnz May 28, 2017

Contributor

So I'm having a bit of trouble running local spec tests, but these should work.
How do others do local testing without pushing to github? (OSX or CentOS)

Contributor

warrenpnz commented May 28, 2017

So I'm having a bit of trouble running local spec tests, but these should work.
How do others do local testing without pushing to github? (OSX or CentOS)

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu May 28, 2017

Member

atom --test spec. You can focus specs by adding an f in front of them, eg fit or fdescribe. Just don't forget to unfocus them before committing.

Member

50Wliu commented May 28, 2017

atom --test spec. You can focus specs by adding an f in front of them, eg fit or fdescribe. Just don't forget to unfocus them before committing.

@warrenpnz

This comment has been minimized.

Show comment
Hide comment
@warrenpnz

warrenpnz May 29, 2017

Contributor

Finally got local spec testing to work and now tests ok.

Contributor

warrenpnz commented May 29, 2017

Finally got local spec testing to work and now tests ok.

@maxbrunsfeld

I left two small changes. Other than that, this looks great. Thanks!

@@ -285,6 +285,10 @@ class TextEditor extends Model
@preferredLineLength = value
displayLayerParams.softWrapColumn = @getSoftWrapColumn()
when 'maxScreenLineLength'
if value isnt @maxScreenLineLength
@maxScreenLineLength = value

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 30, 2017

Contributor

Since the maxScreenLineLength field affects value returned by getSoftWrapColumn, I think we should include the following statement here:

displayLayerParams.softWrapColumn = @getSoftWrapColumn()

This way, if you just update the maxScreenLineLength field, we'll still recompute the text layout in the display layer.

@maxbrunsfeld

maxbrunsfeld May 30, 2017

Contributor

Since the maxScreenLineLength field affects value returned by getSoftWrapColumn, I think we should include the following statement here:

displayLayerParams.softWrapColumn = @getSoftWrapColumn()

This way, if you just update the maxScreenLineLength field, we'll still recompute the text layout in the display layer.

This comment has been minimized.

@warrenpnz

warrenpnz May 30, 2017

Contributor

Fixed in latest commit. Thanks for the suggestions.

@warrenpnz

warrenpnz May 30, 2017

Contributor

Fixed in latest commit. Thanks for the suggestions.

Show outdated Hide outdated spec/text-editor-registry-spec.js
@battaglr

This comment has been minimized.

Show comment
Hide comment
@battaglr

battaglr Sep 7, 2017

Is this still being considered?

battaglr commented Sep 7, 2017

Is this still being considered?

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Sep 11, 2017

Contributor

@warrenpnz if you can get the builds green and resolve conflicts we'll get this merged.

Contributor

iolsen commented Sep 11, 2017

@warrenpnz if you can get the builds green and resolve conflicts we'll get this merged.

@wpowell-oss

This comment has been minimized.

Show comment
Hide comment
@wpowell-oss

wpowell-oss Sep 11, 2017

Contributor

Not sure what thee errors are as one looks like AppVeyor failed to install a module during the build that was not included in the build, and the CircleCI one is an internal eror for code signing. Any help on figuring these out would be appreiated as I can't see how they relate to the code updates

Contributor

wpowell-oss commented Sep 11, 2017

Not sure what thee errors are as one looks like AppVeyor failed to install a module during the build that was not included in the build, and the CircleCI one is an internal eror for code signing. Any help on figuring these out would be appreiated as I can't see how they relate to the code updates

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 11, 2017

Member

A rebase is needed so that Appveyor can rebuild the PR.

Member

50Wliu commented Sep 11, 2017

A rebase is needed so that Appveyor can rebuild the PR.

@wpowell-oss

This comment has been minimized.

Show comment
Hide comment
@wpowell-oss

wpowell-oss Sep 12, 2017

Contributor

Rebase complete.

Contributor

wpowell-oss commented Sep 12, 2017

Rebase complete.

@iolsen iolsen merged commit 7f7bdbf into atom:master Sep 12, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@battaglr

This comment has been minimized.

Show comment
Hide comment
@battaglr

battaglr Sep 12, 2017

Awesome! 👏 👏 👏 👏

battaglr commented Sep 12, 2017

Awesome! 👏 👏 👏 👏

@warrenpnz

This comment has been minimized.

Show comment
Hide comment
@warrenpnz

warrenpnz Sep 12, 2017

Contributor

Thank you all for the help in getting this one through.

Contributor

warrenpnz commented Sep 12, 2017

Thank you all for the help in getting this one through.

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