New KeySequence Editor for Keyboard Shortcut Preferences #2367

blink1073 · 2015-04-26T14:52:44Z

Fixes #478
Fixes #494
Fixes #1940
Fixes #2125

Allows the user to enter keyboard shortcut preferences by typing them in.

blink1073 · 2015-04-26T14:53:13Z

So far I just slimmed it down to one column, next I will go and make it editable.

blink1073 · 2015-04-26T14:55:01Z

Note to self, use gist: https://gist.github.com/blink1073/946df268c3685a3f443e

blink1073 · 2015-04-26T20:04:30Z

Boom:

blink1073 · 2015-04-26T20:05:01Z

I used the native shortcut display (this is OSX).

blink1073 · 2015-04-26T20:11:36Z

@ccordoba12, you can now enter the key sequences, and they are getting set properly in Shortcut.save, but the changes are not having any effect on the editor yet.

goanpeca · 2015-04-26T20:15:17Z

Looks cool :-)

Nodd · 2015-04-26T20:16:05Z

Beautiful !

blink1073 · 2015-04-27T02:07:09Z

I stand corrected, I had just forgotten to do a --reset.
We can input any valid keyboard sequence, including compound ones.

ccordoba12 · 2015-04-27T16:12:03Z

This is a great work @blink1073!! Thanks for this :-)

I just have one suggestion for you, that I don't know if it's implemented in your PR or not. I'd like to have every entry in the new editor localized, i.e. that we can read Completado del código (when the locale is es) instead of code completion.

We are very good at localization, so it's really ugly to not have our shortcuts localized right now.

Nodd · 2015-04-27T16:20:34Z

Also the options displayed should be Capitalized in English too, right ?

ccordoba12 · 2015-04-27T16:23:05Z

Yep, that's true too :-)

blink1073 · 2015-04-27T22:43:16Z

Fixed! Note the sweet triple-key sequence.

goanpeca · 2015-04-27T22:47:20Z

I have no idea what that means! but looks awesome

goanpeca · 2015-04-27T22:53:47Z

How are we going to localize the name of the shortcuts?

ccordoba12 · 2015-04-28T20:02:51Z

spyderlib/plugins/shortcuts.py

@@ -159,17 +63,12 @@ def data(self, index, role=Qt.DisplayRole):
        column = index.column()
        if role == Qt.DisplayRole:
            if column == CONTEXT:
-                return to_qvariant(shortcut.context)
+                return to_qvariant(_(shortcut.context.capitalize()))


I don't know if this is enough to get things localized.

Don't we need to pass a real string to _(...) instead of something computed from a method/function, so that gettext grabs that string for translation?

I say it because I haven't seen any examples in our sourcecode of _(...) applied to something that's not a string :-)

Yep, I think this wont do, and we will need to add a tuple or something to the shortcuts list and fix where needed in config.py and the create_shortcut calls... and wherever needed...?

spyderlib\config.py

