-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add keyboard support for moving and zooming elements and viewport #241
Add keyboard support for moving and zooming elements and viewport #241
Conversation
Hi @aylin-sarioglu, thank you very much for your contribution. I'll look at it at the beginning of next week. |
0a6fc5d
to
06e66ae
Compare
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.
Thank you very much for this great contribution @aylin-sarioglu! ✨
In general it looks good to me, I added some comments inline, mainly addressing naming and some minor restructurings.
And there are two subjects I would like to discuss:
- I noticed that the key listeners do not support any sort of "key stroke debounce" atm. As soon as I start zooming/moving very fast (e.g. hitting
+
very quickly and frequently without a significant pause) the animation (i.e. the execution of move/resize) slows down significantly. Could you please have a quick look if there is some way to debounce the execution of thekeyDown
event? - Regarding the key listeners in general: I tested this feature also with the Theia integration and it works great in this context as well. However, it would be awesome, if we could continue the approach of separating the key listener and the actual executor by wrapping the current
keyDown
behaviour in a separate Action.
This would have the benefit, that we can then simply re-register via theGLSPDiagramKeybindingContext
for the theia-integration. This way, the bindings appear in the keymap too, which allows the user to browse all available shortcuts as well as edit them to their liking (although I agree with you here, the shortcuts for moving and zooming are already the perfect fit 😄).
For example, theMarkerNavigatorKeyListener
only triggers an action, which is handled by a separate action handlerNavigateToMarkerActionHandler
. For the standalone example, the marker navigation is register to the keybindings Ctrl+./Ctrl+,. For the theia-integration it is then registered via a keybinding contribution under the previously mentioned diagram context. The following screenshot shows that keybinding in Theia and that it's linked to the diagram context:
As an example you can test the differences between the standalone and the theia-integration if you additionally register themarkerNavigatorContextMenuModule
for the standalone example. Then you can observe, that the marker navigation works in the standalone via Ctrl+./Ctrl+, and in the theia-integration via F8/Shift+F8. (First you have to validate the model via the checkmark tool in the toolbox, this creates issue marker on some nodes. You can then navgiate between those markers.)
Thanks again Aylin for your great contribution!
PS: I'm sure you already noticed, but the current theia-integration example does not offer the keyboards shortcut overview by default. You can activate this by adding the @theia/keymaps
dependency to the example browser-app
.
packages/client/src/features/tools/viewport/movement-key-tool.ts
Outdated
Show resolved
Hide resolved
packages/client/src/features/tools/viewport/movement-key-tool.ts
Outdated
Show resolved
Hide resolved
packages/client/src/features/tools/viewport/movement-key-tool.ts
Outdated
Show resolved
Hide resolved
packages/client/src/features/tools/viewport/movement-key-tool.ts
Outdated
Show resolved
Hide resolved
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.
Hi @aylin-sarioglu, thanks for the great improvements ✨!
Overall it looks good to me, I added a few minor comments inline.
Once they are resolved, I guess we are close to merging this pr 👍
Please also squash your commits, thanks!
And one thing, as I am looking again at the code, perhaps it would be a good idea to nest the move-zoom
and view-key-tools
features inside the new accessibility
folder now as well-
This would group all the features in the accessibility context in one directory, which would be useful for later maintenance after all.
84bd031
to
2b2d30e
Compare
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.
Thank you @aylin-sarioglu for the quick updates! ✨
Looks good to me, I just have one minor comment left.
If that's fixed, we are good to go from my POV :)
packages/client/src/features/accessibility/move-zoom/move-handler.ts
Outdated
Show resolved
Hide resolved
…board - Introduce tool and handler for moving viewport and elements via keyboard shortcut - Introduce tool and handler for zooming viewport and elements via keyboard shortcut - Provide tool for an improved implementation for deselecting elements
ace8bd5
to
a6dbb48
Compare
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.
Thank you @aylin-sarioglu for the quick updates and the reporting of the bug! 👍
…he keyboard (#241) - Introduce tool and handler for moving viewport and elements via keyboard shortcut - Introduce tool and handler for zooming viewport and elements via keyboard shortcut - Provide tool for an improved implementation for deselecting elements Part of eclipse-glsp/glsp#1029
This PR is part of my diploma research conducted at the Model Engineering lab of TU Wien in the Business Informatics Group. It introduces the move and zoom functionality.
Examples
Move elements
Move viewport
Zoom elements
+
or-
to zoom in/out into the direction of the element or elementsCTRL + 0
to reset to default zoom levelZoom viewport
+
or-
to zoom in/out in viewportCTRL + 0
to reset to default zoom level@martin-fleck-at, @ndoschek
Part of eclipse-glsp/glsp#1029