Since Krita has forked xmlgui there is an opportunity to redesign any aspect of the shortcut configuration system we please. The system needs a real, consistent design with a goal-directed refactoring.
**The existing system.**
The current system is hobbled together. There are two main sources of actions inside the existing system, a wide variety of other actions defined in the old Calligra libraries, and a second, entirely separate input configuration subsystem.
- KisAction
-- Used throughout the GUI, inherits from QWidgetAction
-- These actions are associated with a particular MainWindow instance.
-- Generally created in KisMainWindow and KisViewManager::setupManagers()
-- Each KisViewManager has a KisActionManager, which sort of does the task we're trying to do already.
-- Generally the best, very regular creation pattern. KisActions are A-OK!
- QActions managed through XMLGUI / KActionCollection
-- Probably 50% of mentions to QAction in the source code are in widgetutils and xmlgui, managed through KActionCollection. The MainWindow, being the top level unit of XMLGUI in Krita, has a KActionCollection. KisActionManager indirectly manages this KActionCollection, through a pointer to the KActionCollection.
-- kstandardaction is interesting. It is probably not how we want to manage our own shortcuts, we may want to absorb it.
-- slangkamp wrote KisViewPlugin, which krita/plugins/extensions, a hybrid between XMLGUI QActions and the KisActions system.
- QActions managed through tools, flake, kundo2, other old Calligra libraries.
-- There are some patterns here, but many differing uses.
-- KoToolBasePrivate contains a QHash<QString, QAction *> of named actions. In particular, this allows the tool to connect its actions when assembling an OptionWidget.
-- Artistictextshape, textshape, and KoPathTool use a great deal of actions with hard coded shortcuts. ConnectionTool, DefaultTool as well. What to do? Add extra pages in the shortcut configuration dialog for keyboard shortcuts during the tool?
-- KisTools also use some of the functionality from the base class, defining additional actions and storing them internally.
-- Kundo2, KoToolProxy, KoToolManager, and libs/widgets/dockers all define a few QActions, seemingly idiosyncratic as well.
-- krita/ui has a deal of QActions scattered around in other places not mentioned above.
- Finally, there is KisInputProfileManager and KisShortcutConfiguration.
-- This is a completely separate system, for the KisTool's Primary/SecondaryAction settings.
-- It's also global static class and uses its own configuration file. It is alright to leave it alone for now.
-----
**Redesign proposal**
Right now, KisPart takes responsibility for dealing with the shortcuts configuration file krita.action and launching the configuration dialog. KisPart is a global static class, it is no problem to carve it into a separate, dedicated global static factory class, which focuses on the shortcuts configuration. We can call this global static class KisShortcutRegistry.
A typical routine to construct a KisAction currently looks something like this. (From KisMainWindow.cpp:2089)
```
d->closeAll = new KisAction(i18nc("@action:inmenu", "Close All"));`
d->closeAll->setActivationFlags(KisAction::ACTIVE_IMAGE);
d->closeAll->setDefaultShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_W));
actionManager->addAction("file_close_all", d->closeAll);
connect(d->closeAll, SIGNAL(triggered()), this, SLOT(slotFileCloseAll()));
```
The string "file_close_all" is the unique identifier name for the action. A basic mechanism is for KisShortcutRegistry to first load XML file krita.action, then expose a call like `defaultShortcut(QString actionName)`, which looks up the shortcut associated with this action. The KisShortcutRegistry could also manage a user configuration file which looks for customized shortcuts within a human readable .ini file. It will see if there are any user customized shortcuts for the action, and if not, it will look for a default inside the krita.action XML file.
```
QKeySequence shortcut = KisShortcutRegistry::defaultShortcut("file_close_all");
d->closeAll->setDefaultShortcut(shortcut);
```
It's immediately obvious that those two lines can be combined into one. The next question is whether shuffling that QKeySequence around is even necessary in the first place. Since we have an external database of defaults, it quickly becomes easy to export essentially all the static information in the five lines above. One could imagine now that krita.action becomes the definitive source of global defaults. KisShortcutRegistry could then become a factory class. When it creates a new action, it can set up the default shortcut, if any, add the tooltip, and perhaps other things. Since C++11 allows connecting signals and slots via std::function, The new syntax might look like this:
```
d->exportPdf = KisShortcutRegistry::createKisAction("file_export_pdf", this, &slotFileCloseAll);
d->closeAll->setActivationFlags(KisAction::ACTIVE_IMAGE);
actionManager->addAction("file_close_all", d->closeAll);
```
That could be simplified even further, if we contain things like ActivationFlags in krita.action.
---
**Discussion**
- A quick scan returns 138 calls to action->setShortcuts() throughout Krita, for both QAction and KisAction, and approximately 100 of those are in Krita's codebase, not XMLGUI. Although the easiest part of the porting will be reusing the mechanics from KisAction and KisView, It would not be hard to write KisShortcutRegistry::createQAction() methods afterward, with the ultimate goal of containing all the defaults in one place. Some helper methods like `::defaultShortcut(QString name) could handle cases where the factory pattern does not work. There is no reason the shortcut registry has to start by handling *every last action at once,* it can incrementally expand to clean up more of the codebase over time. (In particular, we don't have to touch the text tools until we feel like it.)
- Slangkamp and boud point out that ultimately, the problem stems from having multiple sources originating actions, in particular XMLGUI. In the long term, it may be worthwhile to do even greater global unification of actions. However, focusing right now on a global shortcut data registry should be good enough. It is a fine stepping stone to unifying all of the mess above. The key is that we can code up our own system for saving custom shortcuts, and make it robust. Then it will not be a problem to rework the XMLGUI areas later, nor is it a problem to bringing in more QActions as each part of the code base gets reworked. The only requirement is that we should maintain a single, clean namespace for Krita's actions.
- The thing I am concerned about right now is i18n. For maximum convenience, we will want our krita.action XML file to be the ultimate source for translation strings. You will notice in my new syntax, i18n() was stripped out. I don't know enough about translation to say whether this is possible, or if there's another second-best solution.
- The proposal means only one additional static class which can be added incrementally, so if the general idea sounds good I can start immediately.
-----
**Qt Creator**
Qt Creator is a good example of a roll-your-own shortcut system. [[ https://github.com/qtproject/qt-creator/blob/master/src/plugins/coreplugin/actionmanager/actionmanager.cpp | They funnel everything into a single class called ActionManager ]]. See Command.h and ActionContainer.h in the same directory for more of their abstractions, perhaps playing a role similar to XMLGUI. Most useful would be examining their shortcut configuration file format.
---
**Bugs**
** Make import/export simple for users: https://bugs.kde.org/show_bug.cgi?id=335153
** Be loud when detecting duplicate shortcuts, but handle them gracefully: https://bugs.kde.org/show_bug.cgi?id=329663
** Be fully translatable: https://bugs.kde.org/show_bug.cgi?id=344491
** Integrate with sketch: https://bugs.kde.org/show_bug.cgi?id=331334
** Handle shortcuts within various widgets & dialogs: https://bugs.kde.org/show_bug.cgi?id=353790
** Make import/export cross platform. This probably needs to come later.
** XMLGUI? display shortcuts on mouse-over whenever possible.
** XMLGUI? Ctrl+w shortcut cannot be changed: https://bugs.kde.org/show_bug.cgi?id=352205
** XMLGUI? Annoying KDE conflict message box: https://bugs.kde.org/show_bug.cgi?id=347337
** XMLGUI? Krita moves around with alt+drag: https://bugs.kde.org/show_bug.cgi?id=331485
** Engage nicely with the text toolSince Krita has forked xmlgui there is an opportunity to redesign any aspect of the shortcut configuration system we please. The system needs a consistent design with a goal-directed refactoring.
**The existing system.**
The current system does not have a unified design. My inspection identified four sorts of places where shortcuts are being defined. There are two centralized sources of QActions inside the existing system, the KisActions and the XMLGUI, there are a variety of other places in the old Calligra libraries that define more QActions, and there is also an entirely independent configuration subsystem for PrimaryAction/SecondaryAction.
1. KisActions
-- These are for the GUI, and inherit from QWidgetAction.
-- These actions are associated with a particular MainWindow instance.
-- Most of these are created in KisMainWindow and KisViewManager::setupManagers().
-- The configuration file krita.action already contains most of the data for these. KisPart is the central configuration manager.
-- Generally these are the nicest part of the code. They have a very regular creation pattern.
-- KisActionManager already does a lot of the central bookkeeping for these things.
-- In general, we want to make the other things work more like KisActions.
2. XMLGUI / KActionCollection
-- Probably 50% of mentions to QAction in the source code are in widgetutils and xmlgui libraries. These are managed through KActionCollection.
-- Each MainWindow, being the top level unit of XMLGUI in Krita, has a KActionCollection.
-- Each MainWindow also has a KisActionManager.
-- The KisActionManager manages KisActions directly, but it also manages the XMLGUI actions as well through a pointer to the KActionCollection.
-- KStandardAction is interesting. It is sort of like a central configuration file for the most common actions, like adding Ctrl+S for save.
-- KStandardAction is nice, but it is also problematic, because we don't have a systematic way to override its standards.
-- slangkamp wrote KisViewPlugin, which is used in krita/plugins/extensions. These provide a hybrid between XMLGUI QActions and the KisActions system.
3. QActions managed through flake, kundo2, libkritaui, and other Krita libraries.
-- There are some patterns here, but many differing uses.
-- The vast majority of these are found in the old Calligra libraries.
-- The vast majority of these are found in the KoTool classes.
-- KoToolBasePrivate contains a QHash<QString, QAction *> of named actions. In particular, this allows the tool to connect its actions when assembling an OptionWidget.
-- Artistictextshape, textshape, and KoPathTool use a great deal of actions with hard coded shortcuts. ConnectionTool, DefaultTool as well. What to do? Add extra pages in the shortcut configuration dialog for keyboard shortcuts during the tool?
-- KisTools also use some of the functionality from the base class, defining additional actions and storing them internally.
-- Elsewhere in the Calligra libraries, there are Kundo2, KoToolProxy, KoToolManager, and libs/widgets/dockers all define a few QActions, seemingly idiosyncratic.
-- libkritaui also defines a good deal of QActions here and there, outside of libkritaui/tools.
4. Finally, there is KisInputProfileManager and KisShortcutConfiguration.
-- This is a completely separate system, for the KisTool's Primary/SecondaryAction settings.
-- It's also global static class and uses its own configuration file. It is alright to leave it alone for now.
-----
**Redesign proposal**
I propose a new global static class (tentatively!) called KisActionRegistry with the role of providing default data for actions throughout Krita. I will argue that the most natural role is to treat KisActionRegistry as a factory class.
A typical routine to construct a KisAction currently looks something like this. (From KisMainWindow.cpp:2089)
```
d->closeAll = new KisAction(i18nc("@action:inmenu", "Close All"));`
d->closeAll->setActivationFlags(KisAction::ACTIVE_IMAGE);
d->closeAll->setDefaultShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_W));
actionManager->addAction("file_close_all", d->closeAll);
connect(d->closeAll, SIGNAL(triggered()), this, SLOT(slotFileCloseAll()));
```
The string "file_close_all" is the unique identifier name for the action. All of the data like the shortcut, tooltip, activation flags, and so on are already duplicated in krita.action. This is quite a lot of data written as C++ code. There is zero performance gain.
Therefore, KisActionRegistry will take over from KisPart the handling of krita.action. (As I mentioned above, KisPart is a global static class, so there should not be any new issues with lifetime.) Furthermore, the KisActionRegistry could also manage a user configuration file which looks for customized shortcuts within a human readable .ini file. It will see if there are any user customized shortcuts for the action, and if not, it will look for a default inside the krita.action XML file. All of this could be transparent. Therefore it seems natural to make KisActionRegistry into factory class. When it creates a new action, it can set up the default shortcut, if any, add the tooltip, and perhaps other things. Since C++11 allows connecting signals and slots via std::function, The new syntax might look like this:
```
d->exportPdf = KisActionRegistry::createKisAction("file_export_pdf", this, &slotFileCloseAll);
```
Not all actions can be easily converted to being provided by a global factory this way. Ultimately we would like to transition to a single place where all action data lives, but during the transition, KisActionRegistry could also provide the following calls:
```
QKeySequence KisActionRegistry::shortcut(QString actionID); // Return shortcut information only
KisAction * KisActionRegistry::createKisAction(QString actionID); // Create action without adding a connection
QAction * KisActionRegistry::createQAction(QString actionID, QObject * that, std::function<()> slot); // Create QAction instead of KisAction
```
---
**Discussion**
- A quick scan returns 138 calls to action->setShortcuts() throughout Krita, for both QAction and KisAction, and approximately 100 of those are in Krita's codebase, not XMLGUI. Although the easiest part of the porting will be reusing the mechanics from KisAction and KisView, It would not be hard to write KisActionRegistry::createQAction() methods afterward, with the ultimate goal of containing all the defaults in one place. Some helper methods like `::defaultShortcut(QString name) could handle cases where the factory pattern does not work. There is no reason the shortcut registry has to start by handling *every last action at once,* it can incrementally expand to clean up more of the codebase over time. (In particular, we don't have to touch the text tools until we feel like it.)
- Slangkamp and boud point out that ultimately, the problem stems from having multiple sources originating actions, in particular XMLGUI. In the long term, it may be worthwhile to do even greater global unification of actions. However, focusing right now on a global shortcut data registry should be good enough. It is a fine stepping stone to unifying all of the messdiffe above.
- KisActionRegistry can be the place where we ensure shortcut data is safe and can be upgraded safely. If we code up our own system for saving custom shortcuts, we can make it as robust as we like. The only requirement is that we should maintain a single, clean namespace for Krita's actions.
- The thing I am most concerned about right now is i18n. You will notice in my new syntax, i18n() was stripped out. The most conveinent thing to do would be to have the krita.action XML be the ultimate source for translation strings. I don't know enough about ki18n to say how/whether this can be done, or what the second best solution would be.
-----
**Qt Creator**
Qt Creator is a good example of a roll-your-own shortcut system. [[ https://github.com/qtproject/qt-creator/blob/master/src/plugins/coreplugin/actionmanager/actionmanager.cpp | They funnel everything into a single class called ActionManager ]]. See Command.h and ActionContainer.h in the same directory for more of their abstractions, perhaps playing a role similar to XMLGUI. Most useful would be examining their shortcut configuration file format.
---
**Bugs**
** Make import/export simple for users: https://bugs.kde.org/show_bug.cgi?id=335153
** Be loud when detecting duplicate shortcuts, but handle them gracefully: https://bugs.kde.org/show_bug.cgi?id=329663
** Be fully translatable: https://bugs.kde.org/show_bug.cgi?id=344491
** Integrate with sketch: https://bugs.kde.org/show_bug.cgi?id=331334
** Handle shortcuts within various widgets & dialogs: https://bugs.kde.org/show_bug.cgi?id=353790
** Make import/export cross platform. This probably needs to come later.
** XMLGUI - display shortcuts on mouse-over whenever possible.
** XMLGUI - Ctrl+w shortcut cannot be changed: https://bugs.kde.org/show_bug.cgi?id=352205
** XMLGUI - Annoying KDE conflict message box: https://bugs.kde.org/show_bug.cgi?id=347337
** XMLGUI - Krita moves around with alt+drag: https://bugs.kde.org/show_bug.cgi?id=331485
** Text tool needs to be cleaned up. That thing is so hairy I'm guessing it should have its own Maniphest with subtasks.