... ('shortcuts', { # ---- Global ---- # -- In spyder.py '_/close pane': ("Shift+Ctrl+F4", _('Close pane')), '_/preferences': ("Ctrl+Alt+Shift+P", _('Preferences')), ...

Is _ not just a function call, where we pass in the result, which is the computed string?

Not really... the gettext scripts when ran, go through the source code and grab any text and create the .po files. This process is done offline every time we need to update the translations. On runtime the _() will fetch the corresponding translated text...

So they need to be capitalized in config.py then.

Seems like that should be a separate PR, non?

Yep, I think is better for another PR, plus not sure what would be the best way @ccordoba12 to implement it...

Yes, sorry for the noise. Let's leave localization for another PR

Only the modifiers are to be localized, right ? In that case the string should be parsed, each modifier translated, then the string would be rebuilt and displayed. I, don't see us providing a translation for every existing possible keybind.

@Nodd I was thinking of providing localization for the Context and the Name column only

goanpeca · 2015-05-04T12:19:53Z

@blink1073 I am trying this on win7 py2.7 and there is something not working.

If I do not use any modifier it works great, but if I enter a shortcut that uses a modifier (for instance Ctrl+A) I see the shortcut, but as soon as I release the Ctrl Key, the shortcut blinks and disappears and leaves an empty table cell... If I close spyder and then open it again, the table cell remains empty...

blink1073 · 2015-05-05T01:04:59Z

My Windows laptop is near dead, @goanpeca, can you try with the new outfitted debug statements and tell me what you find?

goanpeca · 2015-05-05T12:51:22Z

@blink1073

So I opened the preferences and took the second shortcut for testing (Debug continue) ....

Here is what I get when pressing (I put some comments in between what was printed by the debug_print):

# Pressed Ctrl (and hold)
key 16777249, npressed: 1 

# Pressed and released F12 (still holding Ctrl)
key 16777275, npressed: 2 
decrementing
keys: set([83886139])

# Release Ctrl
keys: set([])

blink1073 · 2015-05-05T13:03:13Z

Hmm, self.keys is getting cleared somewhere. Can you add debug_print in the places where it is cleared?

goanpeca · 2015-05-05T13:21:36Z

@blink1073

Ok I checked now... it seems to be related to the Ctrl key only.

Shift+F12 works
Alt+F12 works
Ctrl+anything... nope

# Press Ctrl
keyPressEvent key 16777249, npressed: 1
# Press and release F12
keyPressEvent key 16777275, npressed: 2
keyPressEvent: decrementing
keyReleaseEvent keys before clear second if: set([83886139])
keyReleaseEvent keys after clear second if: set([])
# Release Ctrl
keyReleaseEvent keys before clear second if: set([])
keyReleaseEvent keys after clear second if: set([])

goanpeca · 2015-05-05T13:39:31Z

Why decremented only when it is a Ctrl modifier?

        if modifiers & Qt.ControlModifier:
            key += Qt.CTRL
            self.npressed -= 1
            debug_print('keyPressEvent: decrementing')

If I comment self.npressed -= 1 it works...

goanpeca · 2015-05-05T13:50:00Z

I notice that only shift and alt allow to put sequence of commands, whereas Ctrl does not allow for that... is this desired/normal?

goanpeca · 2015-05-05T13:57:15Z

@blink1073 this is the "fix" I found for the moment, but I am not sure if it results in the desired behavior...

    def keyReleaseEvent(self, e):
        if self.currentIndex().column() != SEQUENCE:
            debug_print('keyReleaseEvent before clear first if: keys: %s' % self.keys)
            self.keys = set()
            self.npressed = 0
            return

        self.npressed -= 1
        debug_print(self.npressed)

        if self.npressed <= 0:
            if self.keys:
                keySequence = QKeySequence(*self.keys)
                self.model.setData(self.currentIndex(),
                                   keySequence.toString())
            debug_print('keyReleaseEvent before clear second if: %s' % self.keys)
            self.keys = set()
            self.npressed = 0

ccordoba12 · 2015-05-05T14:37:11Z

@goanpeca, you could make a PR against @blink1073 PR with your changes :-)

blink1073 · 2015-05-05T14:42:18Z

@goanpeca, that should fix it. It looks like the decrement was only needed on OSX (but I'll have to test in Linux to be sure).

blink1073 · 2015-05-05T14:43:19Z

I tried it on Win7 and we can now use multi-key Ctrl sequences as well.

goanpeca · 2015-06-25T21:51:02Z

@ccordoba12 you can have fun now reviewing this one ;-)

ccordoba12 · 2015-07-08T13:17:08Z

spyderlib/plugins/shortcuts.py

+MODIFIERS = {Qt.Key_Shift: Qt.SHIFT,
+             Qt.Key_Control: Qt.CTRL,
+             Qt.Key_Alt: Qt.ALT,
+             Qt.Key_Meta: Qt.META}


Why is this mapping necessary?

Not sure, I think @blink1073 did this ? ?

ccordoba12 · 2015-07-08T13:31:39Z

spyderlib/plugins/shortcuts.py

+              border: 0px solid grey;
+              padding:0px;
+              border-radius: 0px;
+            }"""


Does this style work well on all OSes?

I have no idea on mac... but Windows and Linux works fine

But I have been using this style in other widgets now... (like the array builder) so maybe we should have a centralized point for styles (that are used in several places?)?

We have a qss stylesheet for Mac already. But let's leave that for another PR :-)

goanpeca · 2015-07-08T13:53:11Z

Rough edge

On this image from @blink1073, the color of the text should be white... I thought I had fixed that... at least in linux it looks ok

ccordoba12 · 2015-07-08T14:29:34Z

spyderlib/plugins/shortcuts.py

+                                QDialogButtonBox, QLabel, QGridLayout,
+                                QLineEdit, QAbstractItemView,
+                                QSortFilterProxyModel, QStyledItemDelegate,
+                                QStyleOptionViewItemV4, QApplication,


It seems there's nothing named QStyleOptionViewItemV4 on Qt5. There is only QStyleOptionViewItem, so this PR is failing there.

Hmm, ok I will check this then

goanpeca · 2015-07-08T15:27:48Z

Could we perhaps merge it as it is? I can make a new PR to address the pending issues, I feel at this point most of the feedback relates to the changes I introduced so it would make life easier for @blink1073 and myself...

ccordoba12 · 2015-07-08T15:36:05Z

Ok, @blink1073 says he doesn't have a problem with that. So let me finish reviewing this PR, and then you can create a new one to address my comments.

ccordoba12 · 2015-07-08T15:42:56Z

spyderlib/plugins/shortcuts.py

+        # QKeySequence accepts a maximum of 4 different sequences
+        if len(self.keys) > 4:
+            # If the user enters more than 4, take the first 4 only
+            self.keys = set([0, 1, 2, 3])


I don't agree with this behavior. I think we shouldn't allow a shortcut in this case.

So if an user enter a sequence of 5 and bigger nothing should happen ???

I mean, instead of taking part of it

ccordoba12 · 2015-07-08T18:16:36Z

Ok, good to go, on Qt5 too!

New KeySequence Editor for Keyboard Shortcut Preferences

blink1073 changed the title ~~[WIP] KeySequence Editor for Keyboard Shortcut Preferences~~ KeySequence Editor for Keyboard Shortcut Preferences Apr 27, 2015

ccordoba12 added the component:Preferences label Apr 27, 2015

ccordoba12 added this to the v3.0 milestone Apr 27, 2015

ccordoba12 reviewed Apr 28, 2015
View reviewed changes

goanpeca assigned blink1073 May 3, 2015

ccordoba12 reviewed Jul 8, 2015
View reviewed changes

Add a placeholder text for the new shortcut

982277b

ccordoba12 reviewed Jul 8, 2015
View reviewed changes

blink1073 added 3 commits July 8, 2015 12:48

Add a shim for QStyleOptionViewItem

f5a8213

Cleanup

42ae593

Rename optionviewitem

62d7917

ccordoba12 changed the title ~~KeySequence Editor for Keyboard Shortcut Preferences~~ New KeySequence Editor for Keyboard Shortcut Preferences Jul 8, 2015

ccordoba12 added a commit that referenced this pull request Jul 8, 2015

Merge pull request #2367 from blink1073/shortcut-editor

c217ed0

New KeySequence Editor for Keyboard Shortcut Preferences

ccordoba12 merged commit c217ed0 into spyder-ide:master Jul 8, 2015

goanpeca mentioned this pull request Jul 9, 2015

Keyboard shortcut editor enhancements #2523

Merged

6 tasks

blink1073 deleted the shortcut-editor branch January 15, 2016 10:54

New KeySequence Editor for Keyboard Shortcut Preferences #2367

New KeySequence Editor for Keyboard Shortcut Preferences #2367

Conversation

blink1073 commented Apr 26, 2015

blink1073 commented Apr 26, 2015

blink1073 commented Apr 26, 2015

blink1073 commented Apr 26, 2015

blink1073 commented Apr 26, 2015

blink1073 commented Apr 26, 2015

goanpeca commented Apr 26, 2015

Nodd commented Apr 26, 2015

blink1073 commented Apr 27, 2015

ccordoba12 commented Apr 27, 2015

Nodd commented Apr 27, 2015

ccordoba12 commented Apr 27, 2015

blink1073 commented Apr 27, 2015

goanpeca commented Apr 27, 2015

goanpeca commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented May 4, 2015

blink1073 commented May 5, 2015

goanpeca commented May 5, 2015

blink1073 commented May 5, 2015

goanpeca commented May 5, 2015

goanpeca commented May 5, 2015

goanpeca commented May 5, 2015

goanpeca commented May 5, 2015

ccordoba12 commented May 5, 2015

blink1073 commented May 5, 2015

blink1073 commented May 5, 2015

goanpeca commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jul 8, 2015

Rough edge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jul 8, 2015

ccordoba12 commented Jul 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Jul 8, 2015