fix: Zoom-in/out using keyboard shortcut not working correctly (#424) #589

Oliver-Loeffler · 2022-10-19T18:28:52Z

Zoom-In/Out was supposed to work with keyboard shortcuts.
For Zoom-In MODIFIER++ and for Zoom-Out MODIFIER+/ were defined.
The predefined KeyCombinations were not working, there seems something wrong with how KeyCodes are handled especially on macOS but also on Windows. This might become an issue for OpenJFX which is a separate thing.

I've played a while with suitable settings which work on Win/Mac/Linux and settled with following idea:

Platform(s)	Accelerator	Action	App (with GER keyb. layout)
WIndows, Linux	`CTRL`+`+`	Zoom In	ok
WIndows, Linux	`CTRL`+`-`	Zoom Out	ok
WIndows, Linux	`CTRL`+`+` (on num key pad)	Zoom In	ok
WIndows, Linux	`CTRL`+`-` (on num key pad)	Zoom Out	ok
macOS 12.0.1	`CMD`+`+`	Zoom In	ok
macOS 12.0.1	`CMD`+`-`	Zoom Out	ok
macOS 12.0.1	`CMD`+`+` (on num key pad)	Zoom In	ok
macOS 12.0.1	`CMD`+`-` (on num key pad)	Zoom Out	ok

Platform(s)	Accelerator	Action	App (any keyb. layout with cursors)
WIndows, Linux	`SHIFT`+`CTRL`+`up`	Zoom In	ok
WIndows, Linux	`SHIFT`+`CTRL`+`right`	Zoom In	ok
WIndows, Linux	`SHIFT`+`CTRL`+`left`	Zoom Out	ok
WIndows, Linux	`SHIFT`+`CTRL`+`down`	Zoom Out	ok
macOS 12.0.1	`SHIFT`+`CMD`+`up`	Zoom In	ok
macOS 12.0.1	`SHIFT`+`CMD`+`right`	Zoom In	ok
macOS 12.0.1	`SHIFT`+`CMD`+`left`	Zoom Out	ok
macOS 12.0.1	`SHIFT`+`CMD`+`down`	Zoom Out	ok

The DocumentWindowController offers a mainKeyEventFilter where those double accelerators can be tracked.
All key handling happens in MenuBarController but this is called from mainKeyEventFilter in DocumentWindowController.

Issue

Fixes #424

Progress

Change must not contain extraneous whitespace
License header year is updated, if required
The PR name must follow the pre-defined format
Verify the contributor has signed Gluon Individual Contributor License Agreement (CLA)

…ller can handle a 2nd set of accelerators.

Unfortunately key code assignment seems to be broken as it does not match with given system locale.

…S and Linux.

abhinayagarwal · 2023-03-24T10:28:34Z

Testing on Mac OS 13.2.1

Preview app

both CMD + = and CMD + SHIFT + = zooms in
both CMD + - and CMD + SHIFT + - zooms out

SceneBuilder app

CMD + = does nothing.
CMD + SHIFT + = zooms in
CMD + - zooms out
CMD + SHIFT + - zooms all out at once

Oliver-Loeffler · 2023-03-25T10:27:28Z

I will review this and complement the table above with our findings. Actually I've tested the above table on an older x86 Macbook with MacOS 10.12.

abhinayagarwal · 2023-03-27T13:44:08Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+            zoomInMenuItem.setAccelerator(new KeyCharacterCombination("+", modifier)); // NOI18N
+            zoomOutMenuItem.setAccelerator(new KeyCharacterCombination("-", modifier)); // NOI18N
+            // Neither KeyCode.MINUS or KeyCode.SUBTRACT seem to work on macOS.
+            // No chance to test on keyboard with num key pad yet.


This is definitely a strange behavior 🤔

Could you reproduce this?

Yes, I can reproduce this on mac.

May be this requires a different approach depending on the keyboard type. Are you using a separate keyboard or numpad? I've just an old laptop available.

Update 2024 and JavaFX 23.0.1,

zoomInMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.PLUS, modifier)); zoomOutMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.MINUS, modifier));

works just fine on macOS

abhinayagarwal · 2023-03-30T03:45:01Z

@Oliver-Loeffler If you need any help, feel free to ping me. It would be nice to have this PR merged before we merge #619 and release SB 20.

Oliver-Loeffler · 2023-03-30T05:18:44Z

I will look into this one tonight.

Oliver-Loeffler · 2023-03-30T19:32:14Z

Well I just learned that Zooming was also a thing for the Preview. Never thought about this one, just worked on the Zoom function for the editor window. Indeed, I'll update this accordingly.

I have tested the key strokes as described in the table above.
With the keys you have posted, I have no zooming at all for preview.

Testing on Mac OS 12.0.1

