-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactorings regarding source model loading and saving #154
Conversation
2e4cbf8
to
115f047
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In general the code looks good to me I have a few minor netpicks
and suggestions.
(Also I only did a code review because I haven't had time yet to test the changes)
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/actions/SaveModelActionHandler.java
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/di/DiagramModule.java
Show resolved
Hide resolved
plugins/org.eclipse.glsp.server/src/org/eclipse/glsp/server/di/GModelJsonDiagramModule.java
Show resolved
Hide resolved
...s/org.eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/GModelFactory.java
Show resolved
Hide resolved
...eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/JsonFileGModelStore.java
Show resolved
Hide resolved
...e.glsp.server/src/org/eclipse/glsp/server/features/core/model/RequestModelActionHandler.java
Show resolved
Hide resolved
....eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/SourceModelStorage.java
Show resolved
Hide resolved
....eclipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/SourceModelStorage.java
Outdated
Show resolved
Hide resolved
...eclipse.glsp.server/src/org/eclipse/glsp/server/features/modelsourcewatcher/FileWatcher.java
Show resolved
Hide resolved
....glsp.server/src/org/eclipse/glsp/server/features/modelsourcewatcher/SourceModelWatcher.java
Show resolved
Hide resolved
This is the first breaking change since the 0.9.0 so maybe it's also a good idea to update the changelog |
115f047
to
5c60fed
Compare
5c60fed
to
1faaafb
Compare
With this change, I'd like to consistently use the term source model and avoid the term model source. This is to avoid the overlap with Sprotty's concept of ModelSource and its unclear relation to a source model and GModel. I suggest to avoid Model Source altogether and instead introduce and consistently use "source model" to refer to the underlying model that we read and write. Thus we have in GLSP two types of models: * Source Model * Graph Model (GModel) Thus, I did the following refactorings: * Rename ModelSourceLoader to SourceModelPersistence * Add saving to the SourceModelPersistence interface * Delegate from the SaveModelActionHandler to that implementation * Rename ModelSourceWatcher into SourceModelWatcher * Update javadoc and variable names where fit
1faaafb
to
bcd0337
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
With this change, I'd like to consistently use the term source model and
avoid the term model source. This is to avoid the overlap with Sprotty's
concept of ModelSource and its unclear relation to a source model and
GModel.
I suggest to avoid Model Source altogether and instead introduce and
consistently use "source model" to refer to the underlying model that we
read and write. Thus we have in GLSP two types of models:
Thus, I did the following refactorings:
Fixes eclipse-glsp/glsp#582