Differential D3254 Diff 7927 libs/pigment/resources/KoColorSet.cpp

Changeset View

Standalone View

View Options

libs/pigment/resources/KoColorSet.cpp

Context not available.
34#include <QDomElement> 34​#include <QDomElement>
35#include <QXmlStreamReader> 35​#include <QXmlStreamReader>
36#include <QTextCodec> 36​#include <QTextCodec>
37​#include <QMap>
37 38​
38#include <DebugPigment.h> 39​#include <DebugPigment.h>
39#include <klocalizedstring.h> 40​#include <klocalizedstring.h>
Context not available.
50 QByteArray data; 51​ QByteArray data;
51 QString comment; 52​ QString comment;
52 qint32 columns; 53​ qint32 columns;
53 QVector<KoColorSetEntry> colors; 54​ QVector<KoColorSetEntry> colors; //ungrouped colors
55​ QStringList groupNames; //names of the groups, this is used to determine the order they are in.
56​ QMap<QString, QVector<KoColorSetEntry>> groups; //grouped colors.
54}; 57​};
55 58​
56KoColorSet::PaletteType detectFormat(const QString &fileName, const QByteArray &ba) { 59​KoColorSet::PaletteType detectFormat(const QString &fileName, const QByteArray &ba) {
Context not available.
152 return true; 155​ return true;
153} 156​}
154 157​
155qint32 KoColorSet::nColors() ​
156{ ​
157 return d->colors.count(); ​
158} ​
159 ​
160qint32 KoColorSet::getIndexClosestColor(KoColor color, bool useGivenColorSpace) ​
161{ ​
162 qint32 closestIndex = 0; ​
163 quint8 highestPercentage = 0; ​
164 quint8 testPercentage = 0; ​
165 KoColor compare = color; ​
166 for (qint32 i=0; i<nColors(); i++) { ​
167 KoColor entry = d->colors.at(i).color; ​
168 if (useGivenColorSpace==true && compare.colorSpace()!=entry.colorSpace()) { ​
169 entry.convertTo(compare.colorSpace()); ​
170 ​
171 } else if(compare.colorSpace()!=entry.colorSpace()) { ​
172 compare.convertTo(entry.colorSpace()); ​
173 } ​
174 testPercentage = (255 - compare.colorSpace()->difference(compare.data(), entry.data())); ​
175 if (testPercentage>highestPercentage) ​
176 { ​
177 closestIndex = i; ​
178 highestPercentage = testPercentage; ​
179 } ​
180 } ​
181 return closestIndex; ​
182} ​
183 ​
184QString KoColorSet::closestColorName(KoColor color, bool useGivenColorSpace) ​
185{ ​
186 int i = getIndexClosestColor(color, useGivenColorSpace); ​
187 QString name = d->colors.at(i).name; ​
188 return name; ​
189} ​
190 ​
191bool KoColorSet::saveToDevice(QIODevice *dev) const 158​bool KoColorSet::saveToDevice(QIODevice *dev) const
192{ 159​{
193 bool res; 160​ bool res;
Context not available.
297 return true; 264​ return true;
298} 265​}
299 266​
300void KoColorSet::add(const KoColorSetEntry & c) 267​quint32 KoColorSet::nColors()
268​{
269​ quint32 total = d->colors.count();
270​ if (!d->groups.empty()) {
271​ QStringList groupNames = getGroupNames();
remptUnsubmitted
Done

This is fine, but something like

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

totals += set.count();

}

is a bit simpler.

rempt: This is fine, but something like Q_FOREACH(const QVector<KoColorSetEntry>& set, d->groups.
woltheravAuthorUnsubmitted
Not Done

Using the group keys now.

woltherav: Using the group keys now.
272​ Q_FOREACH (QString name, groupNames) {
gladhornUnsubmitted
Done

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.

gladhorn: Using const references in foreach is more efficient. Q_FOREACH (const QString &name…
273​ total += d->groups[name].size();
274​ }
275​ }
276​ return total;
277​}
278​
279​quint32 KoColorSet::nColorsGroup(QString groupName) {
280​ if (d->groups.contains(groupName)) {
281​ return d->groups.value(groupName).size();
gladhornUnsubmitted
Not Done

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.

gladhorn: Here you do the lookup twice. If possible, try to use the value() function directly (one…
282​ } else if (groupName == QString()){
283​ return d->colors.size();
284​ } else {
remptUnsubmitted
Done

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.

rempt: Are you sure about this logic? I sort of would expect an empty groupName to return the count of…
285​ return 0;
286​ }
287​}
288​
289​quint32 KoColorSet::getIndexClosestColor(KoColor color, bool useGivenColorSpace)
gladhornUnsubmitted
Done

Use const ref for parameters: const KoColor &color

gladhorn: Use const ref for parameters: const KoColor &color
290​{
291​ quint32 closestIndex = 0;
292​ quint8 highestPercentage = 0;
293​ quint8 testPercentage = 0;
294​ KoColor compare = color;
295​ for (quint32 i=0; i<nColors(); i++) {
296​ KoColor entry = getColorGlobal(i).color;
297​ if (useGivenColorSpace==true && compare.colorSpace()!=entry.colorSpace()) {
gladhornUnsubmitted
Done

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

gladhorn: I personally prefer spaces around the == and != operators. I'm not sure what the policy is…
298​ entry.convertTo(compare.colorSpace());
299​
300​ } else if(compare.colorSpace()!=entry.colorSpace()) {
301​ compare.convertTo(entry.colorSpace());
302​ }
303​ testPercentage = (255 - compare.colorSpace()->difference(compare.data(), entry.data()));
304​ if (testPercentage>highestPercentage)
305​ {
306​ closestIndex = i;
307​ highestPercentage = testPercentage;
308​ }
309​ }
310​ return closestIndex;
311​}
312​
313​QString KoColorSet::closestColorName(KoColor color, bool useGivenColorSpace)
gladhornUnsubmitted
Done

const ref again

gladhorn: const ref again
314​{
315​ int i = getIndexClosestColor(color, useGivenColorSpace);
316​ QString name = d->colors.at(i).name;
317​ return name;
318​}
319​
320​void KoColorSet::add(const KoColorSetEntry & c, QString groupName)
301{ 321​{
302 d->colors.push_back(c); 322​ if (d->groups.contains(groupName) || d->groupNames.contains(groupName)) {
323​ d->groups[groupName].push_back(c);
324​ } else {
325​ d->colors.push_back(c);
326​ }
327​}
328​
329​quint32 KoColorSet::insertBefore(const KoColorSetEntry &c, qint32 index, QString groupName)
gladhornUnsubmitted
Done

const QString &

gladhorn: const QString &
330​{
331​ quint32 newIndex = index;
332​ if (d->groups.contains(groupName)) {
333​ d->groups[groupName].insert(index, c);
334​ } else if (groupName == QString()){
gladhornUnsubmitted
Done

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

gladhorn: .isEmpty() is even nicer, both to read and it's faster :)
335​ d->colors.insert(index, c);;
336​ } else {
337​ warnPigment << "Couldn't find group to insert to";
338​ }
339​ return newIndex;
303} 340​}
304 341​
305void KoColorSet::remove(const KoColorSetEntry & c) 342​void KoColorSet::remove(const KoColorSetEntry & c)
Context not available.
311 } 348​ }
312 ++it; 349​ ++it;
313 } 350​ }
351​ QMap<QString, QVector<KoColorSetEntry>>::const_iterator g = d->groups.constBegin();
remptUnsubmitted
Not Done

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?

