Skip to content
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

Custom Editor API Feedback - Controlling the Dirty Flag #97348

Closed
hediet opened this issue May 9, 2020 · 12 comments
Closed

Custom Editor API Feedback - Controlling the Dirty Flag #97348

hediet opened this issue May 9, 2020 · 12 comments
Assignees
Labels
custom-editors Custom editor API (webview based editors) *out-of-scope Posted issue is not in scope of VS Code under-discussion Issue is under discussion for relevance, priority, approach

Comments

@hediet
Copy link
Member

hediet commented May 9, 2020

I'm trying to integrate drawio into VS Code as editor for pngs with an embedded drawio document.
As pngs are binary files, I'm using the proposed custom editor API.

The drawio iframe captures all key events (so that shortcuts work). This includes ctrl+s.
In my extension, I can react to that (drawio sends a save request to the parent window when ctrl+s is pressed in the drawio iframe).

However, I don't see how I can clear the dirty state of VS Code from my extension. I could just save the file on disk when I get the save event from the webview, but VS Codes dirty flag would remain changed. That wouldn't be a great user experience.

It would be great if VS Code could provide an API to reset the dirty flag.
An alternative would be an API to save custom documents though VS Code. It would call saveCustomDocument of the custom editor provider and then reset the dirty flag.

@jrieken
Copy link
Member

jrieken commented May 11, 2020

This includes ctrl+s. In my extension, I can react to that

In addition to explicit save (via saveCustomDocument) or only via its own gestures? The latter would mean the File > Save, custom keybindings etc won't work...

Doesn't have drawio events for when things are changing? That you could simply forward via the provider's event?

@mjbvz mjbvz added custom-editors Custom editor API (webview based editors) under-discussion Issue is under discussion for relevance, priority, approach labels May 11, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented May 11, 2020

We will likely not add direct control over the dirty bit. The consequences of getting dirty tracking wrong are quite bad—including user data loss or blocking the user from exiting VS Code—and it is surprisingly difficult to get correct.

Your extension needs to let VS Code fire the save event and have this trigger drawio's save (or whatever you need to actually save the data)

@hediet
Copy link
Member Author

hediet commented May 11, 2020

via its own gestures?

Via its own gestures. The drawio iframe captures all key events and prevents them from propagation. Keybindings from VS Code don't work when the iframe is focused and shouldn't, as drawio has its own keybindings.

Pressing Ctrl+S when the drawio iframe is focused will not trigger a save by VS Code. The iframe will send a message to my extension though, indicating that Ctrl+S has been pressed. From there I would like to request a save that also clears the dirty flag.

If I call save on a TextDocument, the text document is saved and VS Code resets the dirty flag.
I would like to have a similar save method on my custom document, that too saves the document and resets the dirty flag. I don't see a way how I can implement a method with this effect.

Your extension needs to let VS Code fire the save event and have this trigger drawio's save (or whatever you need to actually save the data)

How do I do that for custom documents?

@mjbvz
Copy link
Collaborator

mjbvz commented May 11, 2020

@hediet Do you currently implement saveCustomDocument? What does the implementation do?

Keybindings from VS Code don't work when the iframe is focused and shouldn't, as drawio has its own keybindings.

You'll need to address this at some point. Users can rebind command like save, undo, and redo and the VS Code keybindings should always be used for these operations. Otherwise the custom editor will always feel alien even though it is displayed in VS Code

@hediet
Copy link
Member Author

hediet commented May 11, 2020

Do you currently implement saveCustomDocument? What does the implementation do?

