Krita/QActions

Actions in Krita

QActions are an abstraction to associate some data like name, icon, and tooltip with a callback method, in order to provide an interface for widgets and keyboard shortcuts.

There has never been guidelines of how to use these until now, and because they can be easily tacked on and forgotten about, the system is prone to rotting. To counteract this, a coherent manager for actions in Krita has been designed. Because the system will remain complex even after existing code is cleaned up, this page also documents the way things are currently done.

I will use the term "action" to refer to objects that inherit from QAction. This does not include the classes PrimaryAction/SecondaryAction, which could perhaps stand a bit of renaming when they are refactored in the future.

New System

All action metadata should be contained in one of the .action XML files. Each action should be identified by a unique name. Otherwise these are in a somewhat flexible "long" format. Much of the rationale of the format is the extractrc program used to extract translation strings. In particular, only text contained in fields <text></text>, <tooltip></tooltip> and some others will be translated.

The global static class KisActionRegistry reads these data files. The class can serve as a factory of actions.

To create a QAction, most of the time, we want to use the method

QAction *action = KisActionRegistry::instance()->makeQAction("name");

Calls like action->setTooltip(i18n("tooltip")); should be completely obviated. The latter is an example of static data which should be removed from the code out to the .action description files. KisActionRegistry will also have a template method for creating derived classes of actions, such as QWidgetActions.

Right now, KisActionRegistry exposes the DOM data, though that might perhaps better be encapsulated another way. The system should be robust against requests for a property that is not in the data, e.g. requesting the shortcut for an action that has no <shortcut> tag.

Saving custom shortcut schemes is next up on the to-do list.

Current Issues

There are general ideas of how the actions system works, but no hard and fast rules.

There are many distinct groups of actions inside Krita.

Collections of actions are stored in a Widgetutils/XMLGUIClient class called KActionCollection. This class has an enormous amount of structure and functionality, to the point which it can be annoying. This is ripe for refactoring, if you ever feel like doing so.

The main KActionCollection is owned by the KisMainWindow, since that class inherits from XMLGUIClient. This main collection gets passed around and referenced from tons and tons of different clients. This is the one we share with KisActionManager, KisViewManager, all the KisXxxManagers inject actions into this collection. (KisViewManager->actionCollection() is referenced 48 times in the code base.) The behavior of this when multiple main windows are open will be discussed later.

There are a number of QActions that are associated with tools. These show up in the Flake library. Tools have their own action collections, their own system (ToolHelper) and their own interface for requesting actions. This is sketchy and probably does no good anymore, since Krita can now use its own action abstractions. Tool actions are also registered to the MainWindow subsystem already in a questionable way.

KStandardActions is supposed to provide a repository of universal actions, like "Save As...". We have our own copy inside of Widgetutils and it doesn't play nice with our system. It should be fixed.

KisActions are a derived class. They use callbacks when they are executed, have ActivationFlags/ActivationConditions to enable/disable actions based on the state of the program.

KisMainWindowObserver is a class that tracks a main window around. It should be examined more.

Macros

Information about the macro subsystem:

[17:47] <slangkamp> abrahams: well the idea behind it was that we could identify operations with ids
[17:48] <slangkamp> abrahams: some operation might be executed from the gui with an action or by a macro functionality
[17:51] <slangkamp> abrahams: if you look e.g. at KisSelectionManager there is a lot of code in the form:
[17:51] <slangkamp>     KisSelectAllActionFactory factory;
[17:51] <slangkamp>     factory.run(m_view);
[17:52] <slangkamp> Usually that code should not exist, but it should get the factory from a registry
[17:53] <slangkamp> so there are two cases action calls it and the ui is put into a dialog
[17:53] <slangkamp> or macro editor uses it (which doesn't exist yet)

Info from Phabricator

This is mostly discussing the way things were, does not reflect the updates, the new system is W

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.