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

Separate tab size and indent size #155450

Merged
merged 5 commits into from
Nov 16, 2022
Merged

Conversation

zeroimpl
Copy link
Contributor

This PR fixes #42740, #10339, #5394, #135058

It enables having separate values for indentation and the display width of tab characters, which is a common requirement of some older projects and/or coding styles. In addition to adding support for an editor.indentSize property, the indentation options on the status bar have been updated to allow independently configuring editor.indentSize and editor.tabSize.

Most of this work was previously implemented but never released. This commit fully enables the feature and adds a few extra enhancements.

@ghost
Copy link

ghost commented Jul 18, 2022

CLA assistant check
All CLA requirements met.

This change allows the indentation size to be distinct from the tab width,
when using spaces for indentation.

This commit simply reverts e2049cd and portions of a60beb9, which had
removed this feature.
Make the "Indent Using Spaces" action not affect the displayed tab width.
Add a new "Change Tab Display Size" action to change that independently.
If the indentSize and tabSize are different, show both in the status bar.

Also for the indentation actions,  replace the "Configured Tab Size" hint
with a "Selected Tab Size" and  "Default Tab Size" options, when it is
ambiguous.
src/vscode-dts/vscode.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Magnus Ihse Bursie <mag@icus.se>
magicus
magicus previously approved these changes Aug 1, 2022
Copy link
Contributor

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a formal reviewer on this project, but as far as I can tell, this PR resolves the issue of properly separating tab and indent size, in a way that does not break compatibility.

@alexdima
Copy link
Member

alexdima commented Aug 2, 2022

The root problem here is that we haven't reached consensus on the way this feature should work, on what configuration options are exposed to users, on what we forward as formatting options to our API, or on what is the exact way in which to avoid breaking all our current users (i.e. all of their custom configurations should behave exactly as they do today).

@zeroimpl
Copy link
Contributor Author

zeroimpl commented Aug 2, 2022

@alexdima It’s not clear to me what the issues are.

In terms of how the feature should work, it’s pretty clear that having 2 separate variables for tabSize and indentSize is the way to go, this is what everybody else does. I understand there’s a possibility of supporting some mixed indentation modes in the future where both tabs and spaces are used, but that seems quite unrelated to me and would likely still leverage these 2 variables. Same goes for the configuration options to users - having 2 config options for these 2 variables is the norm.

The existing API is broken already by having tabSize be interpreted as indentSize, and I’m not sure what changes would be had in mind here, nor how this PR makes it worse.

I don’t think this change should break any custom configurations as it preserves the existing behavior as the default. However I don’t know much about how other people customize their VSCode. In my case I tried a few things including some extensions but couldn’t find any way to customize it to indent with 4 spaces but display existing tabs as 8 spaces.

Given that this is quite a longstanding issue and appears to be relatively harmless fix to me, I feel should prioritize getting some fix in anyways even if it isn’t perfect. Or at the very least could have a timeline planned for addressing the issue in full - there’s currently no mention of this on the roadmap.

@magicus
Copy link
Contributor

magicus commented Aug 8, 2022

@alexdima When you say "consensus", who are the interested parties that need to reach consensus?

I understand that the lack of a firm decision on how to handle this issue has been a stumbling stone for quite some time, but for me, being an outsider of Microsoft, it is not clear who needs to get involved to reach this consensus., nor if any attempt is made to get the attention of those whose cooperation is needed to move this forward.

Just saying "we do not have consensus", but not making the effort needed to gather the interested parties, is just saying "we do not intend to fix this". There is a lot of consensus among us users who are being hampered by this lack in VS Code, but apparently that is not the consensus that is required here.

@magicus
Copy link
Contributor

magicus commented Oct 19, 2022

