-
Notifications
You must be signed in to change notification settings - Fork 742
feat(editors): indicate when editor contains error or warning #1816
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
base: main
Are you sure you want to change the base?
Changes from all commits
380ec66
0490785
3ed8329
4c6df61
44576ba
df5c9ef
6b2a491
ba71f34
53131ea
f245417
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,11 @@ | ||
| import { action, computed, makeObservable, observable, reaction } from 'mobx'; | ||
| import { | ||
| action, | ||
| computed, | ||
| makeObservable, | ||
| observable, | ||
| reaction, | ||
| runInAction, | ||
| } from 'mobx'; | ||
| import type * as MonacoType from 'monaco-editor'; | ||
| import { MosaicDirection, MosaicNode, getLeaves } from 'react-mosaic-component'; | ||
|
|
||
|
|
@@ -84,6 +91,7 @@ export class EditorMosaic { | |
| setEditorFromBackup: action, | ||
| addNewFile: action, | ||
| renameFile: action, | ||
| editorSeverityMap: observable, | ||
| }); | ||
|
|
||
| // whenever the mosaics are changed, | ||
|
|
@@ -104,6 +112,9 @@ export class EditorMosaic { | |
| } | ||
| }, | ||
| ); | ||
| // TODO: evaluate if we need to dispose of the listener when this class is | ||
| // destroyed via FinalizationRegistry | ||
| window.monaco.editor.onDidChangeMarkers(this.setSeverityLevels.bind(this)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: does this binding need to be cleaned up on destroy/ in the constructor?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's a good callout. After looking a bit more, I don't think there's an easy way of disposing of this callback since JavaScript classes don't provide a lifecycle hook for when the class gets destroyed (in the same way that React does when a component gets unmounted). Since If this proves to be a problem, we might want to look into the The whole "Avoid where possible" section in that API makes me just a bit wary for now. Happy for pushback on this assessment, though. |
||
| } | ||
|
|
||
| /** File is visible, focus file content */ | ||
|
|
@@ -160,8 +171,17 @@ export class EditorMosaic { | |
| // create a monaco model with the file's contents | ||
| const { monaco } = window; | ||
| const language = monacoLanguage(id); | ||
| const model = monaco.editor.createModel(value, language); | ||
|
|
||
| // set a URI for each editor for stable identification for monaco features | ||
| const uri = monaco.Uri.parse(`inmemory://fiddle/${id}`); | ||
| let model: MonacoType.editor.ITextModel; | ||
| const maybeModel = monaco.editor.getModel(uri); | ||
| if (maybeModel) { | ||
| model = maybeModel; | ||
| model.setValue(value); | ||
| } else { | ||
| model = monaco.editor.createModel(value, language, uri); | ||
| } | ||
| // if we have an editor available, use the monaco model now. | ||
| // otherwise, save the file in `this.backups` for future use. | ||
| const backup: EditorBackup = { model }; | ||
|
|
@@ -263,6 +283,7 @@ export class EditorMosaic { | |
|
|
||
| this.backups.delete(id); | ||
| this.editors.set(id, editor); | ||
| this.editorSeverityMap.set(id, window.monaco.MarkerSeverity.Hint); | ||
| this.setEditorFromBackup(editor, backup); | ||
| } | ||
|
|
||
|
|
@@ -342,6 +363,10 @@ export class EditorMosaic { | |
| } | ||
| }; | ||
|
|
||
| public getAllEditorIds(): EditorId[] { | ||
| return [...this.editors.keys()]; | ||
| } | ||
|
|
||
| public getAllEditors(): Editor[] { | ||
| return [...this.editors.values()]; | ||
| } | ||
|
|
@@ -380,4 +405,28 @@ export class EditorMosaic { | |
| disposable.dispose(); | ||
| }); | ||
| } | ||
|
|
||
| public editorSeverityMap = observable.map< | ||
| EditorId, | ||
| MonacoType.MarkerSeverity | ||
| >(); | ||
|
|
||
| public setSeverityLevels() { | ||
| runInAction(() => { | ||
| for (const id of this.getAllEditorIds()) { | ||
| const markers = window.monaco.editor.getModelMarkers({ | ||
| resource: window.monaco.Uri.parse(`inmemory://fiddle/${id}`), | ||
| }); | ||
|
|
||
| const maxSeverity: MonacoType.MarkerSeverity = markers.reduce( | ||
| (max, marker) => { | ||
| return Math.max(max, marker.severity); | ||
| }, | ||
| window.monaco.MarkerSeverity.Hint, | ||
| ); | ||
|
|
||
| this.editorSeverityMap.set(id, maxSeverity); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
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.
question: can we just have
const severityLevel = editorMosaic.editorSeverityMaphere?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
toJScall here is to force a re-render when the values change.Due to the way
renderToolbar()is only called deep in the third-party Mosaic component, I don't think React + MobX is able to pick up the changes and re-render without this hack.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.
Added a comment indicating this