<div dir="ltr">Hello everyone, today i'm going to talk you about the preset widgets, signals between them and about signals a bit more generally.<div><br></div><div>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.</div><div>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.</div><div>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.</div><div><br></div><div>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.</div><div>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.</div><div>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.</div><div>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.</div><div><br></div><div>So there's a continuous fight between these widgets.</div><div>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.</div><div><br></div><div>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.</div><div>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.</div><div>This has the ultimate decision of which preset is the current one.</div><div>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.</div><div>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.</div><div>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.</div><div>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.</div><div><br></div><div>Another important thing is that we should split signals and call paths much more, and this is true in many other cases.</div><div>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).</div><div>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).</div><div><br></div><div>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).</div><div><br></div><div>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.</div><div>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.</div><div><br></div><div>Last but not least, we have this fight happening between higher classes and the base classes, as i shown you in the previous point.</div><div>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.</div><div>For instance look at KoResourceItemChooser::slotAfterResourcesLayoutReset(), where it ends up blocking the resourceSelect signal when having a relayout.</div><div>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.</div><div><br></div><div><br></div><div>TL;DR:</div><div><br></div><div>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.</div><div><br></div><div>2) We don't distinguish call paths that comes from or signals that are emitted due to UI or non UI events.</div><div>This leads to problems because in some cases creates loops (that are dealt with but in a way that creates other functionality issues).</div><div>We should split these paths by using different functions and different signals and we should be sure that one can't trigger the other.</div><div><br></div><div>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.</div><div><br></div><div>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. </div><div>We need to have specific signals for specific operations.</div><div><br></div><div>5) We have fighting occurring between higher classes and base classes too, about the resource selection (preset widgets fighting with KoResourceItemChooser etc).</div><div>We probably should have base classes that are more dumb than we have now.</div><div><br></div><div>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.</div><div>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.</div><div><br></div><div>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.</div><div>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.</div></div>