Preview app
- both CMD+= and CMD+SHIFT+= zooms in -> cannot reproduce
- both CMD+- and CMD+SHIFT+- zooms out -> cannot reproduce
SceneBuilder app
- CMD+= does nothing. -> I have no = key -> cannot reproduce
- CMD+SHIFT+= zooms in -> does nothing
- CMD+- zooms out -> yes, confirm, can reproduce
- CMD+SHIFT+- zooms all out at once -> yes, confirm, can reproduce

This behavior was actually the reason why I've implemented SHIFT+CMD+right and SHIFT+CMD+up for Zoom-In and SHIFT+CMD+left and SHIFT+CMD+down for Zoom-Out.

I'll take an image of my keyboard - may be this will help to figure out whats off. Unfortunately I have no numpad for my Mac, so I cannot test this.

Oliver-Loeffler · 2023-03-30T20:00:14Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+        if (EditorPlatform.IS_MAC && "+".equals(event.getText())) {
+            runActionController(zoomInController, event);
+            return;
+        }


There is also now special handling of CMD++ for MacOS in the MenuBar controller. Could you please try this @abhinayagarwal ?

Oliver-Loeffler · 2023-03-30T20:30:41Z

Well, the preview window zoom seems to be a different thing, actually I would take care for this in a different PR. I've got this somehow working but not sure if this is desired.

We can couple the preview zoom to the workspace zoom or we can design it a way that preview and workspace can operate with independent zoom levels. Key code / menmoic assignments can stay the same, which part of the program than receives the zoom instruction is the decided by window selection.

Oliver-Loeffler · 2023-04-06T08:54:38Z

I will not get this one completed before end of next week. As I started digging into this topic I found, that the proposed key combinations using + or - (or even =) are not available for many keyboard layouts. My proposal here is to use more generic combinations which are available on most layouts, such as CMD+up for zoom-in and CMD+down for zoom-out. I've also tested this PR using an external Apple keyboard with numpad and it works as expected - given one has a german key layout. This is the point, pre-configured keys for zoom will break or work depending on the keyboard layout used.

Eventually its even thinkable to make those key combinations configurable. As of now, I have not yet found how to detect which keyboard layout is effectively used for a system. If this would be detectable, one could generate key combinations for certain layouts and a generic combination which fits all.

On the other hand, it seems that zoom is actually not yet implemented for the preview window. If this works in some cases, then by accident. This should be solved by a separate PR.

What do you think about this @abhinayagarwal ?

jperedadnr

Tested on macOS, and it looks good. I've left a few comments.

jperedadnr · 2024-10-30T10:40:44Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+     * 
+     * @param event {@link KeyEvent} If any action is triggered, the event will be consumed.
+     */
+    public void handleAdditionalZoomAccelerators(KeyEvent event) {


handleAdditionalZoomAccelerators is public API, so it should check if the modifier is down (any future use of this API else where will fail otherwise if there is no check):

public void handleAdditionalZoomAccelerators(KeyEvent event) { if (event == null || !(EditorPlatform.IS_MAC ? event.isMetaDown() : event.isControlDown())) { return; } ... } ``

jperedadnr · 2024-10-30T10:52:36Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+            zoomInMenuItem.setAccelerator(new KeyCharacterCombination("+", modifier)); // NOI18N
+            zoomOutMenuItem.setAccelerator(new KeyCharacterCombination("-", modifier)); // NOI18N
+            // Neither KeyCode.MINUS or KeyCode.SUBTRACT seem to work on macOS.
+            // No chance to test on keyboard with num key pad yet.


Update 2024 and JavaFX 23.0.1,

zoomInMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.PLUS, modifier)); zoomOutMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.MINUS, modifier));

works just fine on macOS

jperedadnr · 2024-10-30T10:53:27Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

        zoomMenu.getItems().add(zoomOutMenuItem);