rempt: Hm... I am not sure about this either: wouldn't it make sense to simply pass the name of the…
woltheravAuthorUnsubmitted
Not Done

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.

woltherav: This is mostly for compatibility reasons, like the global index. It's used in only one place…
352​ while (g!= d->groups.constEnd()) {
353​ for (auto it = d->groups[g.key()].begin(); it != d->groups[g.key()].end(); /*noop*/) {
354​ if ((*it) == c) {
355​ it = d->groups[g.key()].erase(it);
356​ return;
357​ }
358​ ++it;
359​ }
360​ ++g;
361​ }
314} 362​}
315 363​
316void KoColorSet::removeAt(quint32 index) 364​void KoColorSet::removeAt(quint32 index, QString groupName)
317{ 365​{
318 d->colors.remove(index); 366​ if (d->groups.contains(groupName)){
367​ if ((quint32)d->groups.value(groupName).size()>index) {
remptUnsubmitted
Done

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

rempt: This is dangerous: if you want to remove by index, make sure that the index is in the range of…
368​ d->groups[groupName].remove(index);
369​ }
370​ } else {
371​ if ((quint32)d->colors.size()>index) {
372​ d->colors.remove(index);
373​ }
374​ }
319} 375​}
320 376​
321void KoColorSet::clear() 377​void KoColorSet::clear()
322{ 378​{
323 d->colors.clear(); 379​ d->colors.clear();
380​ d->groups.clear();
381​}
382​
383​KoColorSetEntry KoColorSet::getColorGlobal(quint32 index)
384​{
385​ KoColorSetEntry e;
386​ quint32 groupIndex = index;
387​ QString groupName = findGroupByGlobalIndex(index, &groupIndex);
remptUnsubmitted
Not Done

I'm not sure I understand this construction.

rempt: I'm not sure I understand this construction.
woltheravAuthorUnsubmitted
Not Done

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

woltherav: It is for compatability right now. Otherwise I have to rewrite the palette docker ad everythin…
388​ e = getColorGroup(groupIndex, groupName);
389​ return e;
390​}
391​
392​KoColorSetEntry KoColorSet::getColorGroup(quint32 index, QString groupName)
393​{
394​ KoColorSetEntry e;
395​ if (d->groups.contains(groupName) && index<(quint32)d->groups.value(groupName).size()) {
396​ e = d->groups.value(groupName).at(index);
397​ } else if (groupName == QString() && index<(quint32)d->colors.size()) {
398​ e = d->colors.at(index);
399​ } else {
400​ warnPigment << "Color group "<<groupName<<" not found";
401​ }
402​ return e;
403​}
404​
405​QString KoColorSet::findGroupByGlobalIndex(quint32 globalIndex, quint32 *index)
gladhornUnsubmitted
Not Done

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.

gladhorn: I don't understand why this function and a few others would return both a string and an index.
woltheravAuthorUnsubmitted
Not Done

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

woltherav: I am not good enough a programmer to say anything about this. @rempt?
406​{
407​ *index = globalIndex;
408​ QString groupName = QString();
409​ if ((quint32)d->colors.size()<=*index) {
410​ *index -= (quint32)d->colors.size();
411​ if (!d->groups.empty() || !d->groupNames.empty()) {
412​ QStringList groupNames = getGroupNames();
413​ Q_FOREACH (QString name, groupNames) {
414​ quint32 size = (quint32)d->groups.value(name).size();
415​ if (size<=*index) {
416​ *index -= size;
417​ } else {
418​ groupName = name;
419​ return groupName;
420​ }
421​ }
422​
423​ }
424​ }
425​ return groupName;
426​}
427​
428​QString KoColorSet::findGroupByColorName(QString name, quint32 *index) {
gladhornUnsubmitted
Done

const QString &

gladhorn: const QString &
429​ *index = (quint32)0;
gladhornUnsubmitted
Done

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.

gladhorn: Why is there a c-style cast here? ``` *index = 0; ``` Is good, the compiler will do all that's…
430​ QString groupName = QString();
431​ for (int i = 0; i<d->colors.size(); i++) {
432​ if(d->colors.at(i).name == name) {
433​ *index = (quint32)i;
434​ return groupName;
435​ }
436​ }
437​ QStringList groupNames = getGroupNames();
438​ Q_FOREACH (QString name, groupNames) {
439​ for (int i=0; i<d->groups[name].size(); i++) {
440​ if(d->groups[name].at(i).name == name) {
441​ *index = (quint32)i;
442​ groupName = name;
443​ return groupName;
444​ }
445​ }
446​ }
447​ return groupName;
448​}
449​
450​QString KoColorSet::findGroupByID(QString id, quint32 *index) {
451​ *index = (quint32)0;
452​ QString groupName = QString();
453​ for (int i = 0; i<d->colors.size(); i++) {
454​ if(d->colors.at(i).id == id) {
455​ *index = (quint32)i;
456​ return groupName;
457​ }
458​ }
459​ QStringList groupNames = getGroupNames();
460​ Q_FOREACH (QString name, groupNames) {
461​ for (int i=0; i<d->groups[name].size(); i++) {
462​ if(d->groups[name].at(i).id == id) {
463​ *index = (quint32)i;
464​ groupName = name;
465​ return groupName;
466​ }
467​ }
468​ }
469​ return groupName;
324} 470​}
325 471​
326KoColorSetEntry KoColorSet::getColor(quint32 index) 472​QStringList KoColorSet::getGroupNames()
327{ 473​{
328 return d->colors[index]; 474​ if (d->groupNames.size()<d->groups.size()) {
gladhornUnsubmitted
Not Done

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.

gladhorn: I'd check this on insertion and not have code here to try to fix things up. Just return d…
woltheravAuthorUnsubmitted
Not Done

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.

woltherav: I am not good enough a programmer to make any decisions on this. I just am returning the keys…
475​ warnPigment << "mismatch between groups and the groupnames list. Will rectify for now.";
476​ Q_FOREACH(QString name, d->groups.keys()) {
477​ if (!d->groupNames.contains(name)) {
478​ d->groupNames.append(name);
479​ }
480​ }
481​ }
482​ return d->groupNames;
329} 483​}
330 484​
331void KoColorSet::setColumnCount(int columns) 485​void KoColorSet::setColumnCount(int columns)
Context not available.
338 return d->columns; 492​ return d->columns;
339} 493​}
340 494​
495​QString KoColorSet::comment()
496​{
497​ return d->comment;
498​}
499​
500​bool KoColorSet::addGroup(QString groupName)
gladhornUnsubmitted
Done

