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

Introducing element navigation via keyboard #276

Merged
merged 1 commit into from Aug 25, 2023

Conversation

aylin-sarioglu
Copy link
Contributor

This PR is part of my diploma research conducted at the Model Engineering lab of TU Wien in the Business Informatics Group. It introduces two navigation mechanisms for iterating through the given model.
Additionally, this PR also includes a Focus Tracker and Notifications.

The Focus Tracker displays the current focused element with a textual indicator on the bottom left corner of the canvas. The Notifications are used to provide user information, e.g. activated resize or navigation modes, displayed on the canvas. Both will be triggered automatically if applicable.

Example

Default Navigation (following directions of relations)

  1. Select an element as starting point.
  2. Use the shortcut N to activate default navigation.
  3. Use arrow keys to iterate through the model according to the directions of the given relations.

Position-based Navigation (following x and y coordinates)

  1. Select an element as starting point.
  2. Use the shortcut ALT+N to activate position-based navigation.
  3. Use arrow keys to iterate through the model according to the positions of the elements, i.e. depending on the order of the elements' x and y coordinates.

@ndoschek @martin-fleck-at

Part of eclipse-glsp/glsp#1029

Copy link
Contributor

@ndoschek ndoschek left a 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 this contribution! 🚀
I didn't have a thorough look at the code yet, but I noticed some overall issues, which I summarized below. Could you have a quick look and let me know what you think? Thank you!

  • Is there a reason why the navigation modes can only be activated if an element is selected? I find this a little confusing, because I have to use the mouse to select an element and then to switch to the keyboard if I prefer that. Also, if selection is lost and navigation mode is on it cannot be restored without the mouse. Also the navigation mode cannot be exited if selection is lost. Could you have another look a t this? Also, could you think of a way to jump to the very first element for example in this case and initially?
  • I would be in favor of introducing visible grouping in the shortcut overview now, as we have multiple overlapping keybindings in different contexts. Either margins between the groups, adding the group name as line or something like that.
  • The Help menu cannot be opened if navigation mode is active.
  • Focus tracker does not reset focus if a creation tool was selected in the toolpalette and then one of the tools in the header is activated (which aborts the creation tool):
    image

Another general comment:

@aylin-sarioglu aylin-sarioglu force-pushed the navigation branch 3 times, most recently from c674004 to f32815b Compare August 23, 2023 18:38
@aylin-sarioglu
Copy link
Contributor Author

Hi @ndoschek, Thank you for your feedback.

  1. Is there a reason why the navigation modes can only be activated if an element is selected? I find this a little confusing because I have to use the mouse to select an element and then switch to the keyboard if I prefer that.

Without selecting elements, activating the navigation wouldn't make sense, as its purpose is to move through those selected diagram elements.
I fixed the code a little bit, as now it is possible to focus the diagram with TAB and then the search can be called via CTRL+F so there is no need to use the mouse to select an element. The only issue here is, that it can happen that while iterating via TAB the focus can jump between the tool palette entries and the diagram. The reason for that is that the tool palette entries each have an ascending tab index, therefore the focus is jumping to different elements. The tool palette entries should have the same index.

  1. Also, if selection is lost and navigation mode is on it cannot be restored without the mouse.

As explained in 1.) , now you can focus on the diagram via TAB and then call the search, without the need for a mouse.

  1. Also, the navigation mode cannot be exited if selection is lost. Could you have another look at this?

This is fixed now.

  1. Also, could you think of a way to jump to the very first element for example in this case and initially?

This was intentionally not added for this prototype, as there is no clear definition of what the first element of a diagram is, i.e. it could be the one positioned at the very top or left of the canvas, as this depends on the positioning of the user. But, by focusing on the diagram (which is now provided with the correction explained in 1.) and calling the search, the desired element could be selected.

Copy link
Contributor

@ndoschek ndoschek left a 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 @aylin-sarioglu for the fast update on this PR!
I added a few comments inline, but nothing major from my POV.

And I noticed one issue regarding the highlighting of navigable elements.
If you exit the navigation mode if navigable elements are highlighted, the CSS class is not reset after exiting the navigation mode (please see screenshot). Could you please doublecheck this? Thanks!

image

-  Introduce tool and handler for element navigation via keyboard shortcut
-  Introduce tool and handler for tracking the current focus of the cursor
-  Introduce tool and handler for the creation of notification/toast
-  Introduce new reposition action for focusing on element without zooming in
Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

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

Thanks @aylin-sarioglu LGTM 👍

@ndoschek ndoschek merged commit 976795c into eclipse-glsp:master Aug 25, 2023
6 checks passed
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.

None yet

3 participants