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

Fix several problems with our shortcuts system #3235

Merged
merged 36 commits into from
Jun 22, 2016

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jun 20, 2016

The main purpose of this PR is to solve conflicts between global and plugin shortcuts.

Now that we are allowing Spyder to be extended by external plugins, we have to be sure to define shortcuts in its right context to avoid ugly conflicts.

Some problems that this PR solves:

  • Ctrl+Enter was not working on Mac because of conflicts with the IPython console shortcuts.
  • Ctrl+Shift+<X> was not working on Mac because of conflicts with the shortcuts shown in in the View > Panes menu (we were using a hack to avoid that but it doesn't work on Mac! :-).

- This prevents clashes with the IPython Console
- Before they were global shortcuts, which was interfering with the
IPython console shortcuts.
- This attribute is going to be used to save shortcuts and show them
when a user selects a global menu.
- This way we avoid adding global shortcuts that override local ones.
- This way we could show shortcuts for those actions but only when
global menus are made visible by the user.
- By doing that we'll prevent clashes with widget shortcuts but at the
same time we'll be able to show shortcuts in our menus, which is
important for our users.
@ccordoba12 ccordoba12 added this to the v3.0beta4 milestone Jun 20, 2016
@Nodd
Copy link
Contributor

Nodd commented Jun 20, 2016

I don't understand the part about hiding and showing shortcuts in the menu. Why aren't they always shown ? From what I understood, the shortcuts are hidden when the menu is hidden.
Also, removing the shortcut from the action also disables the shortcut ?

@ccordoba12
Copy link
Member Author

@Nodd, thanks for asking! This is the thing:

If we directly assign a shortcut to an action present in a global menu or toolbar (e.g. Run cell) that shortcut acts at the application level and so it supersedes the same shortcut present in any widget (e.g. Ctrl+Enter in the IPython console). Before I was using a hack to solve this: assigning as context for those shortcuts Qt.WidgetWithChildrenShortcut, which in essence disabled them. Problem is this hack doesn't work on Mac, so our shortcuts were really broken there!

However, I think it's quite valuable to show these shortcuts next to their actions' text when the user selects a menu. So that's why I show and hide them when the menu becomes visible/invisible.

I know this is very intricate (and @goanpeca probably won't approve it) but I haven't found find any other way of doing it. And I invested two full days thinking on how to solve this problem!

The easiest thing would be to not show these shortcuts, but I think that wouldn't help our users to memorize them. So we're forced to circumvent Qt limitations here :-)

@Nodd
Copy link
Contributor

Nodd commented Jun 20, 2016

OK I understand. Not a perfect solution indeed, but that's a tricky situation. If Qt.WidgetWithChildrenShortcut works on the other platforms, maybe the set/unset shortcut kack can be enabled on mac only ?

I thought that child widgets had the priority when using shortcuts, but maybe it's not the case with application shortcuts.

I discovered recently that in the vim plugin there is a conflict between Esc to focus the vim widget (defined at application level) and Esc to close the search pane (defined in itself only, I suppose). I'll have to enable the shortcut for the editor only.

@ccordoba12
Copy link
Member Author

maybe the set/unset shortcut hack can be enabled on mac only ?

I really prefer one solution for all platforms. Else you'd have to remember to change contexts for Linux and Windows, and add shown_shortcut to create_action and/or with_effect=False to register_shortcut for Mac.

That could become a nightmare in no time :-)

I discovered recently that in the vim plugin there is a conflict between Esc to focus the vim widget (defined at application level) and Esc to close the search pane

Yes, please define it only for the Editor because it also conflicts with the IPython Console (Esc is used there to hide its completion widget).

@ccordoba12
Copy link
Member Author

@Nodd, on second thought I think you were right :-) So I added code to handle the case of context=Qt.WidgetShortcut for Mac in our create_action and register_shortcut methods.

So things are transparent now for external plugin developers: if they want to disable a shortcut from an action in a global menu, they just need to set their context to Qt.WidgetShortcut and we will take care of the (tricky!!) Mac case ;-)

@ccordoba12
Copy link
Member Author

But for that they need to use create_action, i.e. they can't create the QAction directly.

@ccordoba12
Copy link
Member Author

ccordoba12 commented Jun 22, 2016

Ok, after a lot of work, I think this one is finally ready!

I reviewed all our shortcuts and checked (or fixed) that their contexts were the right ones. Things were in a semi-broken state but hopefully they are good now.

I really don't know how @blink1073 and @goanpeca were able to do substantial contributions to our shortcuts support, given the previous state of things! ;-)

@ccordoba12 ccordoba12 merged commit dc88996 into spyder-ide:master Jun 22, 2016
@ccordoba12 ccordoba12 deleted the fix-shortcuts branch June 22, 2016 20:37
@blink1073
Copy link
Contributor

🎉

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.

3 participants