Preset widgets refactoring

[Dmitry Kazakov]

Hi, Stefano!


On Wed, Jul 1, 2015 at 7:38 PM, Stefano Bonicatti <smjert at gmail.com> wrote:

>
> The idea to solve this is first of all to have them be only widget, so
> they take input and display something, but they should have the less logic
> possible.
> This means having a central class that deals with presets, this class
> contains the current selected preset that must be in sync with the other
> widgets.
>

Technically speaking we have one. It is KisCanvasResourceProvider (aka
KoCanvasResourceManager). But its main problem is that when it emits a
"preset-changed" signal we cannot distinguish who has initiated one. We
also have KisAcyclicSignalConnector class. I'm not sure it helps you here,
but the idea is the same. It breaks loops in the updating process.


> This has the ultimate decision of which preset is the current one.
>

 Well, KisCanvasResourceProvider is the one.

Widgets will only speak with this class about selected resources so that
> the class know where the events come from and know when to ignore or send
> an update to the other widgets.
> For this to work the widgets can give updates to this class through
> signals but the class itself will directly call widgets functions, not
> through signals.
> This means a widget will register to this class and thanks to a common
> interface the class will be able to speak with the widget.
> This is to avoid having the class sending signals that are caught by every
> widget, when it should've been caught by only 2, where then the third has
> to know, somehow, that it should discard that event.
>

Really looks like what KisAcyclicSignalConnector was created for.



TL;DR:
> 1) We have issues with widgets presets being too smart and fighting each
> other when selecting a preset, especially between the preset docker, preset
> popup and the preset strip.
> 2) We don't distinguish call paths that comes from or signals that are
> emitted due to UI or non UI events.This leads to problems because in some
> cases creates loops (that are dealt with but in a way that creates other
> functionality issues).We should split these paths by using different
> functions and different signals and we should be sure that one can't
> trigger the other.
> 3) We have generic signals (canvasResourceChanged) that are used to keep
> track of specific things (presets), but we don't check if this specific
> thing is really changed and we do unnecessary work that might also create
> functionality issues.
> 4) We have reuse of signals beyond scope, like resourceRemoved to force an
> update of the resource model, but this triggers code that it shouldn't be
> called in some cases. We need to have specific signals for specific
> operations.
> 5) We have fighting occurring between higher classes and base classes too,
> about the resource selection (preset widgets fighting with
> KoResourceItemChooser etc).We probably should have base classes that are
> more dumb than we have now.
> Solution: a part from the ones given in the list, having a central class
> that deals with keeping track of the current preset and talking with the
> widgets avoiding loops or avoiding to call widgets function due to signal
> sent by those same widgets, i think, solves this issues.Widgets should only
> take input and display results, they will only be able to directly affect
> this central class through signals and only very indirectly and with the
> necessary controls, the other widgets.
> This is already quite long so i will end this here, but is necessary to
> understand the need of the refactoring and also the fact that it can't
> really be done in steps.. because there are a lot of places that are
> failing and they are all connected so we need to fix all those
> together.Next step from me is to obtain a sort of graph of interactions to
> have in front of the eyes what currently happens, between all the classes
> involved, when a preset is selected, or what happens when something
> generate a preset selection.


Well, central class idea is fine, except that we already have a central
class, KisCanavsResourceProvider. Though you can add one class below it,
yes.

I guess that the refactoring can be done in kind of "steps". You need to
define a common interface for such widget objects. And probably consider
using KisAcyclicSignalConnector. The latter one will allow you to avoid
special hardcoded function calls breaking the loops. After you defined a
new interface, you can wrap in into a special object, which is technically
mediator [0] for KisCanavsResourceProvider. After you have your mediator
implemented you can clean the usages of old signals in the widgets one by
one.


[0] - https://en.wikipedia.org/wiki/Mediator_pattern

-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20150704/2ff75c5e/attachment.html>