Add Enhanced Keyboard Shortcut Mechanism #91
Labels
No Label
client
server
user made
L
M
S
XL
bug
bugfix
discussion
documentation
feature
maintenance
postponed
refactoring
wontfix
No Milestone
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: zdm/envoy#91
Loading…
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 shortcutKeyMapping
interface - 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
SceneContext
so 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
EnvoyShortcutConfig
or declaring a scene-specific shortcut by implementingKeyMapping
in 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?
Entry
interface when implementing it.Okay. I agree. It's unnecessary.
@ -0,0 +108,4 @@
}
/**
* Returns all stored keyboard shortcuts for the given scene constant
Missing 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 can
Change "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 scene
for (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
addAll
for a custom object type that has no connection to the original? Even if I'd store it in anEntry
instead 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 used
Consider changing this to "Configure global shortcuts"
@ -41,0 +45,4 @@
@Override
public Map<KeyCombination, Runnable> getKeyboardShortcuts() {
final var map = new HashMap<KeyCombination, Runnable>();
Maybe use
Map.of()
here instead.Now, that's how I imagined it 👍