Add Enhanced Keyboard Shortcut Mechanism #91

Merged
delvh merged 5 commits from enhanced-shortcut-mechanism into develop 2 years ago
delvh commented 2 years ago
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 2 years ago
delvh added the
client
M
labels 2 years ago
delvh self-assigned this 2 years ago
delvh requested review from DieGurke 2 years ago
delvh requested review from kske 2 years ago
Poster
Owner

@kske is this how you imagined it to be?

@kske is this how you imagined it to be?
DieGurke approved these changes 2 years ago
DieGurke 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.
Poster
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 2 years ago
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.
public static void initializeEnvoyShortcuts() {
final var instance = GlobalKeyShortcuts.getInstance();
// Add the option to exit Linux-like with "Control" + "Q" or "Alt" + "F4"
kske commented 2 years ago
Owner

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
import envoy.client.ui.SceneContext.SceneInfo;
/**
* Contains all KeyBoardshotcuts used throughout the application.
kske commented 2 years ago
Owner

Change "KeyBoardshortcuts" to "keyboard shortcuts".

Change "KeyBoardshortcuts" to "keyboard shortcuts".
delvh marked this conversation as resolved
* @author Leon Hofmeister
* @since Envoy Client v0.3-beta
*/
public final class KeyCombinationAction implements Entry<KeyCombination, Runnable> {
kske commented 2 years ago
Owner

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?
delvh commented 2 years ago
Poster
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.
delvh commented 2 years ago
Poster
Owner

Okay. I agree. It's unnecessary.

Okay. I agree. It's unnecessary.
delvh marked this conversation as resolved
}
/**
* Returns all stored keyboard shortcuts for the given scene constant
kske commented 2 years ago
Owner

Missing a dot here.

Missing a dot here.
delvh marked this conversation as resolved
/**
* Provides methods to set the keyboard shortcuts for a specific scene.
* Should only be implemented by Controllers of Scenes so that these methods can
kske commented 2 years ago
Owner

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
supplyKeyboardShortcuts(sceneInfo, scene);
// Supply the global custom keyboard shortcuts for that scene
for (final var shortcut : GlobalKeyShortcuts.getInstance().getKeyboardShortcuts(sceneInfo))
kske commented 2 years ago
Owner

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?
delvh commented 2 years ago
Poster
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
stage.setTitle("Envoy");
stage.getIcons().add(IconUtil.loadIcon("envoy_logo"));
// Configure global shortcuts used
kske commented 2 years ago
Owner

Consider changing this to "Configure global shortcuts"

Consider changing this to "Configure global shortcuts"
delvh marked this conversation as resolved
@Override
public Map<KeyCombination, Runnable> getKeyboardShortcuts() {
final var map = new HashMap<KeyCombination, Runnable>();
kske commented 2 years ago
Owner

Maybe use Map.of() here instead.

Maybe use `Map.of()` here instead.
delvh marked this conversation as resolved
kske approved these changes 2 years ago
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 2 years ago
delvh deleted branch enhanced-shortcut-mechanism 2 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.