-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add property map in model state to store values in between handler calls #132
Conversation
Also removes superfluous import in layout operation handler Fixes eclipse-glsp/glsp#401
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.
Change looks good to me in general and I like the idea of the generic map in the state for simple custom use cases. However, I have a few minor remarks which you maybe want to consider. Thank you Philip!
@@ -34,6 +36,7 @@ | |||
protected String clientId; | |||
|
|||
protected Map<String, String> options; | |||
protected final Map<String, Object> memento = new HashMap<>(); |
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.
I am a bit worried about the name memento
since it kind of implies a 'store state - restore state' functionality. From what I understand this is necessary since we only have the model state as persistence for handlers so this is really for custom data. Maybe 'properties', 'data', 'userData', 'appData', 'sessionData' or something similar would be a good alternative? But I leave that up to you.
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.
You're right. I used memento because I was only thinking about stateless handlers and for them it is sort-of a memento.
I think I'll go with properties
.
} | ||
|
||
@Override | ||
public Optional<Object> getMemento(final String key) { |
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.
Not something that needs to be done immediately but in my experience 'Object' is almost never really useful and the second step after calling this method would be to cast the Object to something you can actually use. Therefore I believe it would be great to already provide that casting step as part of an additional method Optional<T> getMemento(String key, Class<T> type)
or something similar to ease the usage of the user storage - maybe a similar method can be provided for clearing.
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.
Nice, sure that makes the API more convenient.
- Move classes from root package into a matching subpackage - Create "org.eclipse.glsp.server.features.core.model" package that contains all actions and related classes that are needed for model loading - Create a dedicated feature subpackage for undo/redo Resolves eclipse-glsp/glsp/issues/132
…lls (eclipse-glsp#132) Also removes superfluous import in layout operation handler Fixes eclipse-glsp/glsp#401
Also removes superfluous import in layout operation handler
Fixes eclipse-glsp/glsp#401