Add Enhanced Keyboard Shortcut Mechanism #91

Merged
delvh merged 5 commits from enhanced-shortcut-mechanism into develop 2020-10-12 16:12:25 +02:00
6 changed files with 15 additions and 59 deletions
Showing only changes of commit 7477f9dbd9 - Show all commits

View File

@ -26,7 +26,8 @@ public class EnvoyShortcutConfig {
public static void initializeEnvoyShortcuts() {
final var instance = GlobalKeyShortcuts.getInstance();
delvh marked this conversation as resolved
Review

Stop calling this Linux-like. It has nothing to do with Linux. At best it's GNOME-like or GTK-like.

Stop calling this Linux-like. It has nothing to do with Linux. At best it's GNOME-like or GTK-like.
// Add the option to exit Linux-like with "Control" + "Q" or "Alt" + "F4"
// Add the option to exit with "Control" + "Q" or "Alt" + "F4" as offered by
// some desktop environments
instance.add(new KeyCodeCombination(KeyCode.Q, KeyCombination.CONTROL_DOWN), ShutdownHelper::exit);
// Add the option to logout using "Control"+"Shift"+"L" if not in login scene

View File

@ -1,64 +1,26 @@
package envoy.client.data.shortcuts;
import java.util.*;
import java.util.Map.Entry;
import javafx.scene.input.KeyCombination;
import envoy.client.ui.SceneContext.SceneInfo;
/**
* Contains all KeyBoardshotcuts used throughout the application.
* Contains all keyboard shortcuts used throughout the application.
*
delvh marked this conversation as resolved
Review

Change "KeyBoardshortcuts" to "keyboard shortcuts".

Change "KeyBoardshortcuts" to "keyboard shortcuts".
* @author Leon Hofmeister
* @since Envoy Client v0.3-beta
*/
public final class GlobalKeyShortcuts {
/**
* Helper class for the Entry interface.
*
* @author Leon Hofmeister
* @since Envoy Client v0.3-beta
*/
public final class KeyCombinationAction implements Entry<KeyCombination, Runnable> {
private final KeyCombination keys;
private final Runnable action;
/**
* Creates a new {@code KeyCombinationAction}.
*
* @param keys the keys to press to perform the given action
* @param action the action to perform
* @since Envoy Client v0.3-beta
*/
public KeyCombinationAction(KeyCombination keys, Runnable action) {
if (keys == null || action == null) throw new NullPointerException("Cannot create key combination action");
this.keys = keys;
this.action = action;
}
@Override
public KeyCombination getKey() { return keys; }
@Override
public Runnable getValue() { return action; }
@Override
public Runnable setValue(Runnable none) {
throw new UnsupportedOperationException("Reassignment of keyboard shortcut actions is not supported");
}
}
private final EnumMap<SceneInfo, Collection<KeyCombinationAction>> shortcuts = new EnumMap<>(SceneInfo.class);
private final EnumMap<SceneInfo, Map<KeyCombination, Runnable>> shortcuts = new EnumMap<>(SceneInfo.class);
private static GlobalKeyShortcuts instance = new GlobalKeyShortcuts();
private GlobalKeyShortcuts() {
for (final var sceneInfo : SceneInfo.values())
shortcuts.put(sceneInfo, new HashSet<KeyCombinationAction>());
shortcuts.put(sceneInfo, new HashMap<KeyCombination, Runnable>());
}
delvh marked this conversation as resolved
Review

What purpose does this serve, and why is this an inner class instead of a nested one?

What purpose does this serve, and why is this an inner class instead of a nested one?
Review
  1. Now it's a nested class
  2. I need to store two parameters for an unknown amount of time, and I don't want to always redeclare the Entry interface when implementing it.
1. Now it's a nested class 2. I need to store two parameters for an unknown amount of time, and I don't want to always redeclare the `Entry` interface when implementing it.
Review

Okay. I agree. It's unnecessary.

Okay. I agree. It's unnecessary.
/**
@ -74,10 +36,7 @@ public final class GlobalKeyShortcuts {
* @param action the action to perform
* @since Envoy Client v0.3-beta
*/
public void add(KeyCombination keys, Runnable action) {
final var keyCombinationAction = new KeyCombinationAction(keys, action);
shortcuts.values().forEach(collection -> collection.add(keyCombinationAction));
}
public void add(KeyCombination keys, Runnable action) { shortcuts.values().forEach(collection -> collection.put(keys, action)); }
/**
* Adds the given keyboard shortcut and its action to all scenes that are not
@ -102,17 +61,16 @@ public final class GlobalKeyShortcuts {
}
// Adding the action to the remaining sceneInfos
final var keyCombinationAction = new KeyCombinationAction(keys, action);
for (final var sceneInfo : include)
shortcuts.get(sceneInfo).add(keyCombinationAction);
shortcuts.get(sceneInfo).put(keys, action);
}
/**
* Returns all stored keyboard shortcuts for the given scene constant
* Returns all stored keyboard shortcuts for the given scene constant.
*
* @param sceneInfo the currently loading scene
* @return all stored keyboard shortcuts for this scene
* @since Envoy Client v0.3-beta
*/
public Collection<KeyCombinationAction> getKeyboardShortcuts(SceneInfo sceneInfo) { return shortcuts.get(sceneInfo); }
public Map<KeyCombination, Runnable> getKeyboardShortcuts(SceneInfo sceneInfo) { return shortcuts.get(sceneInfo); }
}

View File

@ -8,9 +8,9 @@ import envoy.client.ui.SceneContext;
/**
* Provides methods to set the keyboard shortcuts for a specific scene.
* Should only be implemented by Controllers of Scenes so that these methods can
* Should only be implemented by controllers of scenes so that these methods can
delvh marked this conversation as resolved Outdated
Outdated
Review

Change "Controllers" and "Scenes" to lowercase and "fxml" to all caps.

Change "Controllers" and "Scenes" to lowercase and "fxml" to all caps.
* automatically be called inside {@link SceneContext} as soon
* as the underlying fxml file has been loaded.
* as the underlying FXML file has been loaded.
*
* @author Leon Hofmeister
* @since Envoy Client v0.3-beta

View File

@ -106,8 +106,7 @@ public final class SceneContext implements EventListener {
stage.setScene(scene);
// Supply the global custom keyboard shortcuts for that scene
for (final var shortcut : GlobalKeyShortcuts.getInstance().getKeyboardShortcuts(sceneInfo))
scene.getAccelerators().put(shortcut.getKey(), shortcut.getValue());
scene.getAccelerators().putAll(GlobalKeyShortcuts.getInstance().getKeyboardShortcuts(sceneInfo));
delvh marked this conversation as resolved Outdated
Outdated
Review

Wouldn't this be easier using addAll, or does this not fit here?

Wouldn't this be easier using `addAll`, or does this not fit here?
Outdated
Review

How am I supposed to addAll for a custom object type that has no connection to the original? Even if I'd store it in an Entry instead of an implementation of entry, I couldn't use addAll.

How am I supposed to `addAll` for a custom object type that has no connection to the original? Even if I'd store it in an `Entry` instead of an implementation of entry, I couldn't use addAll.
// Supply the scene specific keyboard shortcuts
if (controller instanceof KeyboardMapping) scene.getAccelerators().putAll(((KeyboardMapping) controller).getKeyboardShortcuts());

View File

@ -85,7 +85,7 @@ public final class Startup extends Application {
stage.setTitle("Envoy");
stage.getIcons().add(IconUtil.loadIcon("envoy_logo"));
// Configure global shortcuts used
// Configure global shortcuts
delvh marked this conversation as resolved Outdated
Outdated
Review

Consider changing this to "Configure global shortcuts"

Consider changing this to "Configure global shortcuts"
EnvoyShortcutConfig.initializeEnvoyShortcuts();
// Create scene context

View File

@ -1,6 +1,6 @@
package envoy.client.ui.controller;
import java.util.*;
import java.util.Map;
import javafx.fxml.FXML;
import javafx.scene.control.*;
@ -45,8 +45,6 @@ public final class SettingsScene implements KeyboardMapping {
@Override
public Map<KeyCombination, Runnable> getKeyboardShortcuts() {
final var map = new HashMap<KeyCombination, Runnable>();
map.put(new KeyCodeCombination(KeyCode.B, KeyCombination.CONTROL_DOWN), this::backButtonClicked);
return map;
return Map.of(new KeyCodeCombination(KeyCode.B, KeyCombination.CONTROL_DOWN), this::backButtonClicked);
delvh marked this conversation as resolved Outdated
Outdated
Review

Maybe use Map.of() here instead.

Maybe use `Map.of()` here instead.
}
}