[KPhotoAlbum] Keywords with id=0?

[Robert Krawitz]

On Sun, 15 Dec 2019 17:31:48 +0100, Johannes Zarl-Zierl wrote:
> Hi Robert,
>
> You beat me to it - I had finally untangled this mess for myself and was about to start working on a fix when I saw the your mail pop up ;-)
>
> Am Sonntag, 15. Dezember 2019, 17:09:02 CET schrieb Robert Krawitz:
>>      addItem(newValue);
>> -    setIdMapping(newValue, id);
>> +    if ( id != 0)
>> +        setIdMapping(newValue, id);
>>      emit itemRenamed(oldValue, newValue);
>>  }
>
> -> I would leave the call unguarded here. Keeping the special knowledge localized to as few functions as possible seems like a good practice to reduce the probability of future bugs.
>
>> @@ -179,8 +181,12 @@ void XMLDB::XMLCategory::initIdMap()
>> 
>>  void XMLDB::XMLCategory::setIdMapping(const QString &name, int id)
>>  {
>> -    m_nameMap.insert(id, name);
>> -    m_idMap.insert(name, id);
>> +    if (id == 0) {
>> +        qCWarning(XMLDBLog, "XMLDB::XMLCategory::setIdMapping attempting to
>> set id for %s to invalid value %d", qPrintable(name), id); +    } else {
>> +        m_nameMap.insert(id, name);
>> +        m_idMap.insert(name, id);
>> +    }
>>  }
>> 
>>  QString XMLDB::XMLCategory::nameForId(int id) const
>
> You can leave the warning out here completely, because now this is
> expected behaviour.

If we stay with this approach, I want to both keep the warning and the
guard.

First the warning: nothing should attempt to set an invalid value; we
want to know if something's trying to do that.  Failing silently might
cause other unintended behavior, and in any event, if something's
breaking the rules, we want to know.

Secondly, the guard: again, we don't want to perform an invalid
operation (which is within the same class, no non-local knowledge
here).  And we don't want renaming a tag or group, which is perfectly
legal, to generate a warning indicating an illegal operation.
-- 
Robert Krawitz                                     <rlk at alum.mit.edu>

***  MIT Engineers   A Proud Tradition   http://mitathletics.com  ***
Member of the League for Programming Freedom  --  http://ProgFree.org
Project lead for Gutenprint   --    http://gimp-print.sourceforge.net

"Linux doesn't dictate how I work, I dictate how Linux works."
--Eric Crampton