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

Fix for issue 1490 #7029

Merged
merged 2 commits into from Aug 29, 2016

Conversation

@hashhar
Copy link
Contributor

commented May 30, 2016

This is an enhancement as requested in #1490

The commit messages are pretty descriptive and explain why I did what I did.

Things left to do:

  • Test the feature works as advertised and under all possible permutations of settings (The only relevant settings are wrappingColumn, viewportWrapping, smartAutomaticLayout)
  • Smoke Test (from the wiki)
@msftclas

This comment has been minimized.

Copy link

commented May 30, 2016

Hi @hashhar, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

I need someone to go over these:

  • What to do about the localization strings since a call to nls is being made while generating the default config. (Need help)
  • Check to see if a better name can be chosen for the setting. (Need help)
  • Add unit tests.
@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2016

@alexandrudima Just pinging you to ask about how to handle the localization stuff since this is a config key and has associated text to be shown in the config file.

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

@alexandrudima @joaomoreno Could you point me to some unit tests so that I can add some for my feature. And what about the call to nls.localize? How do I handle that?

@alexandrudima

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

@hashhar

Tests for the commonEditorConfig file are in https://github.com/Microsoft/vscode/blob/standalone/0.1.x/src/vs/editor/test/common/config/commonEditorConfig.test.ts#L10

Also, I don't think this option will make configuring straight-forward. Today, solely wrappingColumn controls the column at which to wrap and the special case of wrappingColumn = 0 is taken as a hint to mean viewport wrapping.

IMHO, to support this feature it is clearly insufficient to have a single option controlling both types of behaviours, therefore I suggest to go in the following direction:

  • introduce a new setting, perhaps viewportWrapping or maybe you can suggest a better name that is a boolean. I am not sure what others call this, perhaps wordWrapping is more commonly used.
  • by default the setting should be off
  • if wrappingColumn = 0, then the setting is switched on internally to maintain backwards compatibility.
  • if wrappingColumn > 0 and the setting is on, then the behaviour you introduce takes effect.

Would you think that makes sense? IMHO it will give end-users a more clear picture of what these settings do.

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2016

Currently that's exactly how it works. Unless wrappingColumn > 0 the setting has no visible effect. And I did design it in a way so that the older settings will continue to work like they did.

I'll move to adding tests and rename the setting to something more obvious.

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2016

I couldn't come up with unit tests. Sorry.

Now let me describe the feature:

There can be 6 settings combinations:

wrappingColumn dynamicWrapping Effect New?
0 false Viewport Wrapping. No
-1 false No Wrapping. No
80 false Wraps at 80 columns. No
0 true Viewport Wrapping. No
-1 true No Wrapping. No
80 true If viewport > 80 cols Wrap at 80 columns; else Viewport Wrapping. Yes

So only the last configuration is a new behaviour. I hope this was clear.

hashhar added some commits May 27, 2016

Rudimentary proof of concept for issue 1490
I fixed the viewport wrapping to on even if the user chose a wrapping
column > 0. I then assign the wrappingColumn value based on the minimum
of 80 or the current viewport width.

Next Steps:
Change the behaviour to respect the user settings. Add a new
configuration option to configure the behaviour. Will need to dicuss
this with the core devs.
Converted the setting into an editor option named dynamicWrapping.
I converted the setting I introduced into a separate config option so
that users can configure it independently of other options. The setting
works in a backwards compatible way so that keeping it off doesn't
change the user experience than what the user is experienced to. Only
when the setting is turned on will it have an effect on the experience.
@coveralls

This comment has been minimized.

Copy link

commented Aug 26, 2016

Coverage Status

Changes Unknown when pulling 7aa98e6 on hashhar:issue-1490 into * on Microsoft:master*.

@coveralls

This comment has been minimized.

Copy link

commented Aug 26, 2016

Coverage Status

Coverage increased (+0.005%) to 60.978% when pulling 7aa98e6 on hashhar:issue-1490 into 7803f7e on Microsoft:master.

@coveralls

This comment has been minimized.

Copy link

commented Aug 26, 2016

Coverage Status

Coverage decreased (-0.005%) to 60.968% when pulling 7aa98e6 on hashhar:issue-1490 into 7803f7e on Microsoft:master.

@coveralls

This comment has been minimized.

Copy link

commented Aug 26, 2016

Coverage Status

Coverage decreased (-0.005%) to 60.968% when pulling 7aa98e6 on hashhar:issue-1490 into 7803f7e on Microsoft:master.

@alexandrudima alexandrudima added this to the August 2016 milestone Aug 26, 2016

@alexandrudima alexandrudima merged commit 786790a into microsoft:master Aug 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandrudima

This comment has been minimized.

Copy link
Member

commented Aug 29, 2016

Thank you for your contribution! ❤️

@hashhar hashhar deleted the hashhar:issue-1490 branch Aug 29, 2016

@hashhar hashhar restored the hashhar:issue-1490 branch Aug 29, 2016

@hashhar hashhar deleted the hashhar:issue-1490 branch Aug 30, 2016

@hashhar hashhar restored the hashhar:issue-1490 branch Aug 30, 2016

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

@alexandrudima It seems some newer commits have rewritten the changes introduced by this PR. I have pushed after rebasing against the latest master right now.

EDIT: My bad. It seems that the name of the setting was changed.

@alexandrudima

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Yeah, hope you don't mind I renamed it to wordWrap

@hashhar

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

@alexandrudima No. It makes more sense. And the description is more clear about what it will do. Thanks. Sorry for the disturbance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.