Keyboard shortcuts change for 0.8.14 - impact on plugins?

[kovidgoyal]

I've also fixed the set to default bug and added shortcut_name to create_action()

Let me know if/when you run into any other issues.

[kovidgoyal]

Added a highlight_group() method tot he preferences widget, to use call it with the group name you want expanded,

 widget.highlight_group(plugin.action_spec[0])

will highlight and expand the shortcut group corresponding to the plugins keyboard shortcuts.

[kiwidude]

@Kovid - just giving this a go now, a few things:

(1) There is a typo in gui2/preferences/keyboard.py of "highligh_group" instead of "highlight_group"

(2) I am launching my dialog to host the widget (rather than using it from its native Preferences dialog home) - as there will be no room to put the widget on my own dialogs without making them all much bigger or tabbed. However I need to figure out the right code to display something as right now the keyboard widget's contents is empty. This snippet below is part of what I have (have extra code for a button box with accept/reject)... what am I missing?

from calibre.gui2.keyboard import ShortcutConfig
class KeyboardConfigDialog(SizePersistedDialog):
    def __init__(self, gui, group_name):
        SizePersistedDialog.__init__(self, gui, 'Keyboard shortcut dialog')
        self.gui = gui
        layout = QVBoxLayout(self)
        self.setLayout(layout)
        self.keyboard_widget = ShortcutConfig(gui)
        layout.addWidget(self.keyboard_widget)
        self.group_name = group_name

    def initialize(self):
        SizePersistedDialog.initialize()
        self.keyboard_widget.initialize(self.gui.keyboard)
        self.keyboard_widget.highlight_group(self.group_name)

(3) How should I handle the situation of where in many of my plugins I end up rebuilding the plugin menus? Because I get an error being thrown where the keyboard logic thinks there is a keyboard shortcut conflict?

calibre, version 0.8.16
ERROR: Unhandled exception: <b>NameConflict</b>:Shortcut for u'Interface Action: Clipboard Search (Clipboard Search) : menu action : Text search' already registered by Text search

Traceback (most recent call last):
  File "D:\CalibreDev\latest\calibre\src\calibre\gui2\dialogs\plugin_updater.py", line 788, in _configure_clicked
  File "D:\CalibreDev\latest\calibre\src\calibre\customize\__init__.py", line 171, in do_user_config
  File "calibre_plugins.clipboard_search.__init__", line 72, in save_settings
  File "calibre_plugins.clipboard_search.action", line 48, in rebuild_menus
  File "calibre_plugins.clipboard_search.common_utils", line 139, in create_menu_action_unique
  File "D:\CalibreDev\latest\calibre\src\calibre\gui2\actions\__init__.py", line 222, in create_menu_action
  File "D:\CalibreDev\latest\calibre\src\calibre\gui2\keyboard.py", line 106, in register_shortcut
NameConflict: Shortcut for u'Interface Action: Clipboard Search (Clipboard Search) : menu action : Text search' already registered by Text search

[kovidgoyal]

2) looks correct. Except that you should pass the dialog as the parent of the widget not the gui.

3) The way I handle this for actions like view is to create the actions only once and add/remove them from the menu as appropriate.

[kiwidude]

My initialize() method wasn't getting called - funny how that meant I got no content! So that is resolved.

Re (3) - hmmm, that is more of a problem. As that means I am going to have to completely rewrite just about every single plugin's menu construction. In particular those that have dynamic menus (many of them) are going to be a big headache I think. Darn.

[kiwidude]

Still thinking about (3). This is going to be a bit of a problem I think, as it brings back into play the reason why I did rebuilding menus in the first place. Take a plugin like Reading List, which has actions on submenus to add and remove from a specific list, which can have shortcuts associated. What happens when the user deletes that list? In my "current" approach, calling self.menu.clear() when the user clicks ok in the config dialog, all the old menus are destroyed and I rebuild everything to reflect the current set of menus. Trying to keep track of the actions registered for each menu, manually removing/adding to those lists is complex enough, but then doing some sort of unregister for deleted menus as well?

I was wondering if there could be a function which given a QMenu iterates through it and "unregisters" where appropriate any shortcuts? So then I could still Clear() and rebuild.

However I realise even that would not be enough because when I rebuild my menus will be registering shortcuts as per what the plugin code thinks they are. The "overriding" which presumably takes place at Calibre startup etc would not be being invoked, so for some of those actions the shortcut will be wrong.