const ref

gladhorn: const ref
501​{
502​ if (d->groups.contains(groupName) || d->groupNames.contains(groupName)) {
503​ return false;
504​ }
505​ d->groupNames.append(groupName);
506​ d->groups[groupName];
507​ return true;
508​}
509​
510​bool KoColorSet::removeGroup(QString groupName, bool keepColors)
gladhornUnsubmitted
Done

const QString &

gladhorn: const QString &
511​{
512​ if (!d->groups.contains(groupName)) {
513​ return false;
514​ }
515​ if (keepColors) {
516​ for (int i = 0; i<d->groups.value(groupName).size(); i++) {
517​ d->colors.append(d->groups.value(groupName).at(i));
518​ }
519​ }
520​ for(int n = 0; n<d->groupNames.size(); n++) {
521​ if (d->groupNames.at(n) == groupName) {
522​ d->groupNames.removeAt(n);
523​ }
524​ }
525​
526​ d->groups.remove(groupName);
527​ return true;
528​}
529​
341QString KoColorSet::defaultFileExtension() const 530​QString KoColorSet::defaultFileExtension() const
342{ 531​{
343 return QString(".kpl"); 532​ return QString(".kpl");
Context not available.
696 entry.color.toXML(doc, el); 885​ entry.color.toXML(doc, el);
697 root.appendChild(el); 886​ root.appendChild(el);
698 } 887​ }
888​ Q_FOREACH(const QString groupName, d->groupNames) {
gladhornUnsubmitted
Done

use const ref in the foreach

gladhorn: use const ref in the foreach
889​ QDomElement gl = doc.createElement("Group");
890​ gl.setAttribute("name", groupName);
891​ root.appendChild(gl);
892​ Q_FOREACH(const KoColorSetEntry &entry, d->groups.value(groupName)) {
893​
894​ // Only save non-builtin profiles.=
gladhornUnsubmitted
Not Done

does the = at the end have a meaning?

gladhorn: does the = at the end have a meaning?
woltheravAuthorUnsubmitted
Not Done

not my code.

woltherav: not my code.
895​ const KoColorProfile *profile = entry.color.colorSpace()->profile();
896​ if (!profile->fileName().isEmpty()) {
897​ profiles << profile;
898​ profileMap[profile] = entry.color.colorSpace();
899​ }
900​ QDomElement el = doc.createElement("ColorSetEntry");
901​ el.setAttribute("name", entry.name);
902​ el.setAttribute("id", entry.id);
903​ el.setAttribute("spot", entry.spotColor ? "true" : "false");
904​ el.setAttribute("bitdepth", entry.color.colorSpace()->colorDepthId().id());
905​ entry.color.toXML(doc, el);
906​ gl.appendChild(el);
907​ }
908​ }
909​
699 doc.appendChild(root); 910​ doc.appendChild(root);
700 if (!store->open("colorset.xml")) { return false; } 911​ if (!store->open("colorset.xml")) { return false; }
701 QByteArray ba = doc.toByteArray(); 912​ QByteArray ba = doc.toByteArray();
Context not available.
799 entry.spotColor = c.attribute("spot", "false") == "true" ? true : false; 1010​ entry.spotColor = c.attribute("spot", "false") == "true" ? true : false;
800 d->colors << entry; 1011​ d->colors << entry;
801 1012​
802 c = c.nextSiblingElement(); 1013​ c = c.nextSiblingElement("ColorSetEntry");
803 1014​
804 } 1015​ }
1016​ QDomElement g = e.firstChildElement("Group");
1017​ while (!g.isNull()) {
1018​ QString groupName = g.attribute("name");
1019​ addGroup(groupName);
1020​ QDomElement cg = g.firstChildElement("ColorSetEntry");
1021​ while (!cg.isNull()) {
1022​ QString colorDepthId = e.attribute("bitdepth", Integer8BitsColorDepthID.id());
1023​ KoColorSetEntry entry;
1024​
1025​
1026​ entry.color = KoColor::fromXML(cg.firstChildElement(), colorDepthId);
1027​ entry.name = cg.attribute("name");
1028​ entry.id = cg.attribute("id");
1029​ entry.spotColor = cg.attribute("spot", "false") == "true" ? true : false;
1030​ add(entry, groupName);
1031​
1032​ cg = cg.nextSiblingElement("ColorSetEntry");
1033​
1034​ }
1035​ g = g.nextSiblingElement("Group");
1036​ }
1037​
805 } 1038​ }
806 1039​
807 1040​
Context not available.