Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per shell keyboard shortcuts #45

Closed
wants to merge 5 commits into from

Conversation

Riebart
Copy link
Contributor

@Riebart Riebart commented Jun 18, 2018

There are several architectural discussions to have on this PR:

There are a couple breaking changes:

  • This futzes with the GUIDs associated with the default shells
  • Existing installs won't have the key bindings for the default shells, and I don't know how it'll respond to installs where there are custom shells without any key bindings.
  • The command enum values have been explicitly set, so if they weren't set to their now explicit values, that'll cause issues in previous installs where it may just straight up crash on start.

Discussion points:

  • Key bindings for shells are tied to the shell profile (for storage), and are saved with the shell profiles, not with the key bindings.
    • Key bindings for shells are still edited in the Key Bindings settings panel, and not with the shell profile itself. This was done mostly because I don't know XAML that well, and didn't want to futz around with that, but also because key bindings edit workflows don't follow the Edit/Save workflow of the shell profiles themselves (where they are read-only fields until you Edit the profile, etc...). This felt more natural from a UI/UX perspective, but did require some fudging to pass the ability to for the shell profile model to be able to trigger a key binding refresh on the key bindings panel when shells were added or deleted.
  • This attempts to maintain the current flow of keyboard shortcut handling:
    • Register keyboard command handlers that call AddTerminal(), which had its signature changed to accept a shell profile to launch with. The registering of handlers that are available on startup is easy and handled in the MainViewModel initialization, and subsequent shell profiles are handled by having the MainViewModel add another KeyBindingChange event callback, which has access to the shells.
  • Shells now have a KeyBinding property which is the list f bindings, as well as a KeyBindingCommand which is the Enum value of the command, basically taken as something in the 1024 and up range, or more specifically as something >= Command.ShellProfileShortcut.
    • There's an opportunity here to convert this KeyBindingCommand and the Id into some unified thing, but since the Command enum is used in a lot of places, conflating those two isn't something we can do without a major architectural decision, and as a PR contribution this didn't seem like the time for that.
  • There's a conversation to be had about whether or not using the existing Command/KeyBinding/KeyboardCommandHandler paradigm is the right call for shell profiles, given that those shortcuts are always going to be caught by the TerminalView, and the action could just be taken there. That might simplify the code, but it would need some justification for why we're handling keyboard commands in a variety of different ways (and would make maintaining other keyboard shortcuts a little trickier, or at least more ambiguous).

Notes:

  • There's a lot of linear searching through the shells list, which should be fixed by sorting out the discrepancy

Still to do:
- MUST ensure that the keyboard shortcut handlers are registered for the shell's command when a new shell is added (this requires that the ShellProfileViewModel, or at least SettingsService, has access to teh KeyboardCommandService and RegisterCommandHandler.
  - Without this, currently if you add a shell, then add a keyboard shortcut without reloading the app, attempts to use that shortcut will cause an exception as no handler is registered for that command ID yet.
- SHOULD ensure that when shells are removed that their associated commands are removed from the keyboard command handler.
  - This will reqire adding a DeregisterCommandHandler() funciton to the KeyboardCommandService.
- CAN probably change the shell IDs from GUIDs to match the command IDs, as those will be unique anyway, just not shared across installs.
@felixse
Copy link
Owner

felixse commented Jun 18, 2018

I would really prefer going for a solution that does not rely on KeyBinding/Command. There is a lot of code added that just implicates that we are reusing something here that was not meant for this.
We could make KeyBinding a base class for both CommandKeyBinding (adds int Key) and ProfileKeyBinding (adds Guid ShellProfileId). Then we can just pass it into the webView and handle it pretty much like I described in #37
This might add a second way of handling key shortcuts, but I think it is worth it from an architectural point of view. In the near future I would add some service to register keyBindings and taking care that no keybindings are used twice, this should deal with most of the issues we might have by having two systems in place.

If we go for ProfileKeyBinding we can give the model a an empty List as a default for both existing defaults and custom shells. That way we don't have to change any Ids of the default shells.

The XAML part is something I can take care of, should be no big deal. We can stick to the Edit/Save workflow by adding a readonly mode to the KeyBindingView. I can make a PR on your fork once we got there.

@Riebart
Copy link
Contributor Author

Riebart commented Jun 19, 2018

So a couple questions:

  • Why do the shells have a GUID? Could an incrementing value be used, or is there a future plan here? I've always used UUIDs where I didn't have any synchronizing mechanism to guarantee uniqueness of IDs, but we have one of those here with the SettingsService.
  • Which specific view do you see invoking the shell action? The Terminal View, or another one?

At the core of it, this is getting sticky I think because we're conflating a compile-time concept (a static list of actions we can invoke from the keyboard) with a runtime-concept (the list of shells we have available to invoke new tabs with). This implementation of shell shortcuts is definitely suboptimal, because I'm trying to use a system designed for the former for the latter, but I don't think it's the keybindings that need to change so much as the actions themselves. I may be wrong there.

@felixse
Copy link
Owner

felixse commented Jun 19, 2018

  • I used GUIDs for the themes because they are stored in Roaming and might get merged if you use Fluent Terminal on multiple devices. When I got to implement ShellProfiles I just reused this to keep the implementation similar, but I agree int Ids would have worked as well.
  • Basically the Javascript part which will then pass it down through the bridge to the TerminalView and the TerminalViewModel (pretty much like the existing keybindings, but in this case it should be enough to just pass the shell id)

Yes, ultimatively that's the problem. My point is that if we already store and handle them quite differently, we might also use some subtyping to better express their differentness. So yeah, the keybindings need to change, but we shouldn't change so much of the existing handling for the command keybindings.

@Riebart
Copy link
Contributor Author

Riebart commented Aug 20, 2018

This needs a major rewrite, given changes to the app since this initial implementation. I'm closing this, and will open a new one when I get to working on that feature.

@Riebart Riebart closed this Aug 20, 2018
@Riebart Riebart deleted the per_shell_shortcuts branch October 1, 2018 06:19
@Riebart Riebart restored the per_shell_shortcuts branch March 27, 2019 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants