[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-kimageshop
Subject: Re: Preset widgets refactoring
From: Stefano Bonicatti <smjert () gmail ! com>
Date: 2015-08-30 15:08:33
Message-ID: CAGMAV4-GNA_v45wNjtvSgj2Ev9iO8i82Nnoc4i2LPPAAc5gNXw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
So here i'm again, time to think "out loud" again (as if i don't do this
often enough!).
More than a month passed (almost two!) and i'm slowly trying, especially
due to the new work, to do the refactoring, right now i almost removed all
the code which KisPaintOpBox shouldn't execute and moved it to a new
KisPresetManager class, though i'm facing several issues that i cannot
solve properly with the current state of the code.
Unfortunately things are much more intertwined than i thought initially and
basically all the GUI classes that deal with presets do "too much things",
step beyond their logic boundaries and complicate everything.
So either i keep running around trying to fight against these to maybe then
find a way to make things work, but that it won't last, or i go back to the
drawing board and actually start to thinking to do a bigger refactoring.
Given that i don't have in depth knowledge of every class involved i just
tried to retain the same functionality and order of calls that
KisPaintOpBox had, while trying to unravel the mess with preset events and
such.
Though this doesn't work, i see a lot of duplicated code, code that is too
reused and then leads to have calls that are not needed, and it's not
(only) a performance problem, but they do something they shouldn't always
do, leading to the issues we have now.
What's giving me issues for instance is KisPaintOpSettingsWidget who is
supposed to signal stuff when its configuration changes, but those are most
of the time blocked (and i don't find exactly a logic about that, if one
has to always block the signals except in rare cases, better not
use signals at all then... or use different code paths for different
operations, one that launches them and one that doesn't, so that they "self
document" themselves instead of always relying on someone else knowing when
to block and when not).
Also here i see setConfiguration called for cases where, probably, a more
precise call (to change only one value) would be needed.
Other things are compositeOps, when a preset is set, an "update composite
op" procedure has to be called, though the things that have to be done are
slightly different than in the case where only the composite op id is
changed, so the code written before cannot really be reused.
At the same time, given that one thinks such a small change should lead to
few other changes around, this actually leads in calling setConfiguration
in the option widget as if the entire config changed, and obviously this is
guarded by signals blocker (but it only changed one value in the settings,
why a big update like setConfiguration?).
Then to help with all this comes events launched by the canvas resource
manager, here we should only hear about preset changes, as in a new preset
has been set.
Though given that my vision is that every widget able to let the user
select a preset has to be wired up to the KisPresetManager through another
much more controlled way, the one i'm using with the KisPaintOpBox, who can
change presets beyond those widgets?
I don't know, and i don't know how to properly support this event without
knowing. As far as i know right now those events are triggered by the other
preset choosing widgets selecting some preset and setting it in the
resource provider.
Though again in my vision if the widgets gets their preset updated not from
a user interaction, they should not notify KisPresetManager, otherwise this
would leads to loops.
This would be tough though, because widgets are just windows on the loaded
presets, but they react to any preset add and there's no way to make
difference from a user interaction and from a non user interaction, because
in both cases the updates comes from the underlying widget/model update
code.
So this means changing the code that loads presets and or models that those
widgets depends on so that they react differently or signal one case
differently from the other one.
So in the end, as i said, back to the drawing board, listing all the
classes involved, what they are supposed to do and have to do, drawing the
interactions and unravel them since there are a lot of things that have to
be streamlined and should have a somewhat fixed order of execution
(especially needed with GUI interaction).
I think that the bigger refactoring cannot be avoided.
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">So here i'm \
again, time to think "out loud" again (as if i don't do this often \
enough!).</div><div class="gmail_quote"><br></div><div class="gmail_quote">More than \
a month passed (almost two!) and i'm slowly trying, especially due to the new \
work, to do the refactoring, right now i almost removed all the code which \
KisPaintOpBox shouldn't execute and moved it to a new KisPresetManager class, \
though i'm facing several issues that i cannot solve properly with the current \
state of the code.</div><div class="gmail_quote"><br></div><div \
class="gmail_quote">Unfortunately things are much more intertwined than i thought \
initially and basically all the GUI classes that deal with presets do "too much \
things", step beyond their logic boundaries and complicate everything.</div><div \
class="gmail_quote">So either i keep running around trying to fight against these to \
maybe then find a way to make things work, but that it won't last, or i go back \
to the drawing board and actually start to thinking to do a bigger \
refactoring.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Given \
that i don't have in depth knowledge of every class involved i just tried to \
retain the same functionality and order of calls that KisPaintOpBox had, while trying \
to unravel the mess with preset events and such.</div><div class="gmail_quote">Though \
this doesn't work, i see a lot of duplicated code, code that is too reused and \
then leads to have calls that are not needed, and it's not (only) a performance \
problem, but they do something they shouldn't always do, leading to the issues we \
have now.</div><div class="gmail_quote"><br></div><div class="gmail_quote">What's \
giving me issues for instance is KisPaintOpSettingsWidget who is supposed to signal \
stuff when its configuration changes, but those are most of the time blocked (and i \
don't find exactly a logic about that, if one has to always block the signals \
except in rare cases, better not use signals at all then... or use different code \
paths for different operations, one that launches them and one that doesn't, so \
that they "self document" themselves instead of always relying on someone \
else knowing when to block and when not).</div><div class="gmail_quote">Also here i \
see setConfiguration called for cases where, probably, a more precise call (to change \
only one value) would be needed.</div><div class="gmail_quote"><br></div><div \
class="gmail_quote">Other things are compositeOps, when a preset is set, an \
"update composite op" procedure has to be called, though the things that \
have to be done are slightly different than in the case where only the composite op \
id is changed, so the code written before cannot really be reused. </div><div \
class="gmail_quote">At the same time, given that one thinks such a small change \
should lead to few other changes around, this actually leads in calling \
setConfiguration in the option widget as if the entire config changed, and obviously \
this is guarded by signals blocker (but it only changed one value in the settings, \
why a big update like setConfiguration?).<br></div><div \
class="gmail_quote"><br></div><div class="gmail_quote">Then to help with all this \
comes events launched by the canvas resource manager, here we should only hear about \
preset changes, as in a new preset has been set.</div><div class="gmail_quote">Though \
given that my vision is that every widget able to let the user select a preset has to \
be wired up to the KisPresetManager through another much more controlled way, the one \
i'm using with the KisPaintOpBox, who can change presets beyond those \
widgets?</div><div class="gmail_quote">I don't know, and i don't know how to \
properly support this event without knowing. As far as i know right now those events \
are triggered by the other preset choosing widgets selecting some preset and setting \
it in the resource provider.</div><div class="gmail_quote">Though again in my vision \
if the widgets gets their preset updated not from a user interaction, they should not \
notify KisPresetManager, otherwise this would leads to loops.<br></div><div \
class="gmail_quote">This would be tough though, because widgets are just windows on \
the loaded presets, but they react to any preset add and there's no way to make \
difference from a user interaction and from a non user interaction, because in both \
cases the updates comes from the underlying widget/model update code.</div><div \
class="gmail_quote">So this means changing the code that loads presets and or models \
that those widgets depends on so that they react differently or signal one case \
differently from the other one.</div><div class="gmail_quote"><br></div><div \
class="gmail_quote">So in the end, as i said, back to the drawing board, listing all \
the classes involved, what they are supposed to do and have to do, drawing the \
interactions and unravel them since there are a lot of things that have to be \
streamlined and should have a somewhat fixed order of execution (especially needed \
with GUI interaction).</div><div class="gmail_quote">I think that the bigger \
refactoring cannot be avoided.</div></div></div>
[Attachment #6 (text/plain)]
_______________________________________________
Krita mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic