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

Keybinding Enhancements #2217

Merged
merged 3 commits into from
May 11, 2015
Merged

Conversation

blink1073
Copy link
Contributor

Fixes #729
Fixes #1212
Fixes #2047
Fixes #2347


Adds the following keybindings, including kill-ring support:

              'editor/start of line': "Meta+A",
              'editor/end of line': "Meta+E",
              'editor/previous line': "Meta+P",
              'editor/next line': "Meta+N",
              'editor/previous char': "Meta+B",
              'editor/next char': "Meta+F",
              'editor/previous word': "Meta+Left",
              'editor/next word': "Meta+Right",
              'editor/kill to line end': "Meta+K",
              'editor/kill to line start': "Meta+U",
              'editor/yank': 'Meta+Y',
              'editor/rotate kill ring': 'Shift+Meta+Y',
              'editor/kill previous word': 'Meta+Backspace',
              'editor/kill next word': 'Meta+D',
              'editor/start of document': 'Ctrl+Up',
              'editor/end of document': 'Ctrl+Down',

Also makes the follow existing key sequences configurable:

              'editor/undo': 'Ctrl+U',
              'editor/redo': 'Ctrl+Y',
              'editor/cut': 'Ctrl+X',
              'editor/copy': 'Ctrl+C',
              'editor/paste': 'Ctrl+V',
              'editor/delete': 'Del',
              'editor/select All': "Ctrl+A",
              'editor/save as': 'Ctrl+Shift+S',

The only changed shortcut is

              'editor/save all': 'Ctrl+Alt+S',

@ccordoba12 ccordoba12 changed the title Emacs keybindings [WIP] Emacs keybindings Mar 1, 2015
@blink1073 blink1073 added this to the v2.4 milestone Mar 1, 2015
@blink1073
Copy link
Contributor Author

I've completed what I originally intended, but am open to suggestion. We could also add another Key column in leiu of the user input method. However, either way we'd have to keep track of state to tell the difference between C-x and C-xa.

@Nodd
Copy link
Contributor

Nodd commented Apr 8, 2015

User input method seems more natural to me. If I remember well Qt supports multiple letter key out of the box so it would be easy to add afterwards.

@goanpeca
Copy link
Member

@blink1073 what needs to be done to get this going?

@blink1073
Copy link
Contributor Author

@goanpeca, this PR is ready as far as I'm concerned. I think it demonstrates a basic functionality that we are missing. I'm not terribly excited about implementing the multi-keystroke functionality.

@goanpeca
Copy link
Member

Why (the multi keystroke), sorry I am kinda o ignorant on this emacs issue...

By multi keystroke it means entering in different states where different keystrokes are used? or..?

@blink1073
Copy link
Contributor Author

Right, unless there is a built in way for Qt to sense a Keystroke sequence for us.

@blink1073
Copy link
Contributor Author

Also, there is the UI of how the user sets the preference for a complex Keystroke sequence. We could allow them to type it in a box with validation, or enter it as a raw key sequence (which for example KDE does when you enter system keyboard shortcuts).

@goanpeca
Copy link
Member

I do not know if Qt has a default for handling states.. if it does not, well I guess is something we need to handle... which, unless I am missing something, would not be very complicated.

@Nodd
Copy link
Contributor

Nodd commented Apr 20, 2015

From http://doc.qt.io/qt-5/qkeysequence.html:

GNU Emacs Style Key Sequences

Key sequences similar to those used in GNU Emacs, allowing up to four key codes, can be created by using the multiple argument constructor, or by passing a human-readable string of comma-separated key sequences.

For example, the key sequence, Ctrl X followed by Ctrl C, can be specified using either of the following ways:

QKeySequence(tr("Ctrl+X, Ctrl+C"));
QKeySequence(Qt::CTRL + Qt::Key_X, Qt::CTRL + Qt::Key_C);

@goanpeca
Copy link
Member

@Nodd I do not see how this would solve this issue of having states?

@Nodd
Copy link
Contributor

Nodd commented Apr 20, 2015

Why do you mean by "state" ?
Why I showed is exactly what is needed to implement:

Add handling of keyboard shortcuts with multiple letter keys? (e.g. C-xs for save).

Qt supports them out of the box.

@goanpeca
Copy link
Member

@Nodd, yes now I see how to use it, my bad ;-)

@ccordoba12
Copy link
Member

Sorry for not reviewing this one before. I'll merge first the other PRs from @goanpeca that are almost ready, and then I'll continue with this one :-)

@goanpeca
Copy link
Member

Almost :-| ? ? ?

@blink1073
Copy link
Contributor Author

I created a small demo showing how to get keyboard sequences from the user. This could be used as the basis for the Keyboard Preferences dialog.

@blink1073
Copy link
Contributor Author

'editor/next char': "Alt+F",
'editor/previous word': "Meta+B",
'editor/next word': "Meta+F",
'editor/Kill to Line End': "Meta+K",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Kill mean? Could you find a better word? ;-)

Also, I'd like (when possible) to use here the same shortcuts as the IPython console (just for consistency and muscle memory :-)