Yes. The implementation writes the current (internal) model of the document to disk. But this does not reset the dirty flag when called directly (obviously, VS Code couldn't know that it is meant to do that).
I'm looking for a way to save a document so that its dirty flag becomes cleared.

You'll need to address at some point. Users can rebind command like save, undo, and redo and the VS Code keybindings should always be used for these operations. Otherwise the custom editor will always feel alien even though it is displayed in VS Code

That is true, but given that draw.io is quite a complex product a lot of people are already used to, it might feel alien to those if none of their drawio-shortcuts work. Also, I would need to change Draw.io's code for that. Implementing that is easily as much work as the entire extension has been so far. Imo, the added value does not relate to the effort it requires.

Being able to trigger VS Code to save a specific custom document so that it's dirty flag is cleared might be good thing to have, anyways.

@mjbvz
Copy link
Collaborator

mjbvz commented May 12, 2020

We've discussed this at length while designing custom editors #77131

The custom editor API sets certain minimum bars that editors need to meet in order to provide good experience to users. These requirements are pretty loose, and there are multiple tiers of custom editors you can implement: readonly, basic editable, editable with undo/redo. We know that means that many existing editor libraries will require changes.

For basic editable editors, we require that VS Code handles save. For your extension, this means either updating drawio to have proper hooks for keyboard events, or intercepting the save keypresses before they reach drawio and then rebroadcasting them back to VS Code. Changing the API on the vscode side would only be a workaround and I believe you will have to solve this problem at some point no matter what.

If you cannot implement the basic editable API at the moment, you also use a readonly custom editor. This would leave your extension free to handle all keyboard presses and save logic (with the clear drawback of never being able to mark your editors dirty, which could cause data loss)

@mjbvz mjbvz closed this as completed May 12, 2020
@mjbvz mjbvz added the *out-of-scope Posted issue is not in scope of VS Code label May 12, 2020
@hediet
Copy link
Member Author

hediet commented May 12, 2020

or intercepting the save keypresses before they reach drawio and then rebroadcasting them back to VS Code

Then please tell me how to do that. This was one of the very first things I asked:

An alternative would be an API to save custom documents through VS Code. It would call saveCustomDocument of the custom editor provider and then reset the dirty flag.

I don't see which API VS Code provides to rebroadcast the save event.

@mjbvz
Copy link
Collaborator

mjbvz commented May 12, 2020

This something you need to do in the webview and there is no VS Code api for it. You need to prevent drawio from eating the keyboard presses. As long as the top frame can see a ctrl+s keydown event, it should rebroadcast that to VS Code

@hediet
Copy link
Member Author

hediet commented May 12, 2020

But don't you think that some kind of command would be useful that lets you properly save a custom document programmatically?
Until now, you can save any other document programmatically. I'm wondering how the vim extension will do this.

I mean with your proposal, the only way to properly save a custom document would be sending the keystroke Ctrl+S or workspace.saveAll();
There seems to be no other way.

@mjbvz
Copy link
Collaborator

mjbvz commented May 12, 2020

If there is a compelling use case, possibly. However I don't think it is the correct fix to the problem you are facing

@zaydek
Copy link

zaydek commented May 12, 2020

What may help is if there is some documentation (if there isn’t already) which makes it clear to custom editor developers that user events need to be first visible to VSCode. I totally feel for this problem; apps that were architected before understanding how VSCode custom editors work will at some level be in contradiction with each other (by no fault of editor developers or VSCode).

For example, I don’t think the editor I’m building will suffer from this specific use-case, but I probably need to add some logic to my editor to not manage a history state stack and let VSCode do that work. I don’t think, no matter how clever I am, could have perceived that is something I shouldn’t engineer because VSCode will do that for me.

I don’t know the solution but I can appreciate the problem; VSCode-first and editor-first architectures have some conflicting design goals. For me, realizing that VSCode editor extensions are ‘managed’ helped me understand this paradigm more easily.

@hediet
Copy link
Member Author

hediet commented May 12, 2020

I'm feeling stupid now because I didn't see that commands.executeCommand("workbench.action.files.save"); is all I ever wanted.

It's conceptually not clean, since it assumes that the document I want to save is the currently focused document. But it's the best option I have.

I'm not going to map these drawio shortcuts to VS Code commands with equivalent default shortcuts anytime soon. That's an horrific amount of work for not much gain. Also, draw.io has not command API, so commands must be somehow triggered with virtual key strokes (basically the same problem I have with VS Code right now, but only the opposite direction).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
custom-editors Custom editor API (webview based editors) *out-of-scope Posted issue is not in scope of VS Code under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

4 participants