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

Better default keymap prefix Ctrl-Alt → Ctrl-i #204

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ulidtko
Copy link
Contributor

@ulidtko ulidtko commented Apr 28, 2018

The default keymap is both awkward and dull.

For one, three-key combos like Ctrl-Alt-k are difficult enough to type "in-flow". Shortcuts supposed to be short!

Next, Ctrl-Alt-T is used in wide variety of Desktop Environments to open new terminal (e.g. #159) regardlessly of current focused app. Ctrl-Alt-L is used to lock screen, again globally. I could gratuitously extrapolate this up to Ctrl-Alt-Delete — and infer the convention that "ctrl+alt namespace" is for system shortcuts, not for apps.

What I propose in this change is very simple, and I really like that Atom can do this. Remember the Ctrl-K shortcuts in Atom, like Ctrl-KU for uppercasing, Ctrl-KB for show/hiding the treeview? We can do the same with Ctrl-I, and carve out a sort of "Idris namespace" in the keymap.

What this means, is that now:

  • Ctrl-IT does Idris type-of;
  • Ctrl-IC does Idris case-split;
  • Ctrl-IW does Idris make-with;
  • Ctrl-IEnter does Idris open-repl.

✔️ easy to type
✔️ easy to remember
✔️ no conflicts with system area
✔️ Just Works by default
✔️ #116 #159 #226 -- all fixed for good

Nothing maps Ctrl-I by default in my Atom installation. Any intra-Atom conflicts, if exist, are unknown to me. Note: this keymap effectively "uses up" just one shortcut from the Atom namespace, instead of ~two dozen. This reduces the likelihood of future conflicts.

The OS X part of keymap, natually, uses Cmd-I prefix as well, for partly the same reasons as above + keeping consistency.


Amended

✔️ smooth migration UI to avoid breaking books and tutorials (see comments below)

@ulidtko
Copy link
Contributor Author

ulidtko commented Apr 28, 2018

TL;DR: just read the diff and hit Merge if LGTY.

@melted
Copy link
Contributor

melted commented Apr 29, 2018

On one hand it's a good idea, On the other hand it will always cause trouble to change such things.

It will impact @edwinb's book too. Maybe it could be activated by a toggle in the settings page?

@ulidtko
Copy link
Contributor Author

ulidtko commented May 1, 2018

activated by a toggle in the settings page

No, please no. Having an unconflicting, ergonomic and straightforwardly mnenomic keymap "out of the box" is not a preference -- it's an expectation.

How otherwise would you suggest, #159 should be fixed?
The only option besides changing the defaults, is forcing users to patch up defaults' defects themselves. Which is, kinda, not cool. Not the way it should be done.


I do have an idea how to avoid breaking books (as well as blog posts etc): we could bind the Ctrl-Alt-_ shortcuts all to the same action, let's say language-idris:legacy-keymap-notice. That would only show an infobox notifying the user about the keymap change, the new shortcuts, and also featuring a button (or a snippet to paste into keymap.cson) to the effect of "bring it back how it was before".

Sounds good?

If no objections, I can implement that, no problem. The migration will be extra-smooth then.

@archaeron, can I get your opinion? I see you were working on exactly the same issue in #26. Like my proposal? Would merge / would not merge?

ulidtko added a commit to ulidtko/atom-language-idris that referenced this pull request May 1, 2018
@ulidtko
Copy link
Contributor Author

ulidtko commented May 1, 2018

I've put together a little migration helper UI as above. Looks like this:

screenshot from 2018-05-01 21-59-52

The migrations.coffee module is not top-quality, as it's intended to be thrown away sooner or later.

Please review & test.

ulidtko added a commit to ulidtko/atom-language-idris that referenced this pull request May 1, 2018
@melted
Copy link
Contributor

melted commented May 2, 2018 via email

@justjoheinz
Copy link
Contributor

I will try out the branch as well, but for me it looks fairly cumbersome. Ctrl-Alt feels like one key for me, but maybe I am just an old dog refusing to learn new tricks.

@justjoheinz
Copy link
Contributor

What are the actual keybindings of the emacs idris mode? It would be nice to have something in line with them, in order to help users use both modes.

@ulidtko
Copy link
Contributor Author

ulidtko commented May 2, 2018

@justjoheinz No idea about the emacs-space, unfortunately I can't use emacs (yet).

Ctrl-Alt feels like one key for me

Well, yeah, maybe. It's still the same three keypresses basically.

The problem with Ctrl-Alt is that Ctrl-Alt-T will pop up a terminal on Ubuntu, and apps like Atom have no chance to bind it -- it's simply not being delivered to apps.

Ctrl-Alt shortcuts are also more likely to clash with other Atom packages, with the likelihood going up as the number of bound Ctrl-Alt-letters grows. Not so with the Ctrl-i prefix.

@ulidtko
Copy link
Contributor Author

ulidtko commented May 22, 2018

Anyone?..

@melted
Copy link
Contributor

melted commented May 22, 2018 via email

@archaeron
Copy link
Member

Not using this often at the moment and also not maintaining it (thanks to everyone for that)
But I still like the idea of using up only one keystroke

@ulidtko
Copy link
Contributor Author

ulidtko commented Feb 1, 2019

Hey all. I've rebased this branch on top of the latest master, to resolve a conflict introduced when #218 has been merged.

Whoever wears the maintainer hat these days -- please take a look. The dupes will continue to be reported on the issue; and this PR solves the issue at its root, in the nicest possible way.

Please, merge!

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

4 participants