archimatetool / archi

Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
MIT License
914 stars 267 forks source link

proof of concept implementation of keyboard shortcuts for palette tools (#985) #994

Closed bkyle closed 3 months ago

Phillipus commented 7 months ago

Hi, thanks for your PR. I'll take a look at it soon. Could you summarise what this does and how?

bkyle commented 7 months ago

The KeyHandler in AbstractDiagramEditor has been changed to set the active tool in the palette based on key presses. Key presses are buffered so that "key chords" can be used instead of being limited to a single keystroke. If there is more than 500ms between keystrokes the buffer is cleared.

Setting the active tool is done by traversing the hierarchy of PaletteEntrys looking for instances of ToolEntry that implement the KeyboardAccessibleToolEntry interface. When found, the key chord for the ToolEntry (see KeyboardAccessibleToolEntry#getKeyChord) is checked against the contents of the buffer. When a complete match is found, the palette's setActiveTool is called with the target ToolEntry.

The rest of the changes support the above in a manner that seemed to be in keeping with the existing design. For example, *UIProvider classes were modified with a getKeyChord method so that it's treated similarly to labels and images for palette entries. There are some cases where a UIProvider didn't exist (or I didn't see one anyway) and the key chord was hard-coded at the editor level.

The key chord for each palette entry (if one exists) is now part of the hint/hover text for the palette entry. e.g. if you hover over a Business Actor the text will say "Business Actor (ba) - ArchiMate Element", the parenthesized text is the key chord. To activate the tool from the keyboard just press "b" then "a".

I tried to make the key chords consistent and memorable. All elements in the same layer start with the same letter, with a notable exception for technology which I decomposed into technology and physical. The consistent keys are used to activate similar concepts across layers. For example, business process is activated with "bp", application process is activated with "ap", technology process is activated with "tp", etc.

This consistency comes at the slight cost of discoverability for some elements. A notable example is a technology node which one might infer is "tn" but is actually "to". I can explain my reasoning for why the key chords are as they are but that's probably not of interest.

One thing to keep in mind is that after a tool is activated using the keyboard shortcut the pointer needs to be moved in order for the cursor to change on screen.

Phillipus commented 7 months ago

Thanks. Leave it with me (might not be straight away).

Phillipus commented 7 months ago

Hi, I've looked at your code now. Thanks for the work that you put into this, you've obviously figured out how a lot of the code works and is structured.

Your approach is logical in several ways - using the various UIProvider classes to supply the key sequences and then mapping those to the palette tool entries.

However, one downside is that the key combinations are hard-coded and not user-configurable. I can imagine that some users would want to customise these key sequences.

Another problem is that the key sequences can conflict with user-defined key shortcuts. These can be configured in Archi's Preferences under "System" and "Keys". There, I added a shorchut combination of "ba" for a command. This over-rode the palette key chord for Business Actor (expected) but none of the other key chords that started with the "b" character worked ("br", "bb", etc). It's possible that some of these hard-coded key sequences will conflict with users' custom key combinations.

I wonder whether it would be possible to use the Eclipse Command framework (ICommandService) to define the shortcut keys? I'm not sure how feasible this is given the amount of palette entries.

I did something similar for jArchi by declaring a parameterized Command, see here and here

bkyle commented 7 months ago

Thanks for reviewing this pull request. The items you've raised bothered me as well. When I first started looking into implementing this PoC I recall seeing that there was a right way to implement commands and shortcuts but I was concerned I'd end up spending lots of time debugging configuration only to find out that what I wanted to do wasn't possible.

When I get some spare time I'll take a look at the Eclipse Command Framework and the parameterized command you've implemented to see if I can do something similar.

Phillipus commented 7 months ago

@bkyle I've been experimenting somewhat with the Command Framework and will also do some more experiments. I think if we take a bit more time over this, we should come up with a solution.

Phillipus commented 7 months ago

@bkyle I've pushed a new branch called palette-keys with a basic implementation of using the Eclipse Command and Key Binding framework. Caveats:

bkyle commented 7 months ago

This looks great!

If a binding is made without modifiers (Ctrl, Alt keys) such as "BA" then typing that key sequence anywhere in the UI will invoke the command. So you can't use normal key sequences.

I ran into this too and was exploring the use of a custom context so that the command handler would only be invoked in relevant situations.

Phillipus commented 7 months ago

I ran into this too and was exploring the use of a custom context so that the command handler would only be invoked in relevant situations.

We're on the same page. I looked into using the org.eclipse.ui.contexts extension point. The issues are:

Phillipus commented 7 months ago

I've force pushed the palette-keys branch with POC code using an org.eclipse.ui.context for one key binding ("BA") that is active only when a diagram has the focus.

bkyle commented 7 months ago

We're on the same page. I looked into using the org.eclipse.ui.contexts extension point. The issues are:

Great minds think alike 😄

  • It would not solve the problem of invoking the Command when editing the name in an element on a diagram

Some ideas I had to address this are below. If either of these seem plausible I'd be happy to look into them further. I mention them because you may know if they are dead ends.

  1. I see that you've defined a org.eclipse.core.expressions.definitions extension which leads me to believe you know more about the core expression framework than I do 😄. I was hoping that there would be some property that could be used to indicate whether the focus was in an inline editor or not and if so disable the command. Are you aware of any property that could be used for that?
  2. Somewhere in the GEF framework I have to believe that there's code that controls the display and focus of the inline editor. Would it be possible to trap those events to enable/disable the commands?
Phillipus commented 7 months ago

It would not solve the problem of invoking the Command when editing the name in an element on a diagram

This is solved now. The key binding has to be set to the "In Archi Diagrams" context.