#197 #196 Improve dirty state handling #101
Conversation
The `SetDirtyStateAction` now also declares the action/operation which caused the dirty state change. Ensure that the `SetDirtyStateAction` is only sent once during a model update Part of eclipse-glsp/glsp#197 Fixes eclipse-glsp/glsp#196
|
|
||
| public SetDirtyStateAction() { | ||
| super(ID); | ||
| } | ||
|
|
||
| public SetDirtyStateAction(final boolean isDirty) { | ||
| public SetDirtyStateAction(final boolean isDirty, final Action causedBy) { |
There was a problem hiding this comment.
Do we have to make this a mandatory parameter? Afaik, VSCode just wants to know that for undo actions, right? I'm wondering whether this justifies making this a mandatory parameter for all other invocations caused by anything else than UndoAction. Also do you think it would be simpler to just have a string argument: reason = 'operation' | 'undo' | 'save'? I don't have a hard opinion on that, but enforcing to provide an actual action seems like a strong requirement for this rather specific use case of VSCode.
Minor knitpick, I think as a name I'd prefer reason rather than causedBy.
There was a problem hiding this comment.
+1 for 'reason' and making it optional. In any case it should be possible to determine wehter the dirty state change was triggered by an actual new edtit/operation, an undo/redo action or a save action. Now thinking about it, we also have a another case: the server could also update the model without an operation invocation (e.g. by receiving an update from the model server).
So maybe we should type reason as follows: "operation"|"undo"|"redo"|"save"|"externalUpdate".
What do you think?
There was a problem hiding this comment.
👍 Yes, sounds good! Maybe just external is good enough, e.g. also if the file got updated on the file system or whatever other external reason. Thanks!
There was a problem hiding this comment.
Maybe it should be just String then and adopters can provide any reason they want and we just have a list of known String reasons but no strict typing?
There was a problem hiding this comment.
Fine with me as well. We could then just use the action.kind as default reason string as well
| if (diagramConfiguration.getLayoutKind() == ServerLayoutKind.AUTOMATIC) { | ||
| layoutEngine.layout(modelState); | ||
| } | ||
| Action modelAction = gModel.getRevision() == 0 ? new SetModelAction(gModel) : new UpdateModelAction(gModel); |
There was a problem hiding this comment.
I already had cases where I had to override this method only to change the animate flag in this method. Would it be possible to extract the new UpdateModelAction(gModel) into a separate method or provide a method that returns the animate boolean that is then used in the creation of this action? I know it is not strictly part of this change but with this change the method just became more complicated and overriding more cumbersome ;-)
There was a problem hiding this comment.
I'm wondering if this is something that we could store in the DiagramConfiguration (similar to the needsClientLayout flag). This way the user doesn't have to subclass the ModelSubmissionHandler and can configure it directly in the DiagramConfiguration.
| return Arrays.asList(action, new SetDirtyStateAction(modelState.isDirty())); | ||
| List<Action> result = new ArrayList<>(); | ||
| result.add(modelAction); | ||
| Optional.ofNullable(causedBy).ifPresent(result::add); |
There was a problem hiding this comment.
I'm not sure I understand why we would need to add the 'cause' to the result? The method comment mentions that the result of this call should be dispatched through the ActionDispatcher (either directly or not), would that lead not to an infinite loop in case we do not need a client layout or do I miss something here?
| } | ||
| } | ||
| return submitModelDirectly(modelState); | ||
| return submitModelDirectly(modelState, needsClientLayout ? null : causedBy); |
There was a problem hiding this comment.
It seems like it should not be possible for needsClientLayout to be true here. If it were, we would already have returned.
...ipse.glsp.server/src/org/eclipse/glsp/server/features/core/model/ModelSubmissionHandler.java
Outdated
Show resolved
Hide resolved
…atures/core/model/ModelSubmissionHandler.java Co-authored-by: Martin Fleck <[email protected]>
…on (eclipse-glsp#72) Log a warning instead of throwing an exception if no elementIds are specified for an DeleteElementAction. Throwing an exception here will break the command stack. In addition, it's not necessary because technically an empty DeleteElementOperation is still valid (and simply does not affect the model at all) Fixes eclipse-glsp/glsp/issues/101
…ipse-glsp#101) The `SetDirtyStateAction` now also declares the reason which caused the dirty state change. Ensure that the `SetDirtyStateAction` is only sent once during a model update Part of eclipse-glsp/glsp#197 Fixes eclipse-glsp/glsp#196
The
SetDirtyStateActionnow also declares the action/operation which caused the dirty state change.Ensure that the
SetDirtyStateActionis only sent once during a model updateEnsure that layout engine is invoked after bounds have been computed (AutomaticServerLayout case)
Part of eclipse-glsp/glsp#197
Fixes eclipse-glsp/glsp#196