Hmmm.

[kovidgoyal]

I suggest you simply call create_menu_action with shortcut=False, then no registering will happen. You will need to jump through a few hoops to make sure the shortcuts still work though, since the keyboard manager will have the old action registered. Should be as simple as adding a method to the keyboard manager to replace the registered action for a given unique name

[kovidgoyal]

I committed replace_action() I haven't tested it, I leave that to you

[kiwidude]

Thanks. I need to think about this a bit more.

For instance, say I have an action with a default keyboard shortcut. It is almost as if I should be calling register_shortcut completely independently of creating the menu item. Because the first time I create the menu item, I will want along the way to register my default. However the second time I create the menu item (after the app has started up), I don't want a shortcut specified, because the keyboard manager should be managing it after a replace_action() call. Is that how you see it?

Also, how do I handle the situation of an action which no longer should exist? (Such as a user deleting the list example above). Presumably the keyboard manager is still holding a reference to this in it's shortcuts dictionary.

[kovidgoyal]

Yes, you should call register shortcut only once per action, you can do that by passing shortcut=False or you can do it by not using create_menu_action and instead registering manually.

As for actions that are removed, I suppose I can add an unregister method. Remeber to call gui.keyboard.finalize after you're done using unregister.

[kiwidude]

@Kovid,

Ok, I just gave the replace_action() a quick whirl and Calibre crashed in a big way (as in a C++/Qt style crash, no Python stack trace). I have a suspicion the Python code is trying to do something with a disposed Qt object?

So here is what I am doing:
The first time round, keyboard shortcut registered as per "normal", with a call to create_menu_action() and shortcut set to the required value

The second time menu is rebuilt, I do the following:
- call the clear() method on my Qt parent menu
- recreate the menu action doing the same call to create_menu_action(), except with shortcut=False
- build the right unique name. This was a bit of a copy/paste nasty, because the unique name that replace_action does it's key lookup by isn't the unique_name I supplied to create_menu_action(). So I replicate that unique_name = '%s : menu action : %s'%(ia.unique_name, unique_name) prior to calling replace_action
- then I call gui.keyboard.replace_action(unique_name, my_new_action)

At this point the big nasty happens.

[kovidgoyal]

Fixed. Read the docstring of the replace_action() method, to see how to use it correctly.

And the action returned by create_menu_action has a member calibre_shortcut_unique_name which gives you the uniq name used by the shortcut registry.

[kiwidude]

Thanks but afraid it still crashes immediately for me. And the problem with calibre_shortcut_unique_name is that it is only set on the action if shortcut is not False.

[kiwidude]

Here is a simplified example for you to replicate the issue (showing kind of what I am doing). You can just paste this into your "InterfaceDemo" ui.py file:

from PyQt4.Qt import QMenu, QToolButton
from calibre.gui2.actions import InterfaceAction

class InterfacePlugin(InterfaceAction):

    name = 'Interface Plugin Demo'

    action_spec = ('Interface Plugin Demo', None,
            'Run the Interface Plugin Demo', None)
    popup_type = QToolButton.MenuButtonPopup
    action_type = 'current'

    def genesis(self):
        icon = get_icons('images/icon.png')
        self.menu = QMenu(self.gui)

        self.rebuild_menu(first_time=True)

        self.qaction.setMenu(self.menu)
        self.qaction.setIcon(icon)

    def rebuild_menu(self, first_time=False):
        m = self.menu
        m.clear()

        shortcut = 'CTRL+7'
        if not first_time:
            shortcut=False
        my_unique_name='Foo'
        ac = self.create_menu_action(m, my_unique_name, 'Foo Menu', None, shortcut)

        if not first_time:
            # Cannot use ac.calibre_shortcut_unique_name because not set when shortcut=False
            my_unique_name = '%s : menu action : %s'%(self.unique_name, my_unique_name)
            self.gui.keyboard.replace_action(my_unique_name, ac)
            self.gui.keyboard.finalize()

    def initialization_complete(self):
        # Simulating menus getting rebuilt
        self.rebuild_menu(first_time=False)
        pass

[kovidgoyal]

Committed code that makes your test case work. Now you must always call finalize() and calibre_shortcut_unique_name is always present.