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

Add line number interval setting #37120

Merged
merged 9 commits into from Nov 14, 2017

Conversation

Projects
None yet
7 participants
@dotweber

dotweber commented Oct 30, 2017

This adds a setting to control the interval at which line numbers are rendered in the editor, described in #36981. You can specify a numerical interval as well as whether or not to show the line number for the current cursor position.

screen shot 2017-10-30 at 3 12 49 pm

screen shot 2017-10-30 at 3 14 59 pm

@alexandrudima alexandrudima added this to the November 2017 milestone Oct 30, 2017

@dotweber dotweber changed the title from Feature/line number interval setting to Add line number interval setting Oct 30, 2017

@wiktor-k

This comment has been minimized.

Show comment
Hide comment
@wiktor-k

wiktor-k Nov 2, 2017

Looks very good! Thanks.

wiktor-k commented Nov 2, 2017

Looks very good! Thanks.

@alexandrudima

For our users sake, who will have to figure out how to configure this setting, I would kindly ask that we remove the OR type for this setting... IMHO, it makes it too complicated.

Show outdated Hide outdated src/vs/editor/common/config/commonEditorConfig.ts Outdated
@wiktor-k

This comment has been minimized.

Show comment
Hide comment
@wiktor-k

wiktor-k Nov 8, 2017

I've fixed issues reported by @alexandrudima (splitting settings) on top of @DDWR's changes but as I can't push these changes to this PR.

Should I create a new PR for that or would it be possible for maintainers to push my changes to this PR?

Thanks in advance for help!

wiktor-k commented Nov 8, 2017

I've fixed issues reported by @alexandrudima (splitting settings) on top of @DDWR's changes but as I can't push these changes to this PR.

Should I create a new PR for that or would it be possible for maintainers to push my changes to this PR?

Thanks in advance for help!

@dotweber

This comment has been minimized.

Show comment
Hide comment
@dotweber

dotweber Nov 9, 2017

@alexandrudima I thought it would be acceptable to use the union type since there are several throughout the configuration settings, with editor.quickSuggestions being a good example:

https://github.com/Microsoft/vscode/blob/master/src/vs/editor/common/config/commonEditorConfig.ts#L355

As requested, I made it so that lineNumberInterval is no longer a union type.

dotweber commented Nov 9, 2017

@alexandrudima I thought it would be acceptable to use the union type since there are several throughout the configuration settings, with editor.quickSuggestions being a good example:

https://github.com/Microsoft/vscode/blob/master/src/vs/editor/common/config/commonEditorConfig.ts#L355

As requested, I made it so that lineNumberInterval is no longer a union type.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

The editor.quickSuggestions is a union type for backwards compatibility purposes and should not be used as a good example. It shipped for a couple of years as a boolean, and then someone in the team refined it.

Member

alexandrudima commented Nov 14, 2017

The editor.quickSuggestions is a union type for backwards compatibility purposes and should not be used as a good example. It shipped for a couple of years as a boolean, and then someone in the team refined it.

@alexandrudima alexandrudima merged commit 0e0e347 into Microsoft:master Nov 14, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Details
@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

@DDWR Thank you for your contribution! ❤️

Member

alexandrudima commented Nov 14, 2017

@DDWR Thank you for your contribution! ❤️

@elektronik2k5

This comment has been minimized.

Show comment
Hide comment
@elektronik2k5

elektronik2k5 Dec 15, 2017

I would love to have a setting to control the line interval, as suggested here: #36981 (comment). The default 10 lines is too sparse, but using a configurable interval, I could set it to 5 or 2 to have just enough line numbers.

elektronik2k5 commented Dec 15, 2017

I would love to have a setting to control the line interval, as suggested here: #36981 (comment). The default 10 lines is too sparse, but using a configurable interval, I could set it to 5 or 2 to have just enough line numbers.

@dotweber

This comment has been minimized.

Show comment
Hide comment
@dotweber

dotweber Dec 22, 2017

@elektronik2k5 The work I did on this PR did have a configurable interval length, but for whatever reason it was completely changed to being a hard-coded interval of 10. I had not noticed that my PR was heavily modified like this and am pretty surprised that no discussion was made around it. @alexandrudima can you please clarify why you made these changes?

dotweber commented Dec 22, 2017

@elektronik2k5 The work I did on this PR did have a configurable interval length, but for whatever reason it was completely changed to being a hard-coded interval of 10. I had not noticed that my PR was heavily modified like this and am pretty surprised that no discussion was made around it. @alexandrudima can you please clarify why you made these changes?

@TrevorSayre

This comment has been minimized.

Show comment
Hide comment
@TrevorSayre

TrevorSayre Jan 3, 2018

@alexandrudima Is there any chance we can get back the ability to specify an interval?

TrevorSayre commented Jan 3, 2018

@alexandrudima Is there any chance we can get back the ability to specify an interval?

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Feb 5, 2018

Member

@dotweber @TrevorSayre

It perhaps does not make sense, but I'm actively trying to add as little as possible to VS Code. And when something is added, I'm trying as much as possible to make it simple to use / configure / discover.

The initial feature request at #36981 had 1 upvote, so AFAIK there were a grand total of 3 interested people in it. The feature request does not mention a configurable interval, so I assumed @dotweber is overdelivering. Plus the screenshots appear to use the value 10. Plus I honestly believe 10 is a good default, as the line numbers are presented in base 10.

I am open to bring the possibility to configure the line numbers interval. But let's start with a fresh feature request (i.e. please create one); PR also most welcome.

Member

alexandrudima commented Feb 5, 2018

@dotweber @TrevorSayre

It perhaps does not make sense, but I'm actively trying to add as little as possible to VS Code. And when something is added, I'm trying as much as possible to make it simple to use / configure / discover.

The initial feature request at #36981 had 1 upvote, so AFAIK there were a grand total of 3 interested people in it. The feature request does not mention a configurable interval, so I assumed @dotweber is overdelivering. Plus the screenshots appear to use the value 10. Plus I honestly believe 10 is a good default, as the line numbers are presented in base 10.

I am open to bring the possibility to configure the line numbers interval. But let's start with a fresh feature request (i.e. please create one); PR also most welcome.

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