39 vscode extension better status bar information #48
Conversation
…ery query" This reverts commit 3d24c3d.
There was a problem hiding this comment.
Thanks a lot! This will eliminate a lot of confusion I've seen with people using Caesar. I've left some minor comments.
There's also an issue I've discovered: If there's a syntax error, a name cannot be resolved, or if there's a type error, then the status bar will falsely show "Verified!". I don't know where in the code this should be fixed, but it's definitely very confusing.
vscode-ext/src/StatusBarComponent.ts
Outdated
| this.view.show(); | ||
| } else { | ||
| this.view.hide(); | ||
| } | ||
| } | ||
|
|
||
| private extendedTooltip(extension: vscode.MarkdownString) { |
There was a problem hiding this comment.
Seems like there is a return type missing? I wonder why TypeScript doesn't complain about this.
Also, I think this method should be inlined. As far as I can tell, it's just used once.
vscode-ext/src/StatusBarComponent.ts
Outdated
| this.view.tooltip.isTrusted = true; | ||
| } | ||
|
|
||
| private countResults(results: [Range, VerifyResult][]): [number, number, number] { |
There was a problem hiding this comment.
I don't see a reason why this is a member function. Either make it a freestanding function or just inline its definition as well (I prefer the latter).
vscode-ext/src/StatusBarComponent.ts
Outdated
| private view: StatusBarItem; | ||
| private tooltipBase: vscode.MarkdownString = stoppedTooltipBase; | ||
| private tooltipExtension: vscode.MarkdownString = new vscode.MarkdownString(); |
There was a problem hiding this comment.
Maybe rename this to tooltipStatusText? The current name is a bit confusing to me.
vscode-ext/src/StatusBarComponent.ts
Outdated
| private view: StatusBarItem; | ||
| private tooltipBase: vscode.MarkdownString = stoppedTooltipBase; |
There was a problem hiding this comment.
Maybe rename this to tooltipMenuText?
| @@ -157,6 +157,13 @@ impl Files { | |||
| self.files.iter().find(|file| &file.path == path) | |||
| } | |||
|
|
|||
| pub fn find_uri(&self, document_id: TextDocumentIdentifier) -> Option<&Arc<StoredFile>> { | |||
There was a problem hiding this comment.
Is this method necessary? Why not just call find(SourceFilePath::Lsp(ident))?
| .lock() | ||
| .unwrap() | ||
| .find_uri(params.text_document.clone()) | ||
| .unwrap_or_else(|| { |
There was a problem hiding this comment.
Possibly just turn this into an .expect("Could not find file id for document"). The unwrap_or_else with a panic is a bit weird to me.
There was a problem hiding this comment.
Clippy complains about the expect and suggests the unwrap_or_else version. I think this is why the unwrap_or_else version is previously preferred in the run_server method as well.
vscode-ext/src/StatusBarComponent.ts
Outdated
| let someVerified = false; | ||
|
|
||
| const editorsWithResults = vscode.window.visibleTextEditors.filter((editor) => { | ||
| return editor.document.languageId === "heyvl" // Only HeyVL files |
There was a problem hiding this comment.
Why do you need to check for HeyVL files? Shouldn't this.verifyStatus already only contain just HeyVL files?
vscode-ext/src/StatusBarComponent.ts
Outdated
| for (const editor of editorsWithResults) { | ||
| const document_id: TextDocumentIdentifier = { uri: editor.document.uri.toString() }; | ||
|
|
||
| // The default will not be used because the filter above ensures that the document is in the map. |
There was a problem hiding this comment.
Why handle the default case then with ?? []? Why not just throw an error then?
vscode-ext/src/StatusBarComponent.ts
Outdated
| this.view.show(); | ||
| } else { | ||
| this.view.hide(); | ||
| } | ||
| } | ||
|
|
||
| private renderTooltip() { |
There was a problem hiding this comment.
Maybe add a comment that this renders the tooltip with the text provided in this.tooltipMenuText? Alternatively, just add tooltipMenuText as a parameter to this function and don't have it as an attribute at all?
vscode-ext/src/StatusBarComponent.ts
Outdated
| } | ||
|
|
||
|
|
||
| private handleReadyStatus() { |
There was a problem hiding this comment.
Maybe call this renderStatusReady
Closes #39.