| rempt |
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
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.
| Lint Skipped |
| Unit Tests Skipped |
Ok, it doesn't build at all, so lets get to that first... Any other comments are welcome in the meantime.
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?
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...
I'm not totally sure I understand the idea behind the global index thing -- I'm probably missing something here, though.
| libs/pigment/resources/KoColorSet.cpp | ||
|---|---|---|
| 271 | This is fine, but something like Q_FOREACH(const QVector<KoColorSetEntry>& set, d->groups.values()) { totals += set.count(); } is a bit simpler. | |
| 284 | 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. | |
| 351 | 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? | |
| 367 | This is dangerous: if you want to remove by index, make sure that the index is in the range of the QVector object. | |
| 387 | 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.
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.
| libs/pigment/resources/KoColorSet.cpp | ||
|---|---|---|
| 271 | Using the group keys now. | |
| 351 | 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. | |
| 387 | It is for compatability right now. Otherwise I have to rewrite the palette docker ad everythin to understand groups. | |
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).
| libs/pigment/resources/KoColorSet.cpp | ||
|---|---|---|
| 272 | 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. | |
| 281 | 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. | |
| 289 | Use const ref for parameters: const KoColor &color | |
| 297 | I personally prefer spaces around the == and != operators. I'm not sure what the policy is though. | |
| 313 | const ref again | |
| 329 | const QString & | |
| 334 | .isEmpty() is even nicer, both to read and it's faster :) | |
| 405 | 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. | |
| 428 | const QString & | |
| 429 | 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. | |
| 474 | 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. | |
| 500 | const ref | |
| 510 | const QString & | |
| 888 | use const ref in the foreach | |
| 894 | 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;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.
| libs/pigment/resources/KoColorSet.cpp | ||
|---|---|---|
| 405 | I am not good enough a programmer to say anything about this. @rempt? | |
| 474 | 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. | |
| 894 | not my code. | |