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

adapt to latest theia 1.24 #57

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

alxflam
Copy link
Contributor

@alxflam alxflam commented Apr 1, 2022

Hi,

with the current Theia 1.24 (and already with 1.23.0) the tree editor cannot be saved anymore.
See changes in Theia#10736 and alxflam/theia@e62fd33.
Monaco now defines different save preferences (the keys as well as the allowed values have changed).

Changes:

  • updated Theia dependency to ^1.24.0
  • implement SaveableSource and the methods createSnapshot and revert (from Saveable, dummy implementations like they were already present in a previous release) as they are now required (see WorkspaceSaveResourceService#save which calls workspaceFrontendContribution#canBeSavedAs)
  • the dummy implementations throw an exception mentioning that subclasses should override these methods
  • adapt autoSave due to changed type and preference key
  • typo fixes in documentation

I had to add a "eslint-disable-next-line import/no-deprecated" to the import of createTreeContainer as there are now two such functions, whose only difference is the type of an optional parameter (and one of these functions is deprecated). Don't know how i can trick typescript to import the not-deprecated function if they are actually named identically.

I did not implement 'onFocusChange' / 'onWindowChange', instead i kept it simple and always save with a delay (delay from the preferences). We may need to look into this once more.

Greetings,
Alex

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Alex,
thank you very much for your contribution :)
It looks pretty good to me. I'm just wondering whether we could provide a (simple) default implementation for createSnapshot and revert either in the BaseTreeEditorWidget or the ResourceTreeEditorWidget. Do you have an idea what would be needed for this?

@alxflam
Copy link
Contributor Author

alxflam commented Apr 5, 2022

@lucas-koehler thanks for your feedback, i'll try to summarize my observations:

  • createSnapshot is called when 'Save As' is triggered: the snapshot returned is then usually simply the editor content (for plain monaco editors simply a string) and is then applied to the new file using applySnapshot (see copyAndSave in WorkspaceFrontendContribution)
  • revert is called upon closing dirty editors, if the user chooses to discard the changes

Now BaseTreeEditorWidget only holds the current state in the instanceData field, but it cannot load the currently persisted state again (would be required for revert). But createSnapshot could simply return the current instanceData. The same is true for NavigatableTreeEditorWidget.

ResourceTreeEditorWidget instead could implement revert by calling load to restore the currently persisted state of the resource and in createSnapshot return this.data.

A few remarks:

  • Somehow instanceData of BaseTreeEditorWidget is not used consistently: While the coffee editor example sets the member (extending from NavigatableTreeEditorWidget), the ResourceTreeEditorWidget instead uses a custom data member.
  • When using a model server, the usual implementation of revert would be to call undo on the ModelServerClient - but there's no base class yet for model sever trees, hence no way to provide this in a generic way

For now, i have added a default implementation for revert to ResourceTreeEditorWidget and createSnapshot for BaseTreeEditorWidget. I think the revert for now needs to be handled by the implementing subclass (if it's not a ResourceTreeEditorWidget). Additionally, ResourceTreeEditorWidget now also reuses the parents instanceData member (else i could not reuse the parents createSnapshot implementation).

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update and the explanations! I guess the separation between data in the resource editor and instanceData in the base editor was a mistake or a leftover from past refactorings. Thanks for fixing that.

When using "save as" in the example, the save seems to work but I get an error message in the UI:
image
Quickly looking for this message in the Theia code, the reason seems to be a missing applySnapshot method. (see copyAndSave in workspace-frontend-contribution)

theia-tree-editor/src/browser/tree-editor-widget.tsx Outdated Show resolved Hide resolved
- provide implementations of createSnapshot and revert where possible
- adapt autoSave due to changed type and preference key
- typo fixes in documentation

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>
@alxflam
Copy link
Contributor Author

alxflam commented Apr 6, 2022

@lucas-koehler thanks for testing, i missed out checking the examples. Now the example should work. I've implemented applySnapshot for the ResourceTreeEditorWidget (notice that setTreeData needs to be called to ensure the view for the newly saved file is also updated and shows the applied state). The object type used for createSnapshot and applySnapshotis the same one the MonacoEditorModel uses (as we need some kind of object to wrap the primitive string).

@lucas-koehler
Copy link
Contributor

@alxflam Thanks for the update and the contribution in general :). The error message no longer appears :)
The CI currently fails but that does not seem to be related to your changes (it cannot download a dependency). I'll rerun it again in a few hours to see if it works better again :D

@lucas-koehler lucas-koehler merged commit 4b9e870 into eclipse-emfcloud:master Apr 7, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants