[KPhotoAlbum] Keywords with id=0?

[Robert Krawitz]

On Sun, 15 Dec 2019 10:17:34 +0100, Tobias Leupold wrote:
> Okay. After being able to reproduce this and reading the code for quite a long
> time, I must admit that I have no idea how or why this happens. Maybe somebody
> with more clue about KPA's internals has? Johannes?!

OK, found it.  The problem is in XMLDB::XMLCategory::renameItem().

Background: IDs are normally only assigned in
XMLDB::XMLCategory::initIdMap(), which is called from void
XMLDB::XMLCategoryCollection::initIdMap(), which is only called from
XMLDB::FileWriter::save().  So IDs are only assigned when the index
file is saved.  So if you're trying to hunt down where an ID is
assigned when you add an item, the search will be fruitless.  However,
the IDs do persist for the life of the session, so that any IDs that
are assigned will remain.

Next, idForMap simply returns

m_idMap[name]

What happens if name is not present in a qMap?  It returns a default
constructed value, which in this case is a default int, which is 0.
But rename() always sets the id mapping for the new name, even if one
previously didn't exist.  So when you rename something, the new name
always gets the value 0.

There are a couple of ways of solving this.  One is what I'm doing
here, for rename to not set the ID if it's zero, and more importantly
refuse to set an ID of zero so that if something else tries to set it
to 0 it will not succeed.  So strictly speaking the change to
renameItem() isn't needed, but this will prevent an unneeded warning
from being emitted.

Perhaps we should improve checking in setIdMapping() and add
removeIdMapping() too.

In any case, if one of you could review this, I'll push it later today.

diff --git a/XMLDB/XMLCategory.cpp b/XMLDB/XMLCategory.cpp
index 712cc689..30e55cdb 100644
--- a/XMLDB/XMLCategory.cpp
+++ b/XMLDB/XMLCategory.cpp
@@ -20,6 +20,7 @@
 #include <DB/ImageDB.h>
 #include <DB/MemberMap.h>
 #include <Utilities/List.h>
+#include "Logging.h"
 
 XMLDB::XMLCategory::XMLCategory(const QString &name, const QString &icon, ViewType type, int thumbnailSize, bool show, bool positionable)
     : m_name(name)
@@ -130,7 +131,8 @@ void XMLDB::XMLCategory::renameItem(const QString &oldValue, const QString &newV
     m_idMap.remove(oldValue);
 
     addItem(newValue);
-    setIdMapping(newValue, id);
+    if ( id != 0)
+        setIdMapping(newValue, id);
     emit itemRenamed(oldValue, newValue);
 }
 
@@ -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

-- 
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