Commit 65fa65a9 authored by Dmitry Kazakov's avatar Dmitry Kazakov

A HUGE refactoring of KisPaintOpBox

This patch introduces a lot of things:

1) Implements a proper model-view-controller design for all the paintop
   preset-based properties, such as opacity, flow and size:

      Model --- KisPaintOpSettings
      Controller --- KoResourceManager (also tracks dependencies among
                     the preset and its derived properties, such as
                     opacity, size, flow, composite op)
      View --- KisPaintOpBox.

   Basically, it means that KisPaintOpBox doesn't write to the settings
   directly anymore. Instead, it uses KoResourceManager for that.

2) Well, there are still a couple of flaws in the MVC design. E.g. the
   settings widgets still write directly into the settings bypassing the
   resource managers. To overcome this issue see the next bullet about
   KisResourceUpdateMediator. There is also a bigger problem, the size
   of the brush is still fetched through the access to the GUI elements,
   or, more precisely m_optionsWidget. I don't think it can be solved atm :(

3) Adds KisResourceUpdateMediator. This is a special class that allows
   the resource manager to track the changes in complex resources, such as
   KisPaintOpPreset. When such resource changes internaly, all the derived
   resources get notification, and if they are also changed, a corresponding
   signal is emitted.


There are still two regressions present:
1) Locked Settings functionality doesn't work
2) Resizing of the brush using Shift+Gesture is slow again :(
parent 183f82b7
......@@ -16,6 +16,7 @@ set(kritaflake_SRCS
KoCanvasBase.cpp
KoResourceManager_p.cpp
KoDerivedResourceConverter.cpp
KoResourceUpdateMediator.cpp
KoCanvasResourceManager.cpp
KoDocumentResourceManager.cpp
KoCanvasObserverBase.cpp
......
......@@ -180,3 +180,18 @@ void KoCanvasResourceManager::removeDerivedResourceConverter(int key)
{
d->manager.removeDerivedResourceConverter(key);
}
void KoCanvasResourceManager::addResourceUpdateMediator(KoResourceUpdateMediatorSP mediator)
{
d->manager.addResourceUpdateMediator(mediator);
}
bool KoCanvasResourceManager::hasResourceUpdateMediator(int key)
{
return d->manager.hasResourceUpdateMediator(key);
}
void KoCanvasResourceManager::removeResourceUpdateMediator(int key)
{
d->manager.removeResourceUpdateMediator(key);
}
......@@ -25,6 +25,7 @@
#include "kritaflake_export.h"
#include "KoDerivedResourceConverter.h"
#include "KoResourceUpdateMediator.h"
class KoShape;
class KoShapeStroke;
......@@ -250,6 +251,22 @@ public:
*/
void removeDerivedResourceConverter(int key);
/**
* @see KoReosurceManager::addResourceUpdateMediator
*/
void addResourceUpdateMediator(KoResourceUpdateMediatorSP mediator);
/**
* @see KoReosurceManager::hasResourceUpdateMediator
*/
bool hasResourceUpdateMediator(int key);
/**
* @see KoReosurceManager::removeResourceUpdateMediator
*/
void removeResourceUpdateMediator(int key);
Q_SIGNALS:
/**
* This signal is emitted every time a resource is set that is either
......
......@@ -19,7 +19,7 @@
#include "KoDerivedResourceConverter.h"
#include "QVariant"
#include "kis_assert.h"
struct KoDerivedResourceConverter::Private
{
......@@ -28,6 +28,8 @@ struct KoDerivedResourceConverter::Private
int key;
int sourceKey;
QVariant lastKnownValue;
};
......@@ -50,3 +52,42 @@ int KoDerivedResourceConverter::sourceKey() const
return m_d->sourceKey;
}
bool KoDerivedResourceConverter::notifySourceChanged(const QVariant &sourceValue)
{
const QVariant newValue = fromSource(sourceValue);
const bool valueChanged = m_d->lastKnownValue != newValue;
m_d->lastKnownValue = newValue;
return valueChanged;
}
QVariant KoDerivedResourceConverter::readFromSource(const QVariant &sourceValue)
{
const QVariant result = fromSource(sourceValue);
KIS_SAFE_ASSERT_RECOVER_NOOP(m_d->lastKnownValue.isNull() ||
result == m_d->lastKnownValue);
m_d->lastKnownValue = result;
return m_d->lastKnownValue;
}
QVariant KoDerivedResourceConverter::writeToSource(const QVariant &value,
const QVariant &sourceValue,
bool *changed)
{
if (changed) {
*changed = m_d->lastKnownValue != value;
}
QVariant newSourceValue = sourceValue;
if (*changed || value != fromSource(sourceValue)) {
m_d->lastKnownValue = value;
newSourceValue = toSource(value, sourceValue);
}
return newSourceValue;
}
......@@ -49,6 +49,14 @@ public:
int key() const;
int sourceKey() const;
QVariant readFromSource(const QVariant &value);
QVariant writeToSource(const QVariant &value,
const QVariant &sourceValue,
bool *changed);
virtual bool notifySourceChanged(const QVariant &sourceValue);
protected:
/**
* Converts the \p value of the source resource into the space of
* the "derived" resource. E.g. preset -> opacity.
......
......@@ -25,43 +25,68 @@
#include <FlakeDebug.h>
#include "KoShape.h"
#include "kis_assert.h"
#include "kis_debug.h"
void KoResourceManager::setResource(int key, const QVariant &value)
void KoResourceManager::slotResourceInternalsChanged(int key)
{
QVariant realValue = value;
int realKey = key;
QVariant currentValue = m_resources.value(key, QVariant());
KIS_SAFE_ASSERT_RECOVER_RETURN(m_resources.contains(key));
notifyDerivedResourcesChanged(key, m_resources[key]);
}
void KoResourceManager::setResource(int key, const QVariant &value)
{
KoDerivedResourceConverterSP converter =
m_derivedResources.value(key, KoDerivedResourceConverterSP());
if (converter) {
realKey = converter->sourceKey();
currentValue = m_resources.value(realKey, QVariant());
realValue = converter->toSource(value, currentValue);
}
const int sourceKey = converter->sourceKey();
const QVariant oldSourceValue = m_resources.value(sourceKey, QVariant());
bool valueChanged = false;
const QVariant newSourceValue = converter->writeToSource(value, oldSourceValue, &valueChanged);
if (valueChanged) {
notifyResourceChanged(key, value);
}
if (oldSourceValue != newSourceValue) {
m_resources[sourceKey] = newSourceValue;
notifyResourceChanged(sourceKey, newSourceValue);
}
if (m_resources.contains(realKey)) {
if (!converter && currentValue == realValue)
return;
m_resources[realKey] = realValue;
} else {
m_resources.insert(realKey, realValue);
}
const QVariant oldValue = m_resources.value(key, QVariant());
m_resources[key] = value;
notifyResourceChanged(key, value);
if (m_updateMediators.contains(key)) {
m_updateMediators[key]->connectResource(value);
}
if (oldValue != value) {
notifyResourceChanged(key, value);
}
}
}
void KoResourceManager::notifyResourceChanged(int key, const QVariant &value)
{
emit resourceChanged(key, value);
notifyDerivedResourcesChanged(key, value);
}
void KoResourceManager::notifyDerivedResourcesChanged(int key, const QVariant &value)
{
QMultiHash<int, KoDerivedResourceConverterSP>::const_iterator it = m_derivedFromSource.constFind(key);
QMultiHash<int, KoDerivedResourceConverterSP>::const_iterator end = m_derivedFromSource.constEnd();
while (it != end && it.key() == key) {
KoDerivedResourceConverterSP converter = it.value();
notifyResourceChanged(converter->key(), converter->fromSource(value));
if (converter->notifySourceChanged(value)) {
notifyResourceChanged(converter->key(), converter->readFromSource(value));
}
it++;
}
}
......@@ -74,7 +99,7 @@ QVariant KoResourceManager::resource(int key) const
const int realKey = converter ? converter->sourceKey() : key;
QVariant value = m_resources.value(realKey, QVariant());
return converter ? converter->fromSource(value) : value;
return converter ? converter->readFromSource(value) : value;
}
void KoResourceManager::setResource(int key, const KoColor &color)
......@@ -175,7 +200,7 @@ void KoResourceManager::clearResource(int key)
void KoResourceManager::addDerivedResourceConverter(KoDerivedResourceConverterSP converter)
{
Q_ASSERT(!m_derivedResources.contains(converter->key()));
KIS_SAFE_ASSERT_RECOVER_NOOP(!m_derivedResources.contains(converter->key()));
m_derivedResources.insert(converter->key(), converter);
m_derivedFromSource.insertMulti(converter->sourceKey(), converter);
......@@ -194,3 +219,21 @@ void KoResourceManager::removeDerivedResourceConverter(int key)
m_derivedResources.remove(key);
m_derivedFromSource.remove(converter->sourceKey(), converter);
}
void KoResourceManager::addResourceUpdateMediator(KoResourceUpdateMediatorSP mediator)
{
KIS_SAFE_ASSERT_RECOVER_NOOP(!m_updateMediators.contains(mediator->key()));
m_updateMediators.insert(mediator->key(), mediator);
connect(mediator.data(), SIGNAL(sigResourceChanged(int)), SLOT(slotResourceInternalsChanged(int)));
}
bool KoResourceManager::hasResourceUpdateMediator(int key)
{
return m_updateMediators.contains(key);
}
void KoResourceManager::removeResourceUpdateMediator(int key)
{
KIS_SAFE_ASSERT_RECOVER_RETURN(m_updateMediators.contains(key));
m_updateMediators.remove(key);
}
......@@ -29,6 +29,8 @@
#include <KoColor.h>
#include <KoUnit.h>
#include "KoDerivedResourceConverter.h"
#include "KoResourceUpdateMediator.h"
class KoShape;
class QVariant;
......@@ -144,7 +146,7 @@ public:
void clearResource(int key);
/**
* Some of the resources may be "derive" from the other. For
* Some of the resources may be "derived" from the others. For
* example opacity, composite op and erase mode properties are
* contained inside a paintop preset, so we need not create a
* separate resource for them. Instead we created a derived resource,
......@@ -152,7 +154,7 @@ public:
* "resource changed" signal (via a different key).
*
* When a parent resource changes, the resource manager emits
* update signals for all its derived resources.
* update signals for all its derived resources.
*/
void addDerivedResourceConverter(KoDerivedResourceConverterSP converter);
......@@ -172,11 +174,35 @@ public:
*/
void removeDerivedResourceConverter(int key);
/**
* Some resources can "mutate", that is they value doesn't change
* (a pointer), whereas the contents changes. But we should still
* notify all the derived resources subscribers that the resource
* has changed. For that purpose we use a special mediator class
* that connects the resource (which is not a QObject at all) and
* the resource manager controls that connection.
*/
void addResourceUpdateMediator(KoResourceUpdateMediatorSP mediator);
/**
* \see addResourceUpdateMediator()
*/
bool hasResourceUpdateMediator(int key);
/**
* \see addResourceUpdateMediator()
*/
void removeResourceUpdateMediator(int key);
Q_SIGNALS:
void resourceChanged(int key, const QVariant &value);
private:
void notifyResourceChanged(int key, const QVariant &value);
void notifyDerivedResourcesChanged(int key, const QVariant &value);
private Q_SLOTS:
void slotResourceInternalsChanged(int key);
private:
KoResourceManager(const KoResourceManager&);
......@@ -186,6 +212,8 @@ private:
QHash<int, KoDerivedResourceConverterSP> m_derivedResources;
QMultiHash<int, KoDerivedResourceConverterSP> m_derivedFromSource;
QHash<int, KoResourceUpdateMediatorSP> m_updateMediators;
};
#endif
......
/*
* 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 "KoResourceUpdateMediator.h"
struct KoResourceUpdateMediator::Private
{
Private(int _key) : key(_key) {}
int key;
};
KoResourceUpdateMediator::KoResourceUpdateMediator(int key)
: m_d(new Private(key))
{
}
KoResourceUpdateMediator::~KoResourceUpdateMediator()
{
}
int KoResourceUpdateMediator::key() const
{
return m_d->key;
}
/*
* 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 __KO_RESOURCE_UPDATE_MEDIATOR_H
#define __KO_RESOURCE_UPDATE_MEDIATOR_H
#include <QScopedPointer>
#include <QSharedPointer>
#include "kritaflake_export.h"
/**
* A special mediator class that connects the resource and the
* resource manager. The resource manager connects to a
* sigResourceChanged() changed and when a resource changes, the
* manager calls connectResource() for this resource. After that, the
* mediator should notify the manager about every change that happens
* to the resource by emitting the corresponding signal.
*
* There is only one mediator for one type (key) of the resource.
*/
class KRITAFLAKE_EXPORT KoResourceUpdateMediator : public QObject
{
Q_OBJECT
public:
KoResourceUpdateMediator(int key);
virtual ~KoResourceUpdateMediator();
int key() const;
virtual void connectResource(QVariant sourceResource) = 0;
Q_SIGNALS:
void sigResourceChanged(int key);
private:
struct Private;
const QScopedPointer<Private> m_d;
};
typedef QSharedPointer<KoResourceUpdateMediator> KoResourceUpdateMediatorSP;
#endif /* __KO_RESOURCE_UPDATE_MEDIATOR_H */
......@@ -26,6 +26,8 @@
#include <QSignalSpy>
#include <QTest>
#include "kis_debug.h"
void TestResourceManager::koShapeResource()
{
KoPathShape * shape = new KoPathShape();
......@@ -118,27 +120,270 @@ void TestResourceManager::testDerivedChanged()
m.setResource(derivedKey, 16);
QCOMPARE(spy.count(), 1);
QCOMPARE(spy.count(), 3);
QList<QVariant> args;
args = spy[0];
QCOMPARE(args[0].toInt(), derivedKey);
QCOMPARE(args[1].toInt(), 16);
args = spy[1];
QCOMPARE(args[0].toInt(), key2);
QCOMPARE(args[1].toInt(), 6);
args = spy[2];
QCOMPARE(args[0].toInt(), otherDerivedKey);
QCOMPARE(args[1].toInt(), 16);
spy.clear();
m.setResource(key2, 7);
QCOMPARE(spy.count(), 4);
QCOMPARE(spy.count(), 3);
args = spy[1];
args = spy[0];
QCOMPARE(args[0].toInt(), key2);
QCOMPARE(args[1].toInt(), 7);
args = spy[2];
args = spy[1];
QCOMPARE(args[0].toInt(), otherDerivedKey);
QCOMPARE(args[1].toInt(), 17);
args = spy[3];
args = spy[2];
QCOMPARE(args[0].toInt(), derivedKey);
QCOMPARE(args[1].toInt(), 17);
}
struct ComplexResource {
QHash<int, QVariant> m_resources;
};
typedef QSharedPointer<ComplexResource> ComplexResourceSP;
Q_DECLARE_METATYPE(ComplexResourceSP);
struct ComplexConverter : public KoDerivedResourceConverter
{
ComplexConverter(int key, int sourceKey) : KoDerivedResourceConverter(key, sourceKey) {}
QVariant fromSource(const QVariant &value) {
KIS_ASSERT(value.canConvert<ComplexResourceSP>());
ComplexResourceSP res = value.value<ComplexResourceSP>();
return res->m_resources[key()];
}
QVariant toSource(const QVariant &value, const QVariant &sourceValue) {
KIS_ASSERT(sourceValue.canConvert<ComplexResourceSP>());
ComplexResourceSP res = sourceValue.value<ComplexResourceSP>();
res->m_resources[key()] = value;
return QVariant::fromValue(res);
}
};
struct ComplexMediator : public KoResourceUpdateMediator
{
ComplexMediator(int key) : KoResourceUpdateMediator(key) {}
void connectResource(QVariant sourceResource) {
m_res = sourceResource;
}
void forceNotify() {
emit sigResourceChanged(key());
}
QVariant m_res;
};
typedef QSharedPointer<ComplexMediator> ComplexMediatorSP;
void TestResourceManager::testComplexResource()
{
const int key = 2;
const int complex1 = 3;
const int complex2 = 4;
KoCanvasResourceManager m(0);
m.addDerivedResourceConverter(toQShared(new ComplexConverter(complex1, key)));
m.addDerivedResourceConverter(toQShared(new ComplexConverter(complex2, key)));
ComplexMediatorSP mediator(new ComplexMediator(key));
m.addResourceUpdateMediator(mediator);
QSignalSpy spy(&m, SIGNAL(canvasResourceChanged(int, const QVariant &)));
ComplexResourceSP r1(new ComplexResource());
r1->m_resources[complex1] = 10;
r1->m_resources[complex2] = 20;
ComplexResourceSP r2(new ComplexResource());
r2->m_resources[complex1] = 15;
r2->m_resources[complex2] = 25;
// ####################################################
// Initial assignment
// ####################################################
m.setResource(key, QVariant::fromValue(r1));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 20);
QCOMPARE(spy[0][0].toInt(), key);
QCOMPARE(spy[0][1].value<ComplexResourceSP>(), r1);
QCOMPARE(spy[1][0].toInt(), complex2);
QCOMPARE(spy[1][1].toInt(), 20);
QCOMPARE(spy[2][0].toInt(), complex1);
QCOMPARE(spy[2][1].toInt(), 10);
spy.clear();
// ####################################################
// Change the whole resource
// ####################################################
m.setResource(key, QVariant::fromValue(r2));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(complex1).toInt(), 15);
QCOMPARE(m.resource(complex2).toInt(), 25);
QCOMPARE(spy[0][0].toInt(), key);
QCOMPARE(spy[0][1].value<ComplexResourceSP>(), r2);
QCOMPARE(spy[1][0].toInt(), complex2);
QCOMPARE(spy[1][1].toInt(), 25);
QCOMPARE(spy[2][0].toInt(), complex1);
QCOMPARE(spy[2][1].toInt(), 15);
spy.clear();
// ####################################################
// Change a derived resource
// ####################################################
m.setResource(complex1, 16);
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(complex1).toInt(), 16);
QCOMPARE(m.resource(complex2).toInt(), 25);
QCOMPARE(spy[0][0].toInt(), complex1);
QCOMPARE(spy[0][1].toInt(), 16);
spy.clear();
// ####################################################
// Change another derived resource
// ####################################################
m.setResource(complex2, 26);
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(complex1).toInt(), 16);
QCOMPARE(m.resource(complex2).toInt(), 26);
QCOMPARE(spy[0][0].toInt(), complex2);
QCOMPARE(spy[0][1].toInt(), 26);
spy.clear();
// ####################################################
// Switch back the whole source resource
// ####################################################
m.setResource(key, QVariant::fromValue(r1));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 20);
QCOMPARE(spy[0][0].toInt(), key);
QCOMPARE(spy[0][1].value<ComplexResourceSP>(), r1);
QCOMPARE(spy[1][0].toInt(), complex2);
QCOMPARE(spy[1][1].toInt(), 20);
QCOMPARE(spy[2][0].toInt(), complex1);
QCOMPARE(spy[2][1].toInt(), 10);
spy.clear();
// ####################################################
// The value keep unchanged case!
// ####################################################
m.setResource(complex1, 10);
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 20);
QCOMPARE(spy.size(), 0);
spy.clear();
// ####################################################
// While switching a complex resource one derived value
// is kept unchanged
// ####################################################
r2->m_resources[complex1] = 10;
m.setResource(key, QVariant::fromValue(r2));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r2);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 26);
QCOMPARE(spy[0][0].toInt(), key);
QCOMPARE(spy[0][1].value<ComplexResourceSP>(), r2);
QCOMPARE(spy[1][0].toInt(), complex2);
QCOMPARE(spy[1][1].toInt(), 26);
spy.clear();
// ####################################################
// No devived values are changed!
// ####################################################
*r1 = *r2;
m.setResource(key, QVariant::fromValue(r1));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 26);
QCOMPARE(spy[0][0].toInt(), key);
QCOMPARE(spy[0][1].value<ComplexResourceSP>(), r1);
spy.clear();
// ####################################################
// Try to set the same pointer. No signals emitted!
// ####################################################
m.setResource(key, QVariant::fromValue(r1));
QCOMPARE(mediator->m_res.value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(key).value<ComplexResourceSP>(), r1);
QCOMPARE(m.resource(complex1).toInt(), 10);
QCOMPARE(m.resource(complex2).toInt(), 26);
QCOMPARE(spy.size(), 0);
spy.clear();
// ####################################################
// Internals 'officially' changed, but the values not
// ####################################################
mediator->forceNotify();
QCOMPARE(spy.size(), 0);
spy.clear();
// ####################################################
// We changed the values, but didn't notify anyone :)