You can find them in

Help > IPython doc > Console help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahem: "C-k: kill from cursor to the end of the line", from the exact docs you pointed to 😛

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know ;-) But since these shortcuts are also going yo be used by people not familiar with Emacs, I was thinking to use Delete or Clear instead of Kill

@ccordoba12
Copy link
Member

This is good and simple, I like it!

The only thing is that I'd like to add more shortcuts because (most surely) people will expect them as part of offering Emacs keybindings. Some of them could be

  • C-x-s -> Save
  • C-x-f -> Open
  • C-s -> Open search and repeat search
  • M-> -> Go to file end
  • M-< -> Go to file start

I don't if @blink1073 can think of others :-)

@blink1073
Copy link
Contributor Author

@ccordoba12, we cannot use compound keyboard preferences unless we change the way they are entered in the dialog.

@goanpeca
Copy link
Member

@blink1073 you would prefer an extra column? or a redesign (with no columns) and a dynamic entry of shortcuts?

@Nodd
Copy link
Contributor

Nodd commented Apr 21, 2015

The columns work, but are not very user-friendly. Adding more of them will be even worse. I'd say that the right thing to do is to enter shortcuts dynamically, it's more practical and more modern.

Here is another idea for later: define default presets to be loaded by the user (Spyder default, Emacs, Matlab, R, Sublime Text, Vi...)

@blink1073
Copy link
Contributor Author

@ccordoba12, I don't follow. You want me to redo the rebase? I'd just as soon copy these files and start a new PR to be honest with you.

@ccordoba12
Copy link
Member

That'd be fine too :-) I just thought you could strip half of your commits here to fix things, but my git-fu is very limited to be honest ;-)

@blink1073 blink1073 force-pushed the emacs-keybindings branch from c39d350 to 8b03dbf Compare May 6, 2015 00:05
@blink1073
Copy link
Contributor Author

Squashing to one commit did the trick, ready!

@ccordoba12
Copy link
Member

Great!! I hope it's not too much to ask, but could you amend the commit message to write something simpler on it? :-)

@blink1073 blink1073 force-pushed the emacs-keybindings branch from 8b03dbf to c60f3a3 Compare May 6, 2015 00:13
@blink1073
Copy link
Contributor Author

Done!

@ccordoba12
Copy link
Member

Thanks! I think the only thing left to fix Meta+Less and Meta+Greater :-)

@blink1073
Copy link
Contributor Author

What do you suggest? The system defaults still work: Ctrl+Home and Ctrl+End.

@blink1073
Copy link
Contributor Author

Should we just use those and punt?

@ccordoba12
Copy link
Member

Yes, let's do that for now. Then in the (future) PR that implements Emacs keybindings, you can see how to solve the issue with Meta+{<,>} :-)

@blink1073
Copy link
Contributor Author

All set.

@ccordoba12
Copy link
Member

Unfortunately all shortcuts that involve Meta don't work on PyQt5. I don't know why :-)

@ccordoba12
Copy link
Member

@Nodd, could you test this branch in PyQt5 5.4 to see if in that version this problem is fixed?

@Nodd
Copy link
Contributor

Nodd commented May 7, 2015

Nope, Meta doesn't work on PyQt5.4.1 either.

@blink1073
Copy link
Contributor Author

Perhaps the Meta key has moved to something like the Esc key in PyQt5?

@Nodd
Copy link
Contributor

Nodd commented May 7, 2015

I don't think so since in #2367 I can set a shortcut using Meta (but I can't use it). Or only the internal representation, not the translated one ?
If it has changed, maybe it's Super now.

@blink1073
Copy link
Contributor Author

Everything is working on PyQt5 on my Mac.

@blink1073
Copy link
Contributor Author

[Python 3.4.3 64bits, Qt 5.3.1, PyQt5 5.3.2 on Darwin]

@@ -717,7 +743,7 @@ def is_ubuntu():
# 2. If you want to *remove* options that are no longer needed in our codebase,
# you need to do a MAJOR update in version, e.g. from 3.0.0 to 4.0.0
# 3. You don't need to touch this value if you're just adding a new option
CONF_VERSION = '17.0.0'
CONF_VERSION = '17.1.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this value to 18.1.0. I merged PR #2237 with the wrong CONF_VERSION and now we need this value :-)

@ccordoba12
Copy link
Member

@blink1073, it seems the problem is only on Linux then ;-)


I think we should merge this PR as it is now and try to find suitable shortcuts in another PR or in master (I could do that :-). This would let us keep things moving because there's nothing else to do here (except for my last comment :-)

@blink1073, what do you think?

@blink1073
Copy link
Contributor Author

Done, merge away!

@ccordoba12
Copy link
Member

Ok, merging!!

ccordoba12 added a commit that referenced this pull request May 11, 2015
@ccordoba12 ccordoba12 merged commit b372429 into spyder-ide:master May 11, 2015
@blink1073 blink1073 deleted the emacs-keybindings branch January 15, 2016 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment