This issue is a blocker for the .drawio.png feature.
This is my context.
I hope something can be done there ;)
- Default Generic Type Argument
I have to admit, I failed to recognize that CustomEditorProvider is generic when I exploratively implemented my first prototype.
I wouldn't make the type parameter optional. Custom binary editors are so advanced, a default does not help much here.
At first, I declared a cast: CustomDocument -> DrawioDocument function to get typesafety back.
Only when I looked at your examples, I figured out how to do it properly.
Of course, basic reasoning on my side would have conluded that it must be generic.
Somehow, I didn't really understand at first whether I should subclass CustomDocument or just use object literals.
- Delegation Methods
I know these service like methods seem to be your design guideline, but I don't understand the benefit here.
If I had designed this API, I would have put all those {action}CustomDocument as an {action} method on the CustomDocument interface.
Basically this is what you did in your examples and what I did in my implementation. We both had to delegate all those methods to the document though.
I think having a stronger CustomDocument interface would've somehow avoided my first confusion and imo leads to better consumer code.
It also makes life cycles clearer, as you are forced to implement openCustomDocument first.
- Undo methods
I used a code action to automatically implement the CustomEditorProvider interface.
It generated this:
// ...
readonly onDidChangeCustomDocument: Event<CustomDocumentEditEvent<T>> | Event<CustomDocumentContentChangeEvent<T>>;
// ...Because I couldn't tell these events apart, I settled on the first and changed it to this:
// ...
private readonly onDidChangeCustomDocumentEmitter = new EventEmitter<
CustomDocumentEditEvent<DrawioDocument>
>();
public readonly onDidChangeCustomDocument = this
.onDidChangeCustomDocumentEmitter.event;
// ...It required me to specify undo and redo which does not make sense for drawio, since drawio has its own undo stack.
I failed to see how I could disable that. Took me like half an hour to figure that out.
I would not have run into this issue if a CustomEditorProvider had a property supportsUndo: true and supportsUndo: false,
each requiring to implement either Event<CustomDocumentEditEvent<T>> or Event<CustomDocumentContentChangeEvent<T>>.
- Too much documentation
This might be just me :D Because of 3., I had to open the docs to see how to disable the undo/redo feature. The docs overwhelmed me. It took me quite some time to find this (I know I just could have searched for "undo"):
Custom editors that use CustomDocumentContentChangeEvent do not support undo/redo.
- saveAs
I didn't really know whether saveAs updates the internal URI or not. The doc comments read:
When the user accepts save as, the current editor is be replaced by an non-dirty editor for the newly saved file.
I didn't understand whether Vs Code does that for me or whether I do have to do it on my own.
I tried to save a drawio diagram as svg or so (I cannot remember it anymore) and something funny happened that did not really help me (I only have a vague memory of that).
workspace.fs
I had funny issues with vscode.d.ts and got a totally outdated one (I used an old extension of me as a template (all my others are more complicated), it still used vscode rather than @types/vscode).
I wondered whether there was a file system abstraction (cause I knew about your custom file system providers), but was unable to find something from within VS Code, so I used fs.readSync(destination.filePath).
Only when looking at your example, I found out about my outdated vscode.d.ts.
I think using workspace.fs is very crucial, so it deserves a mention in saveCustomDocument and saveCustomDocumentAs.
CustomDocumentBackupContext.destination
I only found out about this one when I studied your example.
I already feared to implement my own random id generation, as the doc comments of backupCustomDocument just read this:
Most commonly this means saving the resource to disk in the
ExtensionContext.storagePath
I think I would have saved some thoughts/fears/minutes if these docs there mentioned that I could just saveTo(context.destination).
Somehow just alone thinking about having to find a unique file in ExtensionContext.storagePath and this stuff drained my energy and demotivated me.
(I would have to add a dependency to something that can generate random strings, to account for collisions, to think about whether the directory exists or not, ...)
I was pretty relieved when I learned about context.destination!!
All in all, I was quite happy with it since I got the job done in a reasonable amount of time (a single day). Only the blocker is urgent for me.