Skip to content
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

GLSP-1423 Add MovementBehavior to control invalid multi element movements #397

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

ivy-lli
Copy link
Contributor

@ivy-lli ivy-lli commented Nov 20, 2024

What it does

Fixes: eclipse-glsp/glsp#1423

Per default change the behavior that all selected elements will be reset to it's origin location when one move is invalid. But add an IMovementBehavior type where this can be changed to the old style.

Question:

  • Should also the red border feedback be on both elements?
  • Better name for the options type / flag on the type?

How to test

Add to the examples/workflow-standalone/src/di.config.ts

export default function createContainer(options: IDiagramOptions): Container {
  ...
  container.bind(TYPES.IMovementRestrictor).to(NoOverlapMovementRestrictor);
  container.bind(TYPES.IMovementBehavior).toConstantValue({ allElementsNeedToBeValid: true }); //or false for the old behavior
  return container;
}

New behavior:
Screen Recording 2024-11-20 at 16 02 56

Old behavior:
Screen Recording 2024-11-20 at 15 50 45

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this change and everything works as expected, thank you very much, Lukas! I just have a few minor comments and then we can merge this PR in my opinion.

  • Should also the red border feedback be on both elements?
    Good question! I can see arguments for both sides as you may not know which move is the culprit if all elements get the erroneous feedback. So I think leaving it only on the actual invalid move for now is fine.
  • Better name for the options type / flag on the type?
    (see inline comment)

@@ -63,6 +63,10 @@ import {
import { FeedbackMoveMouseListener } from './change-bounds-tool-move-feedback';
import { ChangeBoundsTracker, TrackedElementResize, TrackedResize } from './change-bounds-tracker';

export interface IMovementBehavior {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit on the fence about whether it should be called IMovementBehavior (as we have with the IMarqueeBehavior) or IMovementOptions (as we have with the IHelperLineOptions). I think both are probably fine, just wanted to note that down somewhere. We should probably unify that at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named it indeed after the MarqueeBehavior (as I invented this too 🙈), however, I think I like Options more, so I will call it options.

- Rename to IMovementOptions
- Add JsDoc to `allElementsNeedToBeValid` option
- Variable renamings
@ivy-lli ivy-lli changed the title GH-1423 Add MovementBehavior to control invalid multi element movements GLSP-1423 Add MovementBehavior to control invalid multi element movements Nov 22, 2024
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you very much for the quick turnaround! LGTM!

@martin-fleck-at martin-fleck-at merged commit 352e764 into master Nov 22, 2024
7 checks passed
@martin-fleck-at martin-fleck-at deleted the issue/1423 branch November 22, 2024 08:05
holkerveen pushed a commit to holkerveen/glsp-client that referenced this pull request Dec 21, 2024
+Fix eclipse-glsp#401 Node creation  (eclipse-glsp#402)
+ Resolve eclipse-glsp#398 Refactor TypeHints API (eclipse-glsp#399)
+ Resolve eclipse-glsp#396 RouterKind for FeedbackEdge (eclipse-glsp#397)
+ Reuse Sprotty's request/response system and remove custom solution(eclipse-glsp#404)
+ Add support for context menu and harmonize with command palette (eclipse-glsp#405)
holkerveen pushed a commit to holkerveen/glsp-client that referenced this pull request Dec 21, 2024
+Fix eclipse-glsp#401 Node creation  (eclipse-glsp#402)
+ Resolve eclipse-glsp#398 Refactor TypeHints API (eclipse-glsp#399)
+ Resolve eclipse-glsp#396 RouterKind for FeedbackEdge (eclipse-glsp#397)
+ Reuse Sprotty's request/response system and remove custom solution(eclipse-glsp#404)
+ Add support for context menu and harmonize with command palette (eclipse-glsp#405)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move multiple elements with one invalid move results in stacked elements
3 participants