-
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
Collab advanced #241
base: master
Are you sure you want to change the base?
Collab advanced #241
Conversation
reload logic for RequestModelAction replace commandStack with commandStackManager
|
||
package org.eclipse.glsp.server.utils; | ||
|
||
public class CollaborationUtil { |
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.
Minor checkstyle issue:
Since this is a utility class it should have a private constructor:
public final class CollaborationUtil {
private CollaborationUtil() {
}
public static final String FALLBACK_SUBCLIENT_ID = "FALLBACK_SUBCLIENT_ID";
}
@@ -0,0 +1,11 @@ | |||
package org.eclipse.glsp.server.internal.gmodel.commandstack; |
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.
Missing copyright header
@@ -0,0 +1,7 @@ | |||
package org.eclipse.glsp.server.command; |
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.
Missing copyright header
@@ -0,0 +1,13 @@ | |||
package org.eclipse.glsp.server.command; |
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.
Missing copyright header
@@ -0,0 +1,53 @@ | |||
package org.eclipse.glsp.server.command; |
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.
Missing copyright header
@@ -62,9 +67,9 @@ public void init() { | |||
} | |||
|
|||
@Override | |||
public void setEditingDomain(final EditingDomain editingDomain) { | |||
public void setEditingDomain(final EditingDomain editingDomain, final String subclientId) { |
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.
For backwards compatibility we should add a deprecated override for setEditingDomain(final EditingDomain editingDomain)
(see comment in getOrCreateEditingDomain)
@@ -20,7 +20,7 @@ | |||
import org.eclipse.glsp.server.model.GModelState; | |||
|
|||
public interface EMFModelState extends GModelState { | |||
void setEditingDomain(EditingDomain editingDomain); | |||
void setEditingDomain(EditingDomain editingDomain, String subclientId); | |||
|
|||
EditingDomain getEditingDomain(); |
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.
Seems a bit weird to me that setEditingDomain
needs to be aware of the subclientId but getEditingDomain
does not. Why is that?
@@ -88,7 +88,7 @@ public List<Action> submitInitialModel(final RequestModelAction requestAction) { | |||
* Therefore we temporarily store the action later retrival | |||
*/ | |||
this.requestModelAction = Optional.of(requestAction); | |||
return submitModel(); | |||
return submitModel(requestAction.getSubclientId()); |
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.
Seems like an accidental change to me. The subClientId should not be used as reason here
@@ -59,7 +60,10 @@ public void executeOperation(final CreateNodeOperation operation) { | |||
.map(location -> LayoutUtil.getRelativeLocation(location, container)); | |||
GModelElement element = createNode(relativeLocation, operation.getArgs()); | |||
container.getChildren().add(element); | |||
actionDispatcher.dispatchAfterNextUpdate(new SelectAction(), new SelectAction(List.of(element.getId()))); | |||
actionDispatcher.dispatchAfterNextUpdate( |
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.
Having to manually add the Subclient id to actions that should be dispatched after the next update is not ideal.
Wouldn't it be better to rework the dispatchAfterNextUpdate
mechanism in the action dispatcher directly?
We should store rework the postUpdateQueue to a Map<string,List>. Whenever the dispatchAfternextUpdate
method is invoked we then store the action in the map entry for the corresponding subclient id of the action.
When we clear the postUpdateQueue (dispatchPostUpdateQueue
) we should only remove the actions that have the same subclientid as the corresponding updateModelAction.
What it does
How to test
Follow-ups
Changelog