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

GH-1377: Extract move and zoom features from accessibility to own module #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haydar-metin
Copy link
Contributor

@haydar-metin haydar-metin commented Nov 19, 2024

What it does

Works on eclipse-glsp/glsp#1377

How to test

Use the arrow keys to move the viewport / elements.
Use +/- to zoom in and out.

Follow-ups

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)

The move and zoom features have been moved to their own modules. They are now enabled by default.

@haydar-metin haydar-metin self-assigned this Nov 19, 2024
@haydar-metin
Copy link
Contributor Author

@tortmayr after some thought, do we even require tools for moving/zooming? Maybe moveElement would require it, but moving the viewport and zooming shouldn't need a tool or?

@haydar-metin haydar-metin marked this pull request as draft November 19, 2024 14:17
@tortmayr
Copy link
Contributor

tortmayr commented Nov 19, 2024

@haydar-metin I agree. Currently zooming/scrolling via mouse is provided by the viewportModule. This module bind the mouse listeners directly instead of using tools. This means that those listeners are always on (independent of the activation state of the tool manager). Listeners that provide the same functionality via keyboard should probably also be bound directly.
Otherwise we end up in an inconsistent state. (e.g. during edge creation zooming via mouse would be possible, but zooming via keyboard not (because default tools are disabled at that point).

So we should not use tools for zooming/moving.
For the move-tool its a different story, here a tool definitely make sense but maybe we should integrate this into the change bounds tool instead of having a separate tool just for keyboard movement

Thinking about it maybe it even makes sense to integrate the keyboard functionality for zooming/moving/scrolling directly into the corresponding default module (viewportModule, changeBoundsToolModule) instead of having extra a11y modules for them.
WDYT?

@eclipse-glsp eclipse-glsp deleted a comment from haydar Nov 20, 2024
@haydar-metin haydar-metin marked this pull request as ready for review November 20, 2024 14:54
@haydar-metin
Copy link
Contributor Author

@tortmayr sounds good to me! I did the following:

  • MoveElementHandler (ActionHandler) moved to bounds, as we don't have a changeBounds container module.
  • MoveElementKeyListener moved to change-bounds-tool
  • MoveViewport*, Zoom** moved to viewport

@tortmayr
Copy link
Contributor

tortmayr commented Dec 6, 2024

@haydar-metin Thanks for providing this change.
If its ok for you I will take a look at this PR after the 2.3 release is completed (planned until the end of next week).
Since this PR only partially addresses eclipse-glsp/glsp#1377 and also introduces some changes to the protocol API we should probably not include it in the current release.
We can then use the next release cycle to further work on eclipse-glsp/glsp#1377 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants