Conversation
mjbvz
left a comment
mjbvz
left a comment
There was a problem hiding this comment.
Submitted a few suggestions
I'm having a hard time understanding what the code is doing. I think encapsulating the logic inside classes and functions would help make this more clear
| ); | ||
|
|
||
| context.subscriptions.push(vscode.workspace.onDidChangeNotebookDocument(e => { | ||
| delayTrigger.trigger(e); |
There was a problem hiding this comment.
If you are editing quickly, this may end up firing after onDidCloseNotebookDocument or onDidRenameFiles. Is that case properly handled?
There was a problem hiding this comment.
likely it won't be, I'm not sure exactly how I would go about handling that. Do you have any initial thoughts?
There was a problem hiding this comment.
One idea: instead of passing e along in a closure, add it to a list. When the delay trigger is finally invoked, it should loop through items in the list. If close is called before that though, remove the entry from the list
There was a problem hiding this comment.
Not completely understanding that as a solution, as when onDidCloseNotebookDocument is invoked, the notebook will already be closed and if a workspace edit is applied, the notebook would reopen dirty. I think this would work/there would be plenty of other solutions if an onWillClose listener was possible, but don't think there's any support for that at the moment.
Current fix is just to check if the notebook is closed at the very beginning of the clean function. It means that if someone is editing quickly they might not have their notebook cleaned, but not sure there is a way to hook in right before the notebook is closed.
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| export function deepClone<T>(obj: T): T { |
There was a problem hiding this comment.
we don't have more generally available versions of these functions? (and better tested?)
There was a problem hiding this comment.
@amunger currently we don't, I borrowed this from the core.
|
@mjbvz I picked up this PR since @Yoyokrazy is out. By reading through the suggestions above, I have made following changes:
Along with changes in the clean up tool, also made required changes to the notebook diff editor to ensure the attachment metadata is presented in the diff editor. I didn't try to tackle the race condition between notebook document change and document close. It might require introducing code action for notebook document save (which only exists in text document) as current approach of attachment cleanup is responding to cell document changed event, but not as a pre-change hook. We can discuss what's the best approach to tackle this. @mjbvz it would be great if you can take another look at this PR. |
| } | ||
| } | ||
| ], | ||
| "configuration": [ |
| const vscodeDiagnostics: vscode.Diagnostic[] = []; | ||
| for (const currDiagnostic of diagnostics) { | ||
| currDiagnostic.ranges.forEach(range => { | ||
| vscodeDiagnostics.push(new vscode.Diagnostic(range, `Attachment ${currDiagnostic.name} not available`, vscode.DiagnosticSeverity.Warning)); |
There was a problem hiding this comment.
We didn't set up nls yet in code.
|
@mjbvz thanks for the review, updated the code per your comments ;) |
| }, | ||
| { | ||
| "command": "ipynb.cleanInvalidImageAttachment", | ||
| "title": "Clean invalid image attachment reference" |
There was a problem hiding this comment.
Do we need the word "reference" here?
There was a problem hiding this comment.
I'm using reference for the image attachment link in the cell document, and use attachment for the data stored in the metadata.
| const fixes: vscode.CodeAction[] = []; | ||
|
|
||
| for (const diagnostic of context.diagnostics) { | ||
| switch (diagnostic.code) { |
There was a problem hiding this comment.
Actually, is this loop just looking to see whether a single diagnostic of type missing_attachment exists?
There was a problem hiding this comment.
There might be multiple diagnostics in the context (e.g., referencing the same invalid attachment multiple times).
6 participants
Notebook will automatically remove un-used image attachments from a cell's metadata, storing them in a cache (to allow for user to move text around and not lose image data).
Cache is structured with entries for each notebook uri, and then each cell fragment under those. top level uri entries will be deleted when notebook editors are closed. The entire cache is disposed of upon the extension closing with the workspace.