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

Desktop: Add keyboard modes to editor (vim, emacs) #2056

Merged
merged 4 commits into from
Nov 6, 2019

Conversation

JuneKelly
Copy link
Contributor

@JuneKelly JuneKelly commented Nov 3, 2019

This adds a new option to settings("Keyboard Mode"), and then sets the appropriate keyboard handler in the ace editor when it renders.

Options are:

  • Vim
  • Emacs
  • Default

The "default" option is equivalent to the old keyboard behaviour, which (as far as I can tell) is informed by the OS the app is running on.

User-Facing Changes

  • A new option in the Appearance section (Keyboard Mode)
  • When in Vim mode, a red block cursor, and vim-style keybindings
  • When in Emacs mode, a green block cursor, and emacs-style keybindings
  • When in default mode, keyboard behaves exactly as it did before, and nothing looks different

Screenshots

Vim Mode:

image

Review

I'm not sure if these changes tread on any of the existing keyboard-shortcut functionality, and I expect that this feature will require some revision to add anything that's missing to the Vim/Emacs modes.

We may want to signal this feature as being "in beta" and gather feedback on what needs to be improved.

Some notes:

  • The onBeforeLoad handler allows us to get a hold of the Ace instance, and fiddle with it's VimAPI before the editor loads. We need to do this so we can map the :w command to save
    • I'm not totally sure that saveIfNeeded() is what we want to call in that handler, but it seemed the most sensible thing at the time
  • The onLoad handler gives us the Editor instance. Looking at the code again, I've left in a stub which doesn't do anything, but it's possible that in testing we'll discover some problem which can be solved by fiddling with the Editor in that onLoad handler. I'll remove it at the end of review if that turns out to not be the case.

Documentation

None in this PR, but I can add some easily, once we're ready to merge.
Arguably, the whole keyboard-shortcut section of the docs would need to be revised.

Related Issues

This adds a new option to settings, and then sets the
appropriate keyboard handler in the ace editor.

The "default" option is equivalent to the old keyboard
behaviour.
@JuneKelly JuneKelly mentioned this pull request Nov 3, 2019
@JuneKelly
Copy link
Contributor Author

I've just realized that I haven't added any tests, will look into that now...

@laurent22
Copy link
Owner

Thanks for the pull request. I've added a few comments. Tests won't be needed for this as we don't have any for GUI code.

Please could you test the buttons in the editor toolbar (bold, italic, etc.), as well as associated shortcuts, and see how they behave in Vim and Emac mode? If it means we need to disable certain buttons in these modes, that's fine, but we need at least file attachment to work.

@JuneKelly
Copy link
Contributor Author

JuneKelly commented Nov 4, 2019

I've tested the editor buttons, (bold, italic, insert attachment, etc), and they seem to do the right thing (Clicking Bold inserts text like **strong text**, with strong text highlighted.

However, the usual keyboard shortucts for these actions (CTRL-B, CTRL-I, and so on) don't work. Not surprising, because they all have meaning in both the Vim and Emacs modes already. This could present us with a UI dilemma, as we list the shortcuts beside their entries in the Edit menu at the top of the screen.

Should we just disable those hints while in a non-default keyboard mode?
Ultimately we'd probably want to figure out a way for the shortcuts to work in every mode.

@tessus tessus added the desktop All desktop platforms label Nov 5, 2019
@laurent22
Copy link
Owner

Not sure what's the best way for this. I get the feeling we could just merge as is, and users interested in these modes will understand why the shortcuts don't work.

Do you know how other apps that support vim mode handle this?

@JuneKelly
Copy link
Contributor Author

Hmm, the best example is maybe IntelliJ IDEA. They have a pretty well fleshed-out Vim plugin, but it does conflict with the default keybindings. There's some expectation that the user will remap important actions for themselves, or choose one action to bind to when there's a conflict.

So, yes I think we can merge, and the vim nerds will know what to expect. We can then solve the problem if and when we get user reports about it.

@tessus
Copy link
Collaborator

tessus commented Nov 6, 2019

Yep, I agree. vim mode prerequisites some advanced knowledge so I think those users will understand that a few shortcuts won't work anymore. We can always add a comment to the setting, the docs or FAQ. And at one point we might have a system where peoplr can choose their shortcuts...

@laurent22
Copy link
Owner

Ok that sounds good then. The changes are isolated enough that people can just try if they want and then we'll what feedback we get. Thanks for this improvement @ShaneKilkelly!

@laurent22 laurent22 merged commit fa3f0d2 into laurent22:master Nov 6, 2019
@JuneKelly JuneKelly deleted the sk-vim-mode branch November 7, 2019 09:17
scoroi pushed a commit to scoroi/joplin that referenced this pull request Nov 10, 2019
…emacs) (laurent22#2056)

* Add keyboard modes to editor (vim, emacs, default)

This adds a new option to settings, and then sets the
appropriate keyboard handler in the ace editor.

The "default" option is equivalent to the old keyboard
behaviour.

* Remove stray console.log

* Move the keyboard-mode setting to the general section

* Change `keyboardMode` setting to `editor.keyboardMode`
@jorfanakis
Copy link

I apologize if this is the wrong place for this. Just wanted to say thank you. Absolutely love this addition to Joplin.

@0xMH
Copy link

0xMH commented Mar 29, 2020

Just wanted to put Aces' Vim Keymap out here for anyone interested in docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vi[m] mode
5 participants