Add group support to KoColorSet and KPL. (in progress)

Authored by woltherav on Nov 4 2016, 8:52 AM.



This adds the ability for KoColorSet to have a QMap of colorsetentry lists.

Also adds functions to manipulate these groups, and saving/loading them from KPL.

I am making this review so that I can verify whether what I did makes sense or not, as I haven't done anything with QMap before.

As discussed in T112

Test Plan

Open this file:

All its colors are in groups, you can view these groups in the palette docker name, where they are written as "groupName - entryName" at the bottom of the docker.

Diff Detail

R37 Krita
Lint Skipped
Unit Tests Skipped
woltherav updated this revision to Diff 7882.Nov 4 2016, 8:52 AM
woltherav retitled this revision from to Add group support to KoColorSet and KPL. (in progress).
woltherav updated this object.
woltherav edited the test plan for this revision. (Show Details)
woltherav added a reviewer: rempt.
woltherav set the repository for this revision to R37 Krita.

Ok, it doesn't build at all, so lets get to that first... Any other comments are welcome in the meantime.

woltherav updated this revision to Diff 7891.Nov 4 2016, 10:58 AM

It now builds.

I changed the return from ncolors to be a quint32, to be internally consistent, but I am not sure if I did right?

woltherav updated this revision to Diff 7900.Nov 4 2016, 11:48 AM
woltherav edited the test plan for this revision. (Show Details)

Ok, you can now load and test groups, but not make, edit or save them...

It seems groups get sorted automatically by their name due the way QMap works...

woltherav edited the test plan for this revision. (Show Details)Nov 4 2016, 11:49 AM
rempt edited edge metadata.Nov 5 2016, 9:52 AM

I'm not totally sure I understand the idea behind the global index thing -- I'm probably missing something here, though.


This is fine, but something like

Q_FOREACH(const QVector<KoColorSetEntry>& set, d->groups.values()) {

totals += set.count();


is a bit simpler.


Are you sure about this logic? I sort of would expect an empty groupName to return the count of the toplevel colorset, which an invalid or non-existent groupName should return 0.


Hm... I am not sure about this either: wouldn't it make sense to simply pass the name of the colorgroup from which set the set should be removed? Or can sets be part of more than one group?


This is dangerous: if you want to remove by index, make sure that the index is in the range of the QVector object.


I'm not sure I understand this construction.

Global index is mostly to keep compatability with existing palette docker code. I'll be looking at the rest.

woltherav updated this revision to Diff 7925.Nov 5 2016, 4:06 PM
woltherav edited edge metadata.
woltherav marked 3 inline comments as done.

Made the suggested changes... I am wondering whether we shouldn't be using QHash instead of QMap, as QMap always sorts alphabetically...(so groups will always be sorted alphabetically, blue coming before red) Other other hand that means rewriting everything to fit the other again :/

extra comments.


Using the group keys now.


This is mostly for compatibility reasons, like the global index. It's used in only one place right now... I don't want full rewrites to be part of this patch just yet.


It is for compatability right now. Otherwise I have to rewrite the palette docker ad everythin to understand groups.

This comment was removed by lsegovia.
woltherav updated this revision to Diff 7927.Nov 5 2016, 6:24 PM

Ok, I fixed the issue by adding a QStringList that orders the groups. Making this change I also made the code a bit more crash-proof. You can now use the getGroupNames for itterating through groups in the order they were read. I am not sure yet how we should deal with manipulating said list, but the QStringList rectifies itself through that function if there's strings missing while moaning about it in the terminal, which should be a good start.

A few inline comments, mostly nit-picking.
The code seems quite complex to me though, having special cases is always a bad thing. So if colors are going to be handled in groups, I'd propose having a default group that still has a name (empty string as name is also possible, but that makes it harder to handle again).


Using const references in foreach is more efficient.

Q_FOREACH (const QString &name, groupNames)

(of course I'd vote for C++11 ranged for, but that seems to be not liked by Krita folks so far...)

for (QString name: qAsConst(groupNames)) { ... }

Anyway, Boud already suggested a nicer solution.


Here you do the lookup twice. If possible, try to use the value() function directly (one lookup) and check if it returned a default constructed empty string or not.


Use const ref for parameters: const KoColor &color


I personally prefer spaces around the == and != operators. I'm not sure what the policy is though.


const ref again


const QString &


.isEmpty() is even nicer, both to read and it's faster :)


I don't understand why this function and a few others would return both a string and an index. Imho it would also be nice to define a struct { QString groupName; quint32 index; } if it's really needed and rather return that instead of having the out parameter.


const QString &


Why is there a c-style cast here?

*index = 0;

Is good, the compiler will do all that's needed to make sure the 0 is of the right type.


I'd check this on insertion and not have code here to try to fix things up. Just return d->groupNames; in the body of the getter.


const ref


const QString &


use const ref in the foreach


does the = at the end have a meaning?

Depending on the access pattern, just using a qvector might be an option too. I have no idea how often this is accessed, so that is the main consideration. It would at least not get out of sync.

struct {
    QString groupName;
    QVector<color> colors;
QVector<ColorGroup> colorGroups;
woltherav marked 11 inline comments as done.Nov 6 2016, 1:46 PM

made changes, except where I am just not good enough a programmer to understand what it being meant. Gonna push this into the branch for now, it was not intended to be a formal review-request anyway.


I am not good enough a programmer to say anything about this. @rempt?


I am not good enough a programmer to make any decisions on this. I just am returning the keys now if they don't match.


not my code.