feat(editor): add formatter status bar with smart configuration managment #16829
Conversation
…ement Introduce a new FormatterService abstraction and enhanced status bar integration for managing document formatters across the editor. New FormatterService (editor-formatter-service.ts): - Define FormatterService interface for formatter management - Provide formatter status, availability, and configuration APIs - Support preference scope detection (User/Workspace/Folder) - Emit events when formatters change Monaco implementation (monaco-formatter-service.ts): - Implement FormatterService using Monaco's language features - Extract formatter metadata from registered providers - Integrate with Theia's preference system for persistence Status bar integration (editor-language-status-service.ts): - Display context-aware formatter status based on available formatters - Handle three distinct states: - 0 formatters: Show error if configured formatter not installed - 1 formatter: Show checkmark with active formatter name - 2+ formatters: Show configured formatter or available count - Show warning/error icons for misconfigured formatters - Smart scope selection when changing formatter: - Auto-use existing scope for Folder/Workspace settings - Prompt for scope only when User setting exists or no setting - Click-to-configure support with quick pick for formatter selection Signed-off-by: Simon Graband <sgraband@eclipsesource.com>
ndoschek
left a comment
ndoschek
left a comment
There was a problem hiding this comment.
Hi @sgraband, thanks for this improvement!
Overall it looks solid to me already, but I noticed one issue and have a few suggestions, would be great if you can take another look, TIA!
- I have one issue, when testing the formatter item in the Theia example app.
When I try to switch the formatter, e.g. to the markdown table formatter, which is not configured as default one, I get this console error:
preference-service.ts:455 Uncaught (in promise) Error: Unable to write to Folder Settings because no resource is provided.
at PreferenceServiceImpl.set (preference-service.ts:455:19)
at MonacoFormatterService.setDefaultFormatter (monaco-formatter-service.ts:184:38)
at EditorLanguageStatusService.showFormatterQuickPick (editor-language-status-service.ts:328:37)
- I would prefer if we integrate the formatter item in the languageStatusItem (
{ }). This way the user could still pin the item if needed and we do not show it by default.
Also, could you please update the PR template, e.g. add a few test instructions, also for later reference.
PS: As discussed offline, I found a few issues with general formatting and configuring the formatting overall, but I will address this separately (GH-15959) after this PR is done.
| } | ||
|
|
||
| protected getEditorModel(editor: TextEditor): ITextModel | undefined { | ||
| const monacoEditor = MonacoEditor.get(this.editorManager.currentEditor); |
There was a problem hiding this comment.
The method accepts an editor parameter but ignores it, always using this.editorManager.currentEditor instead, shouldn't it use the parameter instead?
| const monacoEditor = MonacoEditor.get(this.editorManager.currentEditor); | |
| const monacoEditor = MonacoEditor.get(editor); |
| displayName: string; | ||
| } | ||
|
|
||
| export type FormatterSettingScope = 'user' | 'workspace' | 'auto' | 'none'; |
There was a problem hiding this comment.
I am not sure about this, this does not list the folder scope.
I would suggest to extend FormatterSettingScope to include 'folder' to match the preference scopes. Or if not, at least document this here then.
Also, independently, I think this could use some documentation to explain the different formatting scopes.
| editor: TextEditor, | ||
| formatters: ReturnType<FormatterService['getAvailableFormatters']> | ||
| ): Promise<QuickPickItem | undefined> { | ||
| const status = this.formatterService!.getFormatterStatus(editor); |
There was a problem hiding this comment.
After the guard clause at lines 303-305, all subsequent uses of FormatterService and QuickInputService within that method scope are safe and don't need !, so I think we should remove ! operators after the guard clauses.
| if (currentScope === PreferenceScope.Folder) { | ||
| return PreferenceScope.Folder; | ||
| } | ||
|
|
||
| if (currentScope === PreferenceScope.Workspace) { | ||
| return PreferenceScope.Workspace; | ||
| } | ||
|
|
||
| if (currentScope === PreferenceScope.User) { | ||
| return this.showScopeSelectionPick(); | ||
| } | ||
|
|
||
| return this.showScopeSelectionPick(); |
There was a problem hiding this comment.
This could be simplified:
| if (currentScope === PreferenceScope.Folder) { | |
| return PreferenceScope.Folder; | |
| } | |
| if (currentScope === PreferenceScope.Workspace) { | |
| return PreferenceScope.Workspace; | |
| } | |
| if (currentScope === PreferenceScope.User) { | |
| return this.showScopeSelectionPick(); | |
| } | |
| return this.showScopeSelectionPick(); | |
| if (currentScope === PreferenceScope.Folder || currentScope === PreferenceScope.Workspace) { | |
| return currentScope; | |
| } | |
| return this.showScopeSelectionPick(); |
There was a problem hiding this comment.
We could also consider extracting formatter-related functionality into a separate class for easier maintenance and better adherence to Single Responsibility Principle.
Introduce a new FormatterService abstraction and enhanced status bar integration for managing document formatters across the editor.
New FormatterService (editor-formatter-service.ts):
Monaco implementation (monaco-formatter-service.ts):
Status bar integration (editor-language-status-service.ts):
What it does
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers