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

vscode support of editor.indentSize #13105

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

rschnekenbu
Copy link
Contributor

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.

Fixes #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

@rschnekenbu rschnekenbu force-pushed the issues/13058 branch 2 times, most recently from 1f54497 to cde68b3 Compare November 28, 2023 12:47
@tsmaeder tsmaeder self-requested a review November 28, 2023 13:26
@rschnekenbu
Copy link
Contributor Author

Thanks a lot for your review, @msujew ! I updated the code with your comments, this can be reviewed again.

};

protected toModelOption(editorPreference: EditorPreferenceChange['preferenceName']): keyof ITextModelUpdateOptions | undefined {
switch (editorPreference) {
case 'editor.tabSize': return 'tabSize';
// @monaco-uplift: uncomment this line once 'editor.indentSize' preference is available
// case 'editor.indentSize': return 'tabSize';
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Given that the previous question resulted in indentSize, it should be here the same as well.

Suggested change
// case 'editor.indentSize': return 'tabSize';
// case 'editor.indentSize': return 'indentSize';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with new commit.

}
this._indentSize = indentSize;
} else if (val === 'tabSize') {
this._indentSize = this._tabSize!;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: I believe this might yield incorrect values in case the tabSize is changed after indentSize has been set:

ext.tabSize = 2;
ext.indentSize = 'tabSize';
ext.tabSize = 4;
console.log(ext.indentSize) // prints '2'

It should store the 'tabSize' value and return the current tabSize value from the indentSize getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this should be a better long term solution. Fixed in new version

Also updates tabSize, as API does not correspond to documentation.

Contributed on behalf of STMicroelectronics

Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
@rschnekenbu
Copy link
Contributor Author

rschnekenbu commented Nov 29, 2023

@msujew, thanks again for your review!
All your comments were addressed. I also rebased on latest master.

@rschnekenbu rschnekenbu requested a review from msujew November 29, 2023 08:16
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me 👍

@rschnekenbu
Copy link
Contributor Author

@msujew, can you merge it please? I can then create the PR for #13065, as this is the last remaining task for the vscode compatibility task.

@tsmaeder tsmaeder merged commit f855288 into eclipse-theia:master Nov 29, 2023
14 checks passed
@vince-fugnitto vince-fugnitto added this to the 1.44.0 milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[vscode] Support TextEditorOptions#indentSize added with 1.83
4 participants