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

GLSP-1357: Update GLSPSaveable for Theia 1.50.x #220

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Jul 4, 2024

What it does

With Theia 1.50 the Saveable implementation was changed in breaking fashion. In particular the Saveable itself is no longer responsible of handling autosave behavior. This change updates the GLSPSaveable implementation to be compatible with Theia > 1.50.x while maintaining the old saveable behavior for Theia < 1.50

  • Introduce utility functions to determine the currently used Theia version and validate whether the version satisfies a given range

  • Update GLSPSaveable implementation to support the onContentChanged event required for Theia 1.50.x. The old autosave behavior is kept and only activated if a Theia version < 1.50.0 is used.

  • Update READMEs

  • Introduce a utility script to change the Theia version used in the example applications

  • Add CI workflows that run periodicaly (once a week)

    • Multi OS/Node Builds against all OS variants in combination with node 18 and 20
    • Theia Compatibility Builds the current master/examples again the minimum/CR/latest Theia version

Fixes eclipse-glsp/glsp#1357

How to test

Build and start the example. Verify that autosaving in the diagram still works as expected.
Switch the example applications to Theia 1.50.1 (yarn change:theia-version 1.50.1)
Rebuild and retest

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

With Theia 1.50 the `Saveable` implementation was changed in breaking fashion.
In particular the `Saveable` itself is no longer responsible of handling autosave behavior. This change updates the `GLSPSaveable` implementation to be compatible with Theia > 1.50.x while maintaining the old saveable behavior for Theia < 1.50

- Introduce utility functions to determine the currently used Theia version and validate whether the version satisfies a given range
- Update `GLSPSaveable` implementation to support the `onContentChanged` event required for Theia 1.50.x.  The old autosave behavior is kept and only activated if a Theia version < 1.50.0 is used.
- Update READMEs

- Add CI workflows that run periodicaly (once a week)
  - `Multi OS/Node` Builds against all OS variants in combination with node 18 and 20
 - `Theia Compatibility` Builds the current master/examples again the minimum/CR/latest Theia version

Fixes eclipse-glsp/glsp#1357
@tortmayr
Copy link
Contributor Author

tortmayr commented Jul 4, 2024

Sidenote: The Theia compat workflow as a PR trigger for now, just to verify that the workflow works as expected.
This PR trigger will be removed before merging.

@tortmayr tortmayr requested a review from martin-fleck-at July 4, 2024 08:00
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Changes look good to me in theory but I do have some comments regarding some methods and I also noticed something that looks like a bug: Open Diagram > Make Changes > Close Diagram > Say you want to save changes instead of losing them > Diagram stays open whereas it should be closed. It seems like only a non-dirty diagram can be closed, not sure if this was always the case.

@tortmayr
Copy link
Contributor Author

tortmayr commented Jul 4, 2024

Changes look good to me in theory but I do have some comments regarding some methods and I also noticed something that looks like a bug: Open Diagram > Make Changes > Close Diagram > Say you want to save changes instead of losing them > Diagram stays open whereas it should be closed. It seems like only a non-dirty diagram can be closed, not sure if this was always the case.

I think this was always the case, but I found a fix for it anyways.
The problem was that Theia operates on the assumption that the dirty state flag is properly updated immediatly after the save operation completes. After invoking save, the SaveableService then checks wether it is save to close the editor i.e. the dirty flag is false. In GLSP the we a client-server roundtrip for save which means the dirty state flag is updated in async fashion.
I have updated the save implementation to return a promise that waits until the client-server save round trip is completed and the dirty state flag has been updated.

@tortmayr tortmayr requested a review from martin-fleck-at July 4, 2024 12:40
Copy link
Contributor

@martin-fleck-at martin-fleck-at 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 quick turnaround, everything looks good to me now!

@tortmayr tortmayr merged commit 1d1bcda into master Jul 4, 2024
7 checks passed
tortmayr added a commit that referenced this pull request Jul 5, 2024
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.

Synchronize Saveable change in theia 1.50.1
2 participants