This is a patch on top of my custom colors patch.
The shared/global assistant color for painting assistants is stored outside of the assistant data model. Because of this I had to add a new property to save to in the KRA saver and loader.
I also had to fix up a few initialization areas in the UI so the loaded values didn't get wiped out.
I also removed a couple "legacy" comments in the KRA saver that got me confused. boud confirmed that those notes were in the calligra days
Have multiple assistants. A couple with a shared color and one with a custom color.
Saved and loaded the file and made sure it kept the same value.
| Lint Skipped |
| Unit Tests Skipped |
Hi, @scottpetrovic!
I have tested your patch and it seems like there is a bug:
Now the assistants have wrong color. From my experience the loaded color will be the one that was before the selected one.
thanks for the review @dkazakov
I accidentally swapped the green and blue values with the string conversion, so that was messing up the colors on load. It looks like it works now on my box now that I fixed that.
Hi, @scottpetrovic!
I have tested your patch, it seems to work fine now. The only thing that worries me is file format consistency. Please make it save all the color options in a uniform way. And make sure you also fix saving in 4.1, we will not be able to change file format after the release.
In my opinion, the best common way of saving QColor would be to extract your serialization methods into something like KisDomUtils and use it everywhere. At first I wanted to propose using QColor::name() instead, which is a bit more robust, but then I found out that your serialization method is also used in KConfigGroup, so it might be a good ideal to reuse your method in the end.
Please fix the patch as soon as possible so that we could push into into 4.1!
| libs/ui/kis_painting_assistants_decoration.cpp | ||
|---|---|---|
| 45 | Why default value for globalAssistantsColor is different in KisDocument and KisPaintingAssistantsDecoration? Perhaps, you could read it from KisConfig? | |
| plugins/impex/libkra/kis_kra_loader.cpp | ||
| 323 | Why do you use two different saving methods for saving global color and local color? Please use one method for both cases, either KoColor or QColor::name() or your own method, but extracted into some common class (e.g. KisDomUtils) and tested. | |
I moved the conversion functions to KisDomUtils and updated the references
I removed one place where the QColor was being defined in the kis document. That wasn't getting used anyway, so I just removed that part.
For the format, I made a new constant that is just simple color data that stores all assistant colors. The reasoning is that KoColor is using byte array data which other areas use like the background color on the canvas. The assistant colors can also be saved to XML through the tool options. I kind of don't like saving byte array data to XML as it is not as usable/editable. Making all the assistant color data save to this 'simple color' format for assistants was my solution.
Let me know if anything still needs to be changed.
Updated notes
| libs/ui/kis_painting_assistants_decoration.cpp | ||
|---|---|---|
| 45 | I just removed this duplicate definition. It doesn't appear to be getting used anyway | |
| plugins/impex/libkra/kis_kra_loader.cpp | ||
| 323 | I was originally following the pattern for the saving/loading for the global assistant color. That was using the byte array KoColor method. The assistant data can also be in XML when saving the assistants, so I just made all assistant saving/loading use the new conversion function that translates them to and from QStrings. 8 bit color information is fine for this storage | |
Hi, @scottpetrovic!
Now the patch looks fine codewise, but it got one regression :(
Newly created assistant color will be black, although in the GUI gray global color is selected. It looks like there is some small problem with initialization.
I guess that is the last blocker for the patch to be pushed.
| libs/global/kis_dom_utils.h | ||
|---|---|---|
| 128 | I would add a check for a length of colorComponents array, it should be 4, who knows what the user will modify manually :) You can use KIS_SAFE_ASSERT_RECOVER_RETURN_VALUE for that | |