@alexdima Has there been any progress in gathering the interested parties needed to reach the consensus needed to move along with this? Can you at least share some update on whom we are waiting for to provide input on how we can proceed? This complete silence on any kind of progress, or what we as a community can do to help move things along, is disheartening. :-(

@alexdima
Copy link
Member

@rebornix @Tyriar Could we please get your approval on this PR as well?

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how to configure a project with this change, I set this:

{
  "editor.indentSize": 2,
  "editor.insertSpaces": true,
  "editor.tabSize": 8
}

And was expecting tab to add 2 spaces and convert to a tab when it goes from 6 to 8. But tab adds 8 spaces and the indent guide shows at 8 spaces. Am I misunderstanding something? This separation of indent and tab size is pretty confusing to me personally.

@zeroimpl
Copy link
Contributor Author

@Tyriar It doesn't convert spaces into tabs. With the above configuration, any existing tabs in the file will be aligned to 8 spaces, but pressing the tab key will only ever insert spaces, and will insert 2 at a time.

Converting leading spaces to tabs automatically should likely be a separate PR, and would need a separate config parameter to control it.

@Tyriar
Copy link
Member

Tyriar commented Oct 25, 2022

With the above configuration, any existing tabs in the file will be aligned to 8 spaces, but pressing the tab key will only ever insert spaces, and will insert 2 at a time.

This isn't what I was seeing when running the PR, it was adding 8 spaces when I pressed tab 🤔

@zeroimpl
Copy link
Contributor Author

Ah you also need to set this:

"editor.detectIndentation": false,

Otherwise it auto-detects the config and overrides whatever you've set in your settings.json for those other parameters. That is confusing, but I'm pretty sure that's unchanged with this PR.

@Tyriar
Copy link
Member

Tyriar commented Oct 25, 2022

Ah ok, that seems to do as it says then, it's just not clear how someone would actually input a tab with this setup apart from copying the character from somewhere else. As long as this doesn't interrupt the default (which it shouldn't because of "tabSize"), I don't see an issue with having this merged, it may still be an educational/discovery issue for the users that need it though.

@pauljamesreid
Copy link

Long time lurker here - Is it just a case of being patient now? I've been using Eclipse (for way too long) now purely because it supports this (we're working on a large, old, codebase that cannot be simply 're-factored'). Would love to ditch it and move over to VSC.

@zeroimpl : If/when this makes it in, you'll be making at least 4 devs here very happy :-D

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this code but the concept and a quick skim of the code lgtm. @rebornix could you check this out soon?

@rebornix
Copy link
Member

The code looks good to me 👍

@pauljamesreid
Copy link

Hi folks. I'm not familiar with the release process (it was somewhat desperate googling that led me to this thread in the first place). What else needs to happen before us users can get our hands on this?

@alexdima alexdima added this to the November 2022 milestone Nov 16, 2022
alexdima added a commit that referenced this pull request Nov 16, 2022
Separate tab size and indent size (PR #155450)
@alexdima alexdima merged commit 357f4e9 into microsoft:main Nov 16, 2022
@alexdima
Copy link
Member

Thank you @zeroimpl !

@simark
Copy link

simark commented Nov 16, 2022

@Tyriar It doesn't convert spaces into tabs. With the above configuration, any existing tabs in the file will be aligned to 8 spaces, but pressing the tab key will only ever insert spaces, and will insert 2 at a time.

Converting leading spaces to tabs automatically should likely be a separate PR, and would need a separate config parameter to control it.

Is there a bug open for this feature? That would be the last piece missing thing in order to be able to use VSCode for projects that use the GNU coding style.

@zeroimpl
Copy link
Contributor Author

Awesome thanks @alexdima and all for helping get this merged. Looking forward to having this feature included

@magicus
Copy link
Contributor

magicus commented Nov 16, 2022

Huge thanks for all involved! I am so looking forward to seeing this in a release.

@brlcad
Copy link

brlcad commented Nov 16, 2022

Per the above discussion, this PR does not address #42740 -- specifically the second bullet which is critical expected behavior for editing. I would also argue @Tyriar 's claim that a separate config parameter is needed to control that is incorrect and inconsistent with all other editors that support mixed editing. Having an editor ignore the tab setting and always insert spaces is nonsensical. That (still) conflates indent with tab incorrectly during editing. The PR does seem to at least address viewing.

@magicus
Copy link
Contributor

magicus commented Nov 16, 2022

@brlcad I think there are several different use cases here. Some of them are solved by this PR, but not all. I think the discussion is helped by being very clear what we are talking about.

My specific use case is that I am editing files where indents should be two spaces, but tab stops should be every eight spaces. Prior to this patch, to edit the file in vscode I had to either lie about the indent size (setting it to 8) which would make the source look correctly, or lie about the tab size (setting it to 2) to use editor support for proper indenting. This behavior will be fixed by this PR.

However, I understand that many people also use a "mixed" indentation strategy (apparently a GNU standard), which basically is "indent by X spaces, but if X is larger than T (the tab size), replace it with (X/T) tabs and then (X%T) spaces", or in other words, replace as many spaces as possible with tabs.

To me, that sounds definitely like a separate issue. Related, sure, but separate. Solving that would require e.g. changing the editor indentation status bar widget, to be able to select this "mixed" mode, in addition to "indent by tabs" and "indent by spaces".

I don't know if it is better to reuse one of the old issues that intermingled the display part (which has been integrated) with the mixed indentation part (which is still lacking), or to open a new, more specific issue. Personally I would think the latter would be clearer, but a new bug perhaps also risks not getting the proper attention of the vscode team.

@brlcad
Copy link

brlcad commented Nov 16, 2022

@magicus That is why I called out issue #42740 as it's already a separate more specific issue with clearly explained behavior. I'm not arguing against the merits or other value of this PR. Claiming this PR satisfies that issue is not correct.

It also does not appear to satisfy the older #5394 issue either which refers to matching a file's existing mixed indentation style during editing, which this PR also does not satisfy.

@pauljamesreid
Copy link

So I've been trying this on our (very large/old) codebase via the insiders build for the last day or so, and I have to say...it works like a charm! I'm sure it doesn't do everything everyone wants, but for my team here (and, I suspect, a very large number of devs like us currently suffering in silence) this is a great addition. We finally have a viable alternative to Eclipse :-D. Massive thanks again to @zeroimpl and the others who made this happen!

PS. If you haven't already, you really should make a big deal of this in the release docco ;-)

@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2022
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.

Separate tab size and indent size
10 participants