-
+        if (EditorPlatform.IS_MAC) {


remove unneeded if-else, new KeyCodeCombination(KeyCode.PLUS, modifier) and new KeyCodeCombination(KeyCode.MINUS, modifier) work fine on macOS with JavaFX 23.0.1

jperedadnr · 2024-10-30T11:11:47Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+            return;
+        }
+
+        if (EditorPlatform.IS_MAC && "+".equals(event.getText())) {


KeyCode.PLUS == event.getCode() works fine

jperedadnr · 2024-10-30T11:12:10Z

app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java

+    public void handleAdditionalZoomAccelerators(KeyEvent event) {
+        // For unknown reasons, on macOS 12.0.1 with  Java 17/JavaFX 19 the "-" key code
+        // is not properly accepted hence this handling here.
+        if (EditorPlatform.IS_MAC && "-".equals(event.getText())) {


KeyCode.MINUS == event.getCode() works fine

Oliver-Loeffler and others added 9 commits October 19, 2022 19:32

Zoom-in/Zoom-out works now with CTRL+Numpad-Add and CTRL+Numpad-Divide.

e31fa43

Exposed zoomIn and zoomOut functionality so that DocumentWindowContro…

8f82c9d

…ller can handle a 2nd set of accelerators.

Reworked key mapping on macOS. CMD+ or CMD- zoom in/out.

e9a0f0e

Unfortunately key code assignment seems to be broken as it does not match with given system locale.

Reworked Windows key assignments.

8af8548

First version of zoom accelerators which works nicely on Windows/macO…

d7c6615

…S and Linux.

Reworked additional keys handling. Added Javadoc for public method.

409f7f9

Removed redundant returns. Numpad key handling is now a separate method.

0db8640

Merge branch 'gluonhq:master' into issue-424

ac7fa35

Merge branch 'gluonhq:master' into issue-424

7aa46cc

abhinayagarwal reviewed Mar 27, 2023

View reviewed changes

Merge branch 'gluonhq:master' into issue-424

7f494b0

Added workaround for "+" on Mac as well.

addcd87

Oliver-Loeffler commented Mar 30, 2023

View reviewed changes

Added workaround for + on Mac as well.

fdec9b9

Oliver-Loeffler force-pushed the issue-424 branch from c953e1f to fdec9b9 Compare March 30, 2023 20:03

abhinayagarwal added this to the 22 milestone Sep 25, 2023

Merge branch 'gluonhq:master' into issue-424

1ccef35

Oliver-Loeffler modified the milestones: 22, 24, 23 Aug 26, 2024

Merge branch 'master' into issue-424

d1e8cc5

Oliver-Loeffler changed the title ~~fix: Zoom-in/out using keyboard shortcut not working correctly~~ fix: Zoom-in/out using keyboard shortcut not working correctly (#424) Oct 1, 2024

Oliver-Loeffler added 3 commits October 2, 2024 19:31

Merge branch 'gluonhq:master' into issue-424

92e5663

Merge branch 'gluonhq:master' into issue-424

b143a12

Merge branch 'gluonhq:master' into issue-424

7c651e8

jperedadnr self-requested a review October 30, 2024 09:46

jperedadnr reviewed Oct 30, 2024

View reviewed changes

Oliver-Loeffler modified the milestones: 24, 25 Dec 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Zoom-in/out using keyboard shortcut not working correctly (#424) #589

fix: Zoom-in/out using keyboard shortcut not working correctly (#424) #589

Oliver-Loeffler commented Oct 19, 2022 •

edited

Loading

abhinayagarwal commented Mar 24, 2023 •

edited

Loading

Oliver-Loeffler commented Mar 25, 2023

abhinayagarwal Mar 27, 2023

Oliver-Loeffler Mar 27, 2023

abhinayagarwal Mar 28, 2023

Oliver-Loeffler Mar 30, 2023

jperedadnr Oct 30, 2024

abhinayagarwal commented Mar 30, 2023

Oliver-Loeffler commented Mar 30, 2023 via email

Oliver-Loeffler commented Mar 30, 2023 •

edited

Loading

Oliver-Loeffler Mar 30, 2023

Oliver-Loeffler commented Mar 30, 2023

Oliver-Loeffler commented Apr 6, 2023

jperedadnr left a comment

jperedadnr Oct 30, 2024 •

edited

Loading

jperedadnr Oct 30, 2024

jperedadnr Oct 30, 2024

jperedadnr Oct 30, 2024

jperedadnr Oct 30, 2024

		zoomMenu.getItems().add(zoomOutMenuItem);

		if (EditorPlatform.IS_MAC) {

fix: Zoom-in/out using keyboard shortcut not working correctly (#424) #589

Are you sure you want to change the base?

fix: Zoom-in/out using keyboard shortcut not working correctly (#424) #589

Conversation

Oliver-Loeffler commented Oct 19, 2022 • edited Loading

Issue

Progress

abhinayagarwal commented Mar 24, 2023 • edited Loading

Preview app

SceneBuilder app

Oliver-Loeffler commented Mar 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinayagarwal commented Mar 30, 2023

Oliver-Loeffler commented Mar 30, 2023 via email

Oliver-Loeffler commented Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Oliver-Loeffler commented Mar 30, 2023

Oliver-Loeffler commented Apr 6, 2023

jperedadnr left a comment

Choose a reason for hiding this comment

jperedadnr Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oliver-Loeffler commented Oct 19, 2022 •

edited

Loading

abhinayagarwal commented Mar 24, 2023 •

edited

Loading

Oliver-Loeffler commented Mar 30, 2023 •

edited

Loading

jperedadnr Oct 30, 2024 •

edited

Loading