Commit e1d0a836 authored by Dmitry Kazakov's avatar Dmitry Kazakov
Browse files

Fixed multiple shortcuts bugs

1) Standard actions, like Copy/Paste are now propertized as well.
   Otherwise, their custom shortcuts were never used.

2) ActionInfoItem now has a special field showing that the action
   is explicitly reset to null. This way we will not break it
   accidentally, when refactoring empty strings in the future
   (like it happened this time).

3) Split up KisActionRegistry and KisActionsSnapshot. The former
   is a singleton and used for propertizing the actions, but the
   latter one is just a snapshot used for filling up the settings
   dialog.

BUG:373184,372198
parent 327138fe
......@@ -107,20 +107,6 @@ public:
void loadActions();
};
// Basically, we are going to insert the current UI/MainWindow ActionCollection
// into the KisActionRegistry.
void KisPart::loadActions()
{
d->actionCollection = currentMainwindow()->viewManager()->actionCollection();
KisActionRegistry * actionRegistry = KisActionRegistry::instance();
Q_FOREACH (auto action, d->actionCollection->actions()) {
auto name = action->objectName();
actionRegistry->addAction(action->objectName(), action);
}
};
KisPart* KisPart::instance()
{
return s_instance;
......
......@@ -133,11 +133,6 @@ public:
*/
KisMainWindow *currentMainwindow() const;
/**
* Load actions for currently active main window into KisActionRegistry.
*/
void loadActions();
/**
* @return the application-wide KisIdleWatcher.
*/
......
......@@ -262,6 +262,9 @@ void GeneralTab::clearBackgroundImage()
m_backgroundimage->setText("");
}
#include "kactioncollection.h"
#include "KisActionsSnapshot.h"
ShortcutSettingsTab::ShortcutSettingsTab(QWidget *parent, const char *name)
: QWidget(parent)
{
......@@ -272,8 +275,26 @@ ShortcutSettingsTab::ShortcutSettingsTab(QWidget *parent, const char *name)
m_page = new WdgShortcutSettings(this);
l->addWidget(m_page, 0, 0);
KisPart::instance()->loadActions();
KisActionRegistry::instance()->setupDialog(m_page);
m_snapshot.reset(new KisActionsSnapshot);
KActionCollection *collection =
KisPart::instance()->currentMainwindow()->actionCollection();
Q_FOREACH (QAction *action, collection->actions()) {
m_snapshot->addAction(action->objectName(), action);
}
QMap<QString, KActionCollection*> sortedCollections =
m_snapshot->actionCollections();
for (auto it = sortedCollections.constBegin(); it != sortedCollections.constEnd(); ++it) {
m_page->addCollection(it.value(), it.key());
}
}
ShortcutSettingsTab::~ShortcutSettingsTab()
{
}
void ShortcutSettingsTab::setDefault()
......
......@@ -105,6 +105,8 @@ public:
{ }
};
class KisActionsSnapshot;
class ShortcutSettingsTab : public QWidget
{
Q_OBJECT
......@@ -112,10 +114,12 @@ class ShortcutSettingsTab : public QWidget
public:
ShortcutSettingsTab(QWidget *parent = 0, const char *name = 0);
~ShortcutSettingsTab();
public:
void setDefault();
WdgShortcutSettings *m_page;
QScopedPointer<KisActionsSnapshot> m_snapshot;
public Q_SLOTS:
......
......@@ -62,18 +62,6 @@ KisAction::~KisAction()
delete d;
}
KisAction *makeKisAction(QString name, QObject *parent)
{
KisAction* a = new KisAction(parent);
KisActionRegistry::instance()->propertizeAction(name, a);
KisActionRegistry::instance()->addAction(name, a);
// TODO: Add other static data (activationFlags, etc.) using getActionXml()
return a;
}
// Using a dynamic QObject property is done for compatibility with KAction and
// XmlGui. We may merge KisAction into the XmlGui code to make this unnecessary,
// but that is probably a lot of work for little benefit. We currently store a
......
......@@ -84,11 +84,6 @@ public:
KisAction(const QIcon& icon, const QString& text, QObject* parent = 0);
virtual ~KisAction();
/**
* Produces a new KisAction based on .action data files.
*/
static KisAction *makeKisAction(QString name, QObject *parent);
void setDefaultShortcut(const QKeySequence & shortcut);
QKeySequence defaultShortcut() const;
......
......@@ -129,12 +129,10 @@ void KisActionManager::addAction(const QString& name, KisAction* action)
Q_ASSERT(d->viewManager->actionCollection());
d->viewManager->actionCollection()->addAction(name, action);
action->setObjectName(name);
action->setParent(d->viewManager->actionCollection());
d->viewManager->actionCollection()->setDefaultShortcut(action, action->defaultShortcut());
d->actions.append(action);
action->setActionManager(this);
KisActionRegistry::instance()->addAction(name, action);
}
void KisActionManager::takeAction(KisAction* action)
......@@ -170,11 +168,10 @@ KisAction *KisActionManager::createAction(const QString &name)
// will add them to the KisActionRegistry for the time being so we can get
// properly categorized shortcuts.
a = new KisAction();
auto actionRegistry = KisActionRegistry::instance();
KisActionRegistry *actionRegistry = KisActionRegistry::instance();
// Add extra properties
actionRegistry->propertizeAction(name, a);
actionRegistry->addAction(name, a);
bool ok; // We will skip this check
int activationFlags = actionRegistry->getActionProperty(name, "activationFlags").toInt(&ok, 2);
int activationConditions = actionRegistry->getActionProperty(name, "activationConditions").toInt(&ok, 2);
......@@ -354,6 +351,9 @@ KisAction *KisActionManager::createStandardAction(KStandardAction::StandardActio
}
}
KisActionRegistry *actionRegistry = KisActionRegistry::instance();
actionRegistry->propertizeAction(standardAction->objectName(), action);
addAction(standardAction->objectName(), action);
delete standardAction;
return action;
......
......@@ -11,6 +11,7 @@ set(kritawidgetutils_LIB_SRCS
kis_icon_utils.cpp
kis_action_registry.cpp
KisActionsSnapshot.cpp
KoGroupButton.cpp
KoProgressBar.cpp
KoProgressUpdater.cpp
......
/*
* Copyright (c) 2016 Dmitry Kazakov <dimula73@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#include "KisActionsSnapshot.h"
#include "kis_action_registry.h"
#include "kactioncollection.h"
#include "kis_debug.h"
struct KisActionsSnapshot::Private
{
QMap<QString, KActionCollection*> actionCollections;
~Private() {
qDeleteAll(actionCollections);
}
};
KisActionsSnapshot::KisActionsSnapshot()
: m_d(new Private)
{
}
KisActionsSnapshot::~KisActionsSnapshot()
{
}
void KisActionsSnapshot::addAction(const QString &name, QAction *action)
{
KisActionRegistry::ActionCategory cat = KisActionRegistry::instance()->fetchActionCategory(name);
if (!cat.isValid()) {
warnKrita << "WARNING: Uncategorized action" << name << "Dropping...";
return;
}
KActionCollection *collection = m_d->actionCollections[cat.componentName];
if (!collection) {
collection = new KActionCollection(0, cat.componentName);
m_d->actionCollections.insert(cat.componentName, collection);
}
collection->addCategorizedAction(name, action, cat.categoryName);
}
QMap<QString, KActionCollection *> KisActionsSnapshot::actionCollections() const
{
return m_d->actionCollections;
}
/*
* Copyright (c) 2016 Dmitry Kazakov <dimula73@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#ifndef KISACTIONSSNAPSHOT_H
#define KISACTIONSSNAPSHOT_H
#include <kritawidgetutils_export.h>
#include <QScopedPointer>
#include <QMap>
class QAction;
class KActionCollection;
class KRITAWIDGETUTILS_EXPORT KisActionsSnapshot
{
public:
KisActionsSnapshot();
~KisActionsSnapshot();
/**
* @brief registers the action in the snapshot and sorts it into a proper
* category. The action is *not* owned by the snapshot.
*
* @param name id string of the action
* @param action the action itself
*/
void addAction(const QString &name, QAction *action);
/**
* Returns all action collections of the current snapshot
*
* WARNING: the collections are owned by the shapshot! Don't destroy
* the snapshot before you are done with the collections!
*/
QMap<QString, KActionCollection*> actionCollections() const;
private:
struct Private;
const QScopedPointer<Private> m_d;
};
#endif // KISACTIONSSNAPSHOT_H
......@@ -30,8 +30,6 @@
#include "kis_debug.h"
#include "KoResourcePaths.h"
#include "kis_icon_utils.h"
#include "kactioncollection.h"
#include "kactioncategory.h"
#include "kis_action_registry.h"
#include "kshortcutschemeshelper_p.h"
......@@ -49,10 +47,37 @@ namespace {
*/
struct ActionInfoItem {
QDomElement xmlData;
QList<QKeySequence> defaultShortcuts;
QList<QKeySequence> customShortcuts;
QString collectionName;
QString categoryName;
inline QList<QKeySequence> defaultShortcuts() const {
return m_defaultShortcuts;
}
inline void setDefaultShortcuts(const QList<QKeySequence> &value) {
m_defaultShortcuts = value;
}
inline QList<QKeySequence> customShortcuts() const {
return m_customShortcuts;
}
inline void setCustomShortcuts(const QList<QKeySequence> &value, bool explicitlyReset) {
m_customShortcuts = value;
m_explicitlyReset = explicitlyReset;
}
inline QList<QKeySequence> effectiveShortcuts() const {
return m_customShortcuts.isEmpty() && !m_explicitlyReset ?
m_defaultShortcuts : m_customShortcuts;
}
private:
QList<QKeySequence> m_defaultShortcuts;
QList<QKeySequence> m_customShortcuts;
bool m_explicitlyReset = false;
};
// Convenience macros to extract text of a child node.
......@@ -78,17 +103,7 @@ namespace {
}
return translatedString;
};
QList<QKeySequence> preferredShortcuts(ActionInfoItem action) {
if (action.customShortcuts.isEmpty()) {
return action.defaultShortcuts;
} else {
return action.customShortcuts;
}
};
}
};
......@@ -112,7 +127,6 @@ public:
};
KisActionRegistry *q;
QMap<QString, KActionCollection*> actionCollections;
};
......@@ -134,37 +148,23 @@ KisActionRegistry::KisActionRegistry()
loadCustomShortcuts();
}
void KisActionRegistry::addAction(const QString &name, QAction *a)
KisActionRegistry::ActionCategory KisActionRegistry::fetchActionCategory(const QString &name) const
{
auto info = d->actionInfo(name);
KActionCollection *collection = d->actionCollections.value(info.collectionName);
if (!collection) {
dbgAction << "No collection found for action" << name;
return;
}
if (collection->action(name)) {
dbgAction << "duplicate action" << name << "in collection" << collection->componentName();
}
else {
}
collection->addCategorizedAction(name, a, info.categoryName);
};
if (!d->actionInfoList.contains(name)) return ActionCategory();
const ActionInfoItem info = d->actionInfoList.value(name);
return ActionCategory(info.collectionName, info.categoryName);
}
void KisActionRegistry::notifySettingsUpdated()
{
d->loadCustomShortcuts();
};
}
void KisActionRegistry::loadCustomShortcuts(const QString &path)
void KisActionRegistry::loadCustomShortcuts()
{
if (path.isEmpty()) {
d->loadCustomShortcuts();
} else {
d->loadCustomShortcuts(path);
}
};
d->loadCustomShortcuts();
}
void KisActionRegistry::loadShortcutScheme(const QString &schemeName)
{
......@@ -184,24 +184,16 @@ void KisActionRegistry::loadShortcutScheme(const QString &schemeName)
QAction * KisActionRegistry::makeQAction(const QString &name, QObject *parent)
{
QAction * a = new QAction(parent);
if (!d->actionInfoList.contains(name)) {
dbgAction << "Warning: requested data for unknown action" << name;
return a;
}
propertizeAction(name, a);
return a;
};
void KisActionRegistry::setupDialog(KisShortcutsDialog *dlg)
{
for (auto i = d->actionCollections.constBegin(); i != d->actionCollections.constEnd(); i++ ) {
dlg->addCollection(i.value(), i.key());
}
}
void KisActionRegistry::settingsPageSaved()
{
// For now, custom shortcuts are dealt with by writing to file and reloading.
......@@ -226,8 +218,7 @@ void KisActionRegistry::applyShortcutScheme(const KConfigBase *config)
auto it = schemeEntries.constBegin();
while (it != schemeEntries.end()) {
ActionInfoItem &info = d->actionInfo(it.key());
if (!it.value().isEmpty())
info.defaultShortcuts = QKeySequence::listFromString(it.value());
info.setDefaultShortcuts(QKeySequence::listFromString(it.value()));
it++;
}
}
......@@ -236,71 +227,46 @@ void KisActionRegistry::applyShortcutScheme(const KConfigBase *config)
void KisActionRegistry::updateShortcut(const QString &name, QAction *action)
{
const ActionInfoItem &info = d->actionInfo(name);
action->setShortcuts(preferredShortcuts(info));
action->setProperty("defaultShortcuts", qVariantFromValue(info.defaultShortcuts));
action->setShortcuts(info.effectiveShortcuts());
action->setProperty("defaultShortcuts", qVariantFromValue(info.defaultShortcuts()));
}
bool KisActionRegistry::propertizeAction(const QString &name, QAction * a)
{
const ActionInfoItem info = d->actionInfo(name);
QDomElement actionXml = info.xmlData;
if (actionXml.text().isEmpty()) {
if (!d->actionInfoList.contains(name)) {
dbgAction << "No XML data found for action" << name;
return false;
}
const ActionInfoItem info = d->actionInfo(name);
// i18n requires converting format from QString.
auto getChildContent_i18n = [=](QString node){return quietlyTranslate(getChildContent(actionXml, node));};
// Note: the fields in the .action documents marked for translation are determined by extractrc.
QString icon = getChildContent(actionXml, "icon");
QString text = getChildContent_i18n("text");
QString whatsthis = getChildContent_i18n("whatsThis");
QString toolTip = getChildContent_i18n("toolTip");
QString statusTip = getChildContent_i18n("statusTip");
QString iconText = getChildContent_i18n("iconText");
bool isCheckable = getChildContent(actionXml, "isCheckable") == QString("true");
a->setObjectName(name); // This is helpful, should be added more places in Krita
a->setIcon(KisIconUtils::loadIcon(icon.toLatin1()));
a->setText(text);
a->setObjectName(name);
a->setWhatsThis(whatsthis);
a->setToolTip(toolTip);
a->setStatusTip(statusTip);
a->setIconText(iconText);
a->setCheckable(isCheckable);
QDomElement actionXml = info.xmlData;
if (!actionXml.text().isEmpty()) {
// i18n requires converting format from QString.
auto getChildContent_i18n = [=](QString node){return quietlyTranslate(getChildContent(actionXml, node));};
// Note: the fields in the .action documents marked for translation are determined by extractrc.
QString icon = getChildContent(actionXml, "icon");
QString text = getChildContent_i18n("text");
QString whatsthis = getChildContent_i18n("whatsThis");
QString toolTip = getChildContent_i18n("toolTip");
QString statusTip = getChildContent_i18n("statusTip");
QString iconText = getChildContent_i18n("iconText");
bool isCheckable = getChildContent(actionXml, "isCheckable") == QString("true");
a->setObjectName(name); // This is helpful, should be added more places in Krita
a->setIcon(KisIconUtils::loadIcon(icon.toLatin1()));
a->setText(text);
a->setObjectName(name);
a->setWhatsThis(whatsthis);
a->setToolTip(toolTip);
a->setStatusTip(statusTip);
a->setIconText(iconText);
a->setCheckable(isCheckable);
}
updateShortcut(name, a);
// TODO: check for colliding shortcuts in .action files either here or in loading code
#if 0
QMap<QKeySequence, QAction*> existingShortcuts;
Q_FOREACH (QAction* action, actionCollection->actions()) {
if(action->shortcut() == QKeySequence(0)) {
continue;
}
if (existingShortcuts.contains(action->shortcut())) {
dbgAction << QString("Actions %1 and %2 have the same shortcut: %3") \
.arg(action->text()) \
.arg(existingShortcuts[action->shortcut()]->text()) \
.arg(action->shortcut());
}
else {
existingShortcuts[action->shortcut()] = action;
}
}
#endif
return true;
}
......@@ -341,16 +307,6 @@ void KisActionRegistry::Private::loadActionFiles()
continue;
}
KActionCollection *actionCollection;
if (!actionCollections.contains(collectionName)) {
actionCollection = new KActionCollection(q, collectionName);
actionCollections.insert(collectionName, actionCollection);
dbgAction << "Adding a new action collection " << collectionName;
} else {
actionCollection = actionCollections.value(collectionName);
}
// Loop over <Actions> nodes. Each of these corresponds to a
// KActionCategory, producing a group of actions in the shortcut dialog.
QDomElement actions = base.firstChild().toElement();
......@@ -359,8 +315,6 @@ void KisActionRegistry::Private::loadActionFiles()
// <text> field
QDomElement categoryTextNode = actions.firstChild().toElement();
QString categoryName = quietlyTranslate(categoryTextNode.text());
// KActionCategory *category = actionCollection->getCategory(categoryName);
// dbgAction << "Using category" << categoryName;
// <action></action> tags
QDomElement actionXml = categoryTextNode.nextSiblingElement();
......@@ -386,8 +340,9 @@ void KisActionRegistry::Private::loadActionFiles()
// Use empty list to signify no shortcut
QString shortcutText = getChildContent(actionXml, "shortcut");
if (!shortcutText.isEmpty())
info.defaultShortcuts << QKeySequence(shortcutText);
if (!shortcutText.isEmpty()) {
info.setDefaultShortcuts(QKeySequence::listFromString(shortcutText));
}
info.categoryName = categoryName;
info.collectionName = collectionName;
......@@ -401,7 +356,7 @@ void KisActionRegistry::Private::loadActionFiles()
actions = actions.nextSiblingElement();
}
}
};
}
void KisActionRegistry::Private::loadCustomShortcuts(QString filename)
{
......@@ -417,15 +372,28 @@ void KisActionRegistry::Private::loadCustomShortcuts(QString filename)
if (localShortcuts.hasKey(i.key())) {
QString entry = localShortcuts.readEntry(i.key(), QString());
if (entry == QStringLiteral("none")) {