Preset widgets refactoring

Wed Jul 1 15:38:29 UTC 2015

Hello everyone, today i'm going to talk you about the preset widgets,
signals between them and about signals a bit more generally.

In Krita right now we have this issue where selecting a preset from a
widget is quite problematic because we don't have only one widget that can
select them, but we have 3 and each of them is "smart" enough to force
Krita to set "their" chosen preset.
What it can happen is that widget A asks for a preset to be selected,
widget B, through signals, receives this event but it doesn't distinguish
between an event that came from it's own ui/input from one from a signal
emitted by someone else, so it uses the same code and sends again (as
widget A does) the same signal, or at least it tries to.
In some specific cases this still work because there are some blockSignals
here and there to limit issues, but it works as a double edged sword.

Then we have a widget with the same base functionality of the others but
that it behaves a little different, and i'm talking about the preset strip
in the brush editor.
That widget as soon as a different brush engine is selected, it has to
relayout the resources and automatically select a resource from the new
list of possible presets.
This is all fine if we are actually in the brush editor, selecting
different preset or different brush engines, but it's not right when we
select a preset from the preset docker that is not present in the current
brush engine.
This activates the preset strip, which calls a relayout and this might end
up trying to select a different preset than the one originally selected.

So there's a continuous fight between these widgets.
Right now it apparently all works due to some black magic and again
blockSignals (or checks to custom boolean when launching an event) to avoid
these loops.. but when trying to fix the wrong selection when choosing a
different tag category in the preset docker, all this come up to the
surface.

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.
This has the ultimate decision of which preset is the current 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.

Another important thing is that we should split signals and call paths much
more, and this is true in many other cases.
But especially with widgets we should distinguish when a signal is emitted
due to an UI event and when it's due to and indirect call, from an UI event
happened somewhere else (one example is the
function KoResourceItemChooser::activated which is called by both UI and
non UI code and it also emits an important, and equal in both cases, signal
that puts into contact the chooser with the higher Krita widgets).
This, in my idea, it's already partially done by having the widgets sending
a signal that is handled by the central class only and then this class
directly calls some widgets internal function to update it, that is
different from the one that is used when the resource selection happens in
the widget itself (more than else this function shouldn't emit any signal
regarding resource selection).

We also have generic signals like canvasResourceChanged which are used to
trigger code that should be run in specific cases, not in every canvas
update, and we don't check if what we are really interested to (in this
case the current preset) is really changed before calling functions that
goes down the class hierarchy, down back to the base classes that,
indirectly, generated that signal, potentially creating a loop again (to
have a reference in the code, look
at KisPaintOpPresetsChooserPopup::canvasResourceChanged).

Then we also have signals reused beyond scope, like the resourceRemoved
signal that is also used to generate an update in the model of the widget
when for instance a brush engine changes or a change of tag category
happens.
The issue is that this signal is dealt with code that assumes that
something has been really removed and forces the first preset in the
available list to be selected ((look at KoResourceModel::resourceRemoved),
giving then problem when the higher classes want to select another preset.

Last but not least, we have this fight happening between higher classes and
the base classes, as i shown you in the previous point.
It seems that KoResourceItemChooser and company are made to work "alone"
and not being subclassed or used inside other classes, because they take
decisions about not sending signals (like resourceSelected, which is the
only connection between the chooser and Krita preset widgets, so that they
can ask to update the current selected preset), and they use the limited
knowledge they have there, where imho they should be much more dumb and
just send those events, then have higher classes that have the complete (or
at least more complete) picture.
For instance look
at KoResourceItemChooser::slotAfterResourcesLayoutReset(), where it ends up
blocking the resourceSelect signal when having a relayout.
The problem here is that when switching tag category in the preset docker,
the chooser and friends selects a new (the first) preset in the list, but
it doesn't let Krita and its higher widgets know about this and so we end
up having that issue where selecting a preset in a tag category that has
only that preset, it's not possible, because the chooser internally has it
selected, but it's not the actual current preset.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20150701/14741c4f/attachment.html>