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
Owner

Now there are two standard ways to declare keyboard shortcuts:

  1. Add an entry in EnvoyShortcutConfig - for a global shortcut
  2. Let a Controller implement the KeyMapping interface - for a scene specific shortcut
Now there are two standard ways to declare keyboard shortcuts: 1) Add an entry in `EnvoyShortcutConfig` - for a global shortcut 2) Let a Controller implement the `KeyMapping` interface - for a scene specific shortcut
delvh added this to the v0.3-beta milestone 2020-10-09 11:13:03 +02:00
delvh added the
client
M
labels 2020-10-09 11:13:03 +02:00
delvh self-assigned this 2020-10-09 11:13:03 +02:00
delvh requested review from mpk 2020-10-09 11:13:09 +02:00
delvh requested review from kske 2020-10-09 11:13:09 +02:00
Author
Owner

@kske is this how you imagined it to be?

@kske is this how you imagined it to be?
mpk approved these changes 2020-10-09 22:15:01 +02:00
mpk left a comment
Owner

Ok I did not understand every bit of this PR but I assume it works properly so you get an approve from me.

Ok I did not understand every bit of this PR but I assume it works properly so you get an approve from me.
Author
Owner

@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 implementing KeyMapping in its controller.

@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 implementing `KeyMapping` in its controller.
kske requested changes 2020-10-12 07:31:20 +02:00
kske left a comment
Owner

Looks well, but before I approve, I would like you to explain some design decisions.

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.

Stop calling this Linux-like. It has nothing to do with Linux. At best it's GNOME-like or GTK-like.
delvh marked this conversation as resolved
@ -0,0 +8,4 @@
import envoy.client.ui.SceneContext.SceneInfo;
/**
* Contains all KeyBoardshotcuts used throughout the application.

Change "KeyBoardshortcuts" to "keyboard shortcuts".

Change "KeyBoardshortcuts" to "keyboard shortcuts".
delvh marked this conversation as resolved
@ -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?

What purpose does this serve, and why is this an inner class instead of a nested one?
Author
Owner
  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.
Author
Owner

Okay. I agree. It's unnecessary.

Okay. I agree. It's unnecessary.
delvh marked this conversation as resolved
@ -0,0 +108,4 @@
}
/**
* Returns all stored keyboard shortcuts for the given scene constant

Missing a dot here.

Missing a dot here.
delvh marked this conversation as resolved
@ -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.

Change "Controllers" and "Scenes" to lowercase and "fxml" to all caps.
delvh marked this conversation as resolved
@ -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?

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

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.
delvh marked this conversation as resolved
@ -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"

Consider changing this to "Configure global shortcuts"
delvh marked this conversation as resolved
@ -41,0 +45,4 @@
@Override
public Map<KeyCombination, Runnable> getKeyboardShortcuts() {
final var map = new HashMap<KeyCombination, Runnable>();

Maybe use Map.of() here instead.

Maybe use `Map.of()` here instead.
delvh marked this conversation as resolved
kske approved these changes 2020-10-12 10:11:30 +02:00
kske left a comment
Owner

Now, that's how I imagined it 👍

Now, that's how I imagined it 👍
delvh merged commit 75f0a65517 into develop 2020-10-12 16:12:24 +02:00
delvh deleted branch enhanced-shortcut-mechanism 2020-10-12 16:12:35 +02:00
This repo is archived. You cannot comment on pull requests.
No description provided.