-
Notifications
You must be signed in to change notification settings - Fork 5
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
[vscode] support of editor.indentSize #94
Conversation
return r > 0 ? r : undefined; | ||
} | ||
if (typeof val === 'string') { | ||
if (val === 'tabSize') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would have been picked up in the first if
in this method, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, fixed with new version
execute: () => | ||
model.updateOptions({ | ||
tabSize: size || tabSize, | ||
indentSize: size || tabSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to set the indent size here? Doesn't this only apply when the indent size is 'tabSize'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I only did the same behavior as vscode is doing (https://github.com/microsoft/vscode/blob/main/src/vs/editor/contrib/indentation/browser/indentation.ts#L254). My initial implementation was to run 2 different cases, if we use spaces or not. If not, I was only updating tabSize. If useSpace, then I would have only updated the indentSize. But the code would have been more complex, and results should be the same.
In this case, size should only be a number as we can pick from 1 to 8.
} | ||
|
||
set indentSize(val: number | string | undefined) { | ||
const indentSize = this.validateIndentSize(val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc says:
When getting a text editor's options, this property will always be a number (resolved). When setting a text editor's options, this property is optional and it can be a number or "tabSize".
with the code as it is, I believe when I do
options.indentSize= 'tabSize?;
console.log(options.indentSize);
I would see "tabSize" in the console, not the numeric value of the tab size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some fixes in this class, as the code was not aligned with documentation indeed. The same remark was applying to tabSize, I also updated accordingly.
Finally, I fixed what seems to me a bug on tabSize, where the _tabSize attribute was never set by the setter, and the line 310 and 313 would lead to an infinite loop, calling setter again and again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any changes to this code in your latest commit. Something missing in it?
@tsmaeder, the PR is ready for review again. I did some changes also in tabSize management to align the APIs and the documentation. |
Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>
…a#13084) This ensures that webview logs are correctly propagated to the backend logger Contributed on behalf of STMicroelectronics
69d1139
to
90a56d4
Compare
Also updates tabSize, as API does not correspond to documentation. Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>
90a56d4
to
1f54497
Compare
closed in favor of PR on main repo |
What it does
Fix missing API TextEditorOptions.indentSize made public on vscode 1.83 (was a proposed API before).
The behavior is available, but the preference requires an uplift of the monaco version. It is not yet available in the generated preferences.
Note; changelog will be pushed once PR is created against main repo.
Fixes eclipse-theia#13058
Contributed on behalf of ST Microelectronics
How to test
The test can be done on a text file, using "Indent using spaces" and "Indent using tabs" commands. The behavior should be the same as before. Only the internals have changed.
The preferences test has to wait the monaco uplift.
The following provided extension can also show the value of identSize when the value has changed with an information message:
Follow-ups
Preferences need to be supported once a monaco uplift has been performed.
Review checklist
Reminder for reviewers