Add marquee tool #108

sgraband · 2021-03-18T12:01:49Z

Add marquee tool to select multiple elements at once. Selecting with the marquee tool is done by clicking and dragging the mouse while the shift key is pressed.

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

planger · 2021-03-18T14:24:13Z

Just one comment from the top of my head, I think it'd be cool to also add a tool to the palette to enable the marquee tool directly. Can be done in a follow-up PR though.

tortmayr

Thanks! In general the tool works well and is a great contribution to glsp.
I have a couple of inline comments and found one general issue:

The current implementation is specific to the workflow-example and cannot be reused generically.
We should move the model configuration and the marquee view definition into the glsp-client package. It is probably a good idea to create a dedicated marqueeToolsModule that handles both the tool registration/binding and the configuration of the marqueeView.

tortmayr · 2021-03-19T11:03:48Z

examples/workflow-glsp/src/model.ts

@@ -58,6 +59,13 @@ export class TaskNode extends RectangularNode implements Nameable, WithEditableL
    }
 }

+export class MarqueeNode extends RectangularNode {
+    static readonly DEFAULT_FEATURES = [connectableFeature, deletableFeature, selectFeature, boundsFeature,


I think we should remove all unneeded features here to avoid potential unintended side effects.
IMO it should be enough to only activate the boundsFeature

tortmayr · 2021-03-19T11:05:35Z

examples/workflow-glsp/src/workflow-views.tsx

@@ -65,6 +65,21 @@ export class ForkOrJoinNodeView extends RectangularNodeView {
    }
 }

+@injectable()


The view definition should be part of @eclipse-glsp/client to enable generic reuse.

tortmayr · 2021-03-19T11:07:47Z

packages/client/src/features/tools/di.config.ts


 const toolsModule = new ContainerModule((bind, _unbind, isBound) => {
    // Register default tools
    bind(GLSP_TYPES.IDefaultTool).to(ChangeBoundsTool);
    bind(GLSP_TYPES.IDefaultTool).to(EdgeEditTool);
    bind(GLSP_TYPES.IDefaultTool).to(DelKeyDeleteTool);
+    bind(GLSP_TYPES.IDefaultTool).to(MarqueeTool);


It probably makes sense to introduce a dedicated 'marqueeToolModule'
This module than binds the corresponding tools, and configures the modelelement/view.

So that the marquee tool is no longer handled by 'tools' feature and is completly handled in a newly created 'marquee-tool' feature?
The marqueeToolModule would then look like this:

const glspMarqueeToolModule = new ContainerModule((bind, unbind, isBound, rebind) => { const context = { bind, unbind, isBound, rebind }; configureModelElement(context, 'marquee', MarqueeNode, MarqueeView); bind(GLSP_TYPES.IDefaultTool).to(MarqueeTool); bind(GLSP_TYPES.ITool).to(MarqueeMouseTool); });

Correct, however on second thought it may be better to just create a utility function configureMarqueeTool which is then called inside of the tools module

const toolsModule = new ContainerModule((bind, _unbind, isBound) => { //... const context = { bind, isBound}; configureMarqueeTool(context); } export function configureMarqueeTool((context: { bind: interfaces.Bind, isBound: interfaces.IsBound }){ configureModelElement(context, 'marquee', MarqueeNode, MarqueeView); bind(GLSP_TYPES.IDefaultTool).to(MarqueeTool); bind(GLSP_TYPES.ITool).to(MarqueeMouseTool); };

Would you then expect the MarqueeNode, MarqueeView, MarqueeMouseTool and MarqueeTool to be in the tools feature or in the separate marquee-tool, even though they will be bound in the tools feature?

I would omit the separate marque-tool for now and keep MarqueeNode, MarqueeView, MarqueeMouseTool and MarqueeTool in the tools feature but extract the actual binding process into a utility function. This way it can be easily reused and/or removed in subclasses.

tortmayr · 2021-03-19T11:11:14Z

packages/client/src/features/tools/marquee-mouse-tool.ts

+export class MarqueeMouseTool extends BaseGLSPTool {
+    static ID = "glsp.marquee-mouse-tool";
+
+    @inject(GLSP_TYPES.MouseTool) protected mouseTool: IMouseTool;


The mouseTool,keyTool and feedbackActionDispatcher are already injected in the super class ('BaseGLSPTool'). No need to inject them here again

tortmayr · 2021-03-19T11:11:58Z

packages/client/src/features/tools/marquee-mouse-tool.ts

+        this.marqueeMouseListener = new MarqueeMouseListener(this.domHelper);
+        this.mouseTool.register(this.marqueeMouseListener);
+        this.keyTool.register(this.shiftKeyListener);
+        this.feedbackDispatcher.registerFeedback(this, [cursorFeedbackAction(CursorCSS.MARQUEE)]);


You can use the dispatchFeedback() method from the super class for this

tortmayr · 2021-03-19T11:12:36Z

packages/client/src/features/tools/marquee-mouse-tool.ts

+    disable(): void {
+        this.mouseTool.deregister(this.marqueeMouseListener);
+        this.keyTool.deregister(this.shiftKeyListener);
+        this.feedbackDispatcher.registerFeedback(this, [cursorFeedbackAction()]);


You can use the `deregisterFeedback()' method from the superclass for this

tortmayr · 2021-03-19T11:15:44Z

packages/client/src/features/tools/marquee-tool.ts

+export class MarqueeTool extends BaseGLSPTool {
+    static ID = "glsp.marquee-tool";
+
+    protected selectionKeyListener: MarqueeKeyListener = new MarqueeKeyListener();


Should probably be named 'marqueeKeyListener' instead of 'selectionKeyListener'

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

tortmayr

Thanks a lot. Change looks good to me now, I only have one minor suggestion

tortmayr · 2021-03-23T11:18:28Z

packages/client/src/features/tools/di.config.ts

    bind(NodeCreationTool).toSelf().inSingletonScope();
    bind(EdgeCreationTool).toSelf().inSingletonScope();
    bind(GLSP_TYPES.ITool).toService(EdgeCreationTool);
    bind(GLSP_TYPES.ITool).toService(NodeCreationTool);

+    configureMarqueeTool({ bind, isBound });
+    configureActionHandler({ bind, isBound }, EnableToolsAction.KIND, GLSPScrollMouseListener);


configureActionHandler() should be moved into the glspViewportModule

tortmayr · 2021-03-23T11:19:17Z

packages/client/src/features/tools/di.config.ts

    bind(NodeCreationTool).toSelf().inSingletonScope();
    bind(EdgeCreationTool).toSelf().inSingletonScope();
    bind(GLSP_TYPES.ITool).toService(EdgeCreationTool);
    bind(GLSP_TYPES.ITool).toService(NodeCreationTool);

+    configureMarqueeTool({ bind, isBound });
+    configureActionHandler({ bind, isBound }, EnableToolsAction.KIND, GLSPScrollMouseListener);
+    configureActionHandler({ bind, isBound }, EnableDefaultToolsAction.KIND, GLSPScrollMouseListener);


configureActionHandler() should be moved into the glspViewportModule

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

tortmayr

LGTM 👍

This reverts commit 012a1a3.

sgraband added 2 commits March 16, 2021 15:38

Add marquee tool

89c1889

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

Add support for selecting edges via marquee-tool

98fbf6b

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

tortmayr self-requested a review March 18, 2021 12:50

tortmayr requested changes Mar 19, 2021

View reviewed changes

Add marquee tool to palette + refactoring

f8e89cf

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

sgraband requested a review from tortmayr March 22, 2021 13:26

Fix node detection

6521aab

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

tortmayr requested changes Mar 23, 2021

View reviewed changes

Move configureActionHanlder() to glspViewportModule

9f1b026

Signed-off-by: Simon Graband <sgraband@eclipsesource.com>

tortmayr self-requested a review March 23, 2021 11:36

tortmayr approved these changes Mar 23, 2021

View reviewed changes

tortmayr merged commit 012a1a3 into eclipse-glsp:master Mar 23, 2021

tortmayr added a commit that referenced this pull request Mar 23, 2021

Revert "Add marquee tool (#108)"

a0c3cde

This reverts commit 012a1a3.

tortmayr mentioned this pull request Mar 23, 2021

Implement marquee selection tool eclipse-glsp/glsp#199

Closed

tortmayr added the changelog label Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add marquee tool #108

Add marquee tool #108

sgraband commented Mar 18, 2021

planger commented Mar 18, 2021

tortmayr left a comment

tortmayr Mar 19, 2021

tortmayr Mar 19, 2021

tortmayr Mar 19, 2021

sgraband Mar 19, 2021 •

edited

Loading

tortmayr Mar 21, 2021 •

edited

Loading

sgraband Mar 22, 2021

tortmayr Mar 22, 2021

tortmayr Mar 19, 2021

tortmayr Mar 19, 2021

tortmayr Mar 19, 2021

tortmayr Mar 19, 2021

tortmayr left a comment

tortmayr Mar 23, 2021 •

edited

Loading

tortmayr Mar 23, 2021 •

edited

Loading

tortmayr left a comment

Add marquee tool #108

Add marquee tool #108

Conversation

sgraband commented Mar 18, 2021

planger commented Mar 18, 2021

tortmayr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgraband Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

tortmayr Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tortmayr left a comment

Choose a reason for hiding this comment

tortmayr Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

tortmayr Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

tortmayr left a comment

Choose a reason for hiding this comment

sgraband Mar 19, 2021 •

edited

Loading

tortmayr Mar 21, 2021 •

edited

Loading

tortmayr Mar 23, 2021 •

edited

Loading

tortmayr Mar 23, 2021 •

edited

Loading