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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e31fa43
Zoom-in/Zoom-out works now with CTRL+Numpad-Add and CTRL+Numpad-Divide.
Oliver-Loeffler Oct 19, 2022
8f82c9d
Exposed zoomIn and zoomOut functionality so that DocumentWindowContro…
Oliver-Loeffler Oct 19, 2022
e9a0f0e
Reworked key mapping on macOS. CMD+ or CMD- zoom in/out.
Oliver-Loeffler Oct 19, 2022
8af8548
Reworked Windows key assignments.
Oliver-Loeffler Oct 19, 2022
d7c6615
First version of zoom accelerators which works nicely on Windows/macO…
Oliver-Loeffler Oct 19, 2022
409f7f9
Reworked additional keys handling. Added Javadoc for public method.
Oliver-Loeffler Oct 19, 2022
0db8640
Removed redundant returns. Numpad key handling is now a separate method.
Oliver-Loeffler Oct 19, 2022
ac7fa35
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Nov 1, 2022
7aa46cc
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Nov 2, 2022
7f494b0
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Mar 30, 2023
addcd87
Added workaround for "+" on Mac as well.
Oliver-Loeffler Mar 30, 2023
fdec9b9
Added workaround for + on Mac as well.
Oliver-Loeffler Mar 30, 2023
1ccef35
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Mar 18, 2024
d1e8cc5
Merge branch 'master' into issue-424
Oliver-Loeffler Oct 1, 2024
92e5663
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Oct 2, 2024
b143a12
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Oct 2, 2024
7c651e8
Merge branch 'gluonhq:master' into issue-424
Oliver-Loeffler Oct 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ public enum ActionStatus {
}
event.consume();
}

if (modifierDown) {
menuBarController.handleAdditionalZoomAccelerators(event);
}
};

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyCodeCombination;
import javafx.scene.input.KeyCombination;
import javafx.scene.input.KeyEvent;
import javafx.scene.layout.StackPane;

/**
Expand Down Expand Up @@ -209,7 +210,11 @@ public class MenuBarController {
private MenuItem showSampleControllerMenuItem;
@FXML
private Menu zoomMenu;


private ZoomInActionController zoomInController;

private ZoomOutActionController zoomOutController;

// Modify
@FXML
private MenuItem fitToParentMenuItem;
Expand Down Expand Up @@ -1274,22 +1279,36 @@ private void handleOnActionMenu(MenuItem i) {
*/

final static double[] scalingTable = {0.25, 0.50, 0.75, 1.00, 1.50, 2.0, 4.0};

private void updateZoomMenu() {
final double[] scalingTable = {0.25, 0.50, 0.75, 1.00, 1.50, 2.0, 4.0};

zoomInController = new ZoomInActionController();
final MenuItem zoomInMenuItem = new MenuItem(I18N.getString("menu.title.zoom.in"));
zoomInMenuItem.setUserData(new ZoomInActionController());
zoomInMenuItem.setAccelerator(new KeyCharacterCombination("+", modifier)); //NOI18N
zoomInMenuItem.setUserData(zoomInController);
zoomMenu.getItems().add(zoomInMenuItem);


zoomOutController = new ZoomOutActionController();
final MenuItem zoomOutMenuItem = new MenuItem(I18N.getString("menu.title.zoom.out"));
zoomOutMenuItem.setUserData(new ZoomOutActionController());
zoomOutMenuItem.setAccelerator(new KeyCharacterCombination("/", modifier)); //NOI18N
zoomOutMenuItem.setUserData(zoomOutController);
zoomMenu.getItems().add(zoomOutMenuItem);

if (EditorPlatform.IS_MAC) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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.
Comment on lines +1296 to +1299
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is definitely a strange behavior 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you reproduce this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I can reproduce this on mac.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

} else {
// Accelerators for standard keyboard (not num key pad)
zoomInMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.PLUS, modifier)); // NOI18N
zoomOutMenuItem.setAccelerator(new KeyCodeCombination(KeyCode.MINUS, modifier)); // NOI18N
// Does not work on Windows:
// zoomInMenuItem.setAccelerator(new KeyCharacterCombination("+", modifier)); // NOI18N
//
// Works in Windows:
// zoomOutMenuItem.setAccelerator(new KeyCharacterCombination("-", modifier)); // NOI18N
}
zoomMenu.getItems().add(new SeparatorMenuItem());

for (int i = 0; i < scalingTable.length; i++) {
final double scaling = scalingTable[i];
final String title = String.format("%.0f%%", scaling * 100); //NOI18N
Expand All @@ -1299,7 +1318,61 @@ private void updateZoomMenu() {
}
}


private void runActionController(MenuItemController controllerToRun, KeyEvent event) {
if (controllerToRun.canPerform()) {
controllerToRun.perform();
}
event.consume();
}

/**
* Provides handling for additional keyboard accelerators assigned to document view
* zoom in/out operation. Zoom is controllable via [+]/[-] keys on standard keyboard
* and on numerical key pad.
*
* Additionally zoom in via arrow keys (up/right) and zoom out (left/down) is possible.
*
* @param event {@link KeyEvent} If any action is triggered, the event will be consumed.
*/
public void handleAdditionalZoomAccelerators(KeyEvent event) {
Copy link
Collaborator

@jperedadnr jperedadnr Oct 30, 2024

Choose a reason for hiding this comment

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

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;
        }
        ...
}
``

// 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())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

runActionController(zoomOutController, event);
return;
}

if (EditorPlatform.IS_MAC && "+".equals(event.getText())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

runActionController(zoomInController, event);
return;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


if (event.isShiftDown()) {
handleArrowKeysForZoom(event);
} else {
handleNumPadKeys(event);
}
}

private void handleNumPadKeys(KeyEvent event) {
switch (event.getCode()) {
case ADD -> runActionController(zoomInController, event);
case SUBTRACT -> runActionController(zoomOutController, event);
default -> { /* no action*/ }
}
}

private void handleArrowKeysForZoom(KeyEvent event) {
switch (event.getCode()) {
case RIGHT -> runActionController(zoomInController, event);
case UP -> runActionController(zoomInController, event);
case LEFT -> runActionController(zoomOutController, event);
case DOWN -> runActionController(zoomOutController, event);
default -> { /* no action*/ }
}
return;
}

private static int findZoomScaleIndex(double zoomScale) {
int result = -1;

Expand Down