Add Enhanced Keyboard Shortcut Mechanism #91
Reference in New Issue
Block a user
No description provided.
Delete Branch "enhanced-shortcut-mechanism"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Now there are two standard ways to declare keyboard shortcuts:
EnvoyShortcutConfig- for a global shortcutKeyMappinginterface - for a scene specific shortcut@kske is this how you imagined it to be?
Ok I did not understand every bit of this PR but I assume it works properly so you get an approve from me.
@DieGurke the purpose of this PR was to remove scene-specific code from
SceneContextso that you can easily port it. So, I've implemented a way to add Keyboard shortcuts without cluttering SceneContext.And now we have two options to declare a shortcut: Declaring a global shortcut in
EnvoyShortcutConfigor declaring a scene-specific shortcut by implementingKeyMappingin its controller.Looks well, but before I approve, I would like you to explain some design decisions.
@@ -0,0 +25,4 @@public static void initializeEnvoyShortcuts() {final var instance = GlobalKeyShortcuts.getInstance();// Add the option to exit Linux-like with "Control" + "Q" or "Alt" + "F4"Stop calling this Linux-like. It has nothing to do with Linux. At best it's GNOME-like or GTK-like.
@@ -0,0 +8,4 @@import envoy.client.ui.SceneContext.SceneInfo;/*** Contains all KeyBoardshotcuts used throughout the application.Change "KeyBoardshortcuts" to "keyboard shortcuts".
@@ -0,0 +21,4 @@* @author Leon Hofmeister* @since Envoy Client v0.3-beta*/public final class KeyCombinationAction implements Entry<KeyCombination, Runnable> {What purpose does this serve, and why is this an inner class instead of a nested one?
Entryinterface when implementing it.Okay. I agree. It's unnecessary.
@@ -0,0 +108,4 @@}/*** Returns all stored keyboard shortcuts for the given scene constantMissing a dot here.
@@ -0,0 +8,4 @@/*** Provides methods to set the keyboard shortcuts for a specific scene.* Should only be implemented by Controllers of Scenes so that these methods canChange "Controllers" and "Scenes" to lowercase and "fxml" to all caps.
@@ -107,2 +107,3 @@supplyKeyboardShortcuts(sceneInfo, scene);// Supply the global custom keyboard shortcuts for that scenefor (final var shortcut : GlobalKeyShortcuts.getInstance().getKeyboardShortcuts(sceneInfo))Wouldn't this be easier using
addAll, or does this not fit here?How am I supposed to
addAllfor a custom object type that has no connection to the original? Even if I'd store it in anEntryinstead of an implementation of entry, I couldn't use addAll.@@ -84,6 +85,9 @@ public final class Startup extends Application {stage.setTitle("Envoy");stage.getIcons().add(IconUtil.loadIcon("envoy_logo"));// Configure global shortcuts usedConsider changing this to "Configure global shortcuts"
@@ -41,0 +45,4 @@@Overridepublic Map<KeyCombination, Runnable> getKeyboardShortcuts() {final var map = new HashMap<KeyCombination, Runnable>();Maybe use
Map.of()here instead.Now, that's how I imagined it 👍