handle messages with ids (more) correctly

when comparing two messages which both have ids, compare only the ids.
this affects finding/reporting duplicates and merging.

4 files changed, 138 insertions, 52 deletions

diff --git a/tools/linguist/linguist/messagemodel.cpp b/tools/linguist/linguist/messagemodel.cpp
index 8966624..024fd91 100644
--- a/tools/linguist/linguist/messagemodel.cpp
+++ b/tools/linguist/linguist/messagemodel.cpp
@@ -210,9 +210,16 @@ bool DataModel::load(const QString &fileName, bool *langGuessed, QWidget *parent
     }
 
     Translator::Duplicates dupes = tor.resolveDuplicates();
-    if (!dupes.byContents.isEmpty()) {
+    if (!dupes.byId.isEmpty() || !dupes.byContents.isEmpty()) {
         QString err = tr("<qt>Duplicate messages found in '%1':").arg(Qt::escape(fileName));
         int numdups = 0;
+        foreach (int i, dupes.byId) {
+            if (++numdups >= 5) {
+                err += tr("<p>[more duplicates omitted]");
+                goto doWarn;
+            }
+            err += tr("<p>* ID: %1").arg(Qt::escape(tor.message(i).id()));
+        }
         foreach (int j, dupes.byContents) {
             const TranslatorMessage &msg = tor.message(j);
             if (++numdups >= 5) {
@@ -224,6 +231,7 @@ bool DataModel::load(const QString &fileName, bool *langGuessed, QWidget *parent
             if (!msg.comment().isEmpty())
                 err += tr("<br>* Comment: %3").arg(Qt::escape(msg.comment()));
         }
+      doWarn:
         QMessageBox::warning(parent, QObject::tr("Qt Linguist"), err);
     }
 
diff --git a/tools/linguist/lupdate/merge.cpp b/tools/linguist/lupdate/merge.cpp
index 13ba4ae..a721439 100644
--- a/tools/linguist/lupdate/merge.cpp
+++ b/tools/linguist/lupdate/merge.cpp
@@ -375,6 +375,7 @@ Translator merge(const Translator &tor, const Translator &virginTor,
                             ++similarTextHeuristicCount;
                             neww++;
 
+                          outdateSource:
                             m.setOldSourceText(m.sourceText());
                             m.setSourceText(mv.sourceText());
                             const QString &oldpluralsource = m.extra(QLatin1String("po-msgid_plural"));
@@ -392,23 +393,35 @@ Translator merge(const Translator &tor, const Translator &virginTor,
                 }
             } else {
                 mv = virginTor.message(mvi);
-                switch (m.type()) {
-                case TranslatorMessage::Finished:
-                default:
-                    if (m.isPlural() == mv.isPlural()) {
-                        newType = TranslatorMessage::Finished;
-                    } else {
-                        newType = TranslatorMessage::Unfinished;
-                    }
+                if (!mv.id().isEmpty()
+                    && (mv.context() != m.context()
+                        || mv.sourceText() != m.sourceText()
+                        || mv.comment() != m.comment())) {
                     known++;
-                    break;
-                case TranslatorMessage::Unfinished:
                     newType = TranslatorMessage::Unfinished;
-                    known++;
-                    break;
-                case TranslatorMessage::Obsolete:
-                    newType = TranslatorMessage::Unfinished;
-                    neww++;
+                    m.setContext(mv.context());
+                    m.setComment(mv.comment());
+                    if (mv.sourceText() != m.sourceText())
+                        goto outdateSource;
+                } else {
+                    switch (m.type()) {
+                    case TranslatorMessage::Finished:
+                    default:
+                        if (m.isPlural() == mv.isPlural()) {
+                            newType = TranslatorMessage::Finished;
+                        } else {
+                            newType = TranslatorMessage::Unfinished;
+                        }
+                        known++;
+                        break;
+                    case TranslatorMessage::Unfinished:
+                        newType = TranslatorMessage::Unfinished;
+                        known++;
+                        break;
+                    case TranslatorMessage::Obsolete:
+                        newType = TranslatorMessage::Unfinished;
+                        neww++;
+                    }
                 }
 
                 // Always get the filename and linenumber info from the
@@ -421,6 +434,7 @@ Translator merge(const Translator &tor, const Translator &virginTor,
                 m.setPlural(mv.isPlural());
                 m.setUtf8(mv.isUtf8());
                 m.setExtraComment(mv.extraComment());
+                m.setId(mv.id());
             }
         }
 
diff --git a/tools/linguist/shared/translator.cpp b/tools/linguist/shared/translator.cpp
index 0a59668..77faabd 100644
--- a/tools/linguist/shared/translator.cpp
+++ b/tools/linguist/shared/translator.cpp
@@ -306,10 +306,15 @@ int Translator::find(const TranslatorMessage &msg) const
 {
     for (int i = 0; i < m_messages.count(); ++i) {
         const TranslatorMessage &tmsg = m_messages.at(i);
-        if (msg.context() == tmsg.context()
-            && msg.sourceText() == tmsg.sourceText()
-            && msg.comment() == tmsg.comment())
-            return i;
+        if (msg.id().isEmpty() || tmsg.id().isEmpty()) {
+            if (msg.context() == tmsg.context()
+                && msg.sourceText() == tmsg.sourceText()
+                && msg.comment() == tmsg.comment())
+                return i;
+        } else {
+            if (msg.id() == tmsg.id())
+                return i;
+        }
     }
     return -1;
 }
@@ -423,8 +428,34 @@ void Translator::dropUiLines()
     }
 }
 
-struct TranslatorMessagePtr {
-    TranslatorMessagePtr(const TranslatorMessage &tm)
+struct TranslatorMessageIdPtr {
+    explicit TranslatorMessageIdPtr(const TranslatorMessage &tm)
+    {
+        ptr = &tm;
+    }
+
+    inline const TranslatorMessage *operator->() const
+    {
+        return ptr;
+    }
+
+    const TranslatorMessage *ptr;
+};
+
+Q_DECLARE_TYPEINFO(TranslatorMessageIdPtr, Q_MOVABLE_TYPE);
+
+inline int qHash(TranslatorMessageIdPtr tmp)
+{
+    return qHash(tmp->id());
+}
+
+inline bool operator==(TranslatorMessageIdPtr tmp1, TranslatorMessageIdPtr tmp2)
+{
+    return tmp1->id() == tmp2->id();
+}
+
+struct TranslatorMessageContentPtr {
+    explicit TranslatorMessageContentPtr(const TranslatorMessage &tm)
     {
         ptr = &tm;
     }
@@ -437,50 +468,81 @@ struct TranslatorMessagePtr {
     const TranslatorMessage *ptr;
 };
 
-Q_DECLARE_TYPEINFO(TranslatorMessagePtr, Q_MOVABLE_TYPE);
+Q_DECLARE_TYPEINFO(TranslatorMessageContentPtr, Q_MOVABLE_TYPE);
 
-inline int qHash(TranslatorMessagePtr tmp)
+inline int qHash(TranslatorMessageContentPtr tmp)
 {
-    return
-        qHash(tmp->context()) ^
-        qHash(tmp->sourceText()) ^
-        qHash(tmp->comment()) ^
-        qHash(tmp->id());
+    int hash = qHash(tmp->context()) ^ qHash(tmp->sourceText());
+    if (!tmp->sourceText().isEmpty())
+        // Special treatment for context comments (empty source).
+        hash ^= qHash(tmp->comment());
+    return hash;
 }
 
-inline bool operator==(TranslatorMessagePtr tmp1, TranslatorMessagePtr tmp2)
+inline bool operator==(TranslatorMessageContentPtr tmp1, TranslatorMessageContentPtr tmp2)
 {
+    if (tmp1->context() != tmp2->context() || tmp1->sourceText() != tmp2->sourceText())
+        return false;
     // Special treatment for context comments (empty source).
-    return (tmp1->context() == tmp2->context())
-        && tmp1->sourceText() == tmp2->sourceText()
-        && tmp1->id() == tmp2->id()
-        && (tmp1->sourceText().isEmpty() || tmp1->comment() == tmp2->comment());
+    if (tmp1->sourceText().isEmpty())
+        return true;
+    return tmp1->comment() == tmp2->comment();
 }
 
 Translator::Duplicates Translator::resolveDuplicates()
 {
     Duplicates dups;
-    QHash<TranslatorMessagePtr, int> refs;
+    QHash<TranslatorMessageIdPtr, int> idRefs;
+    QHash<TranslatorMessageContentPtr, int> contentRefs;
     for (int i = 0; i < m_messages.count();) {
         const TranslatorMessage &msg = m_messages.at(i);
-        QHash<TranslatorMessagePtr, int>::ConstIterator it = refs.constFind(msg);
-        if (it != refs.constEnd()) {
-            TranslatorMessage &omsg = m_messages[*it];
-            if (omsg.isUtf8() != msg.isUtf8() && !omsg.isNonUtf8()) {
-                // Dual-encoded message
-                omsg.setUtf8(true);
-                omsg.setNonUtf8(true);
-            } else {
-                // Duplicate
-                dups.byContents.insert(*it);
+        TranslatorMessage *omsg;
+        int oi;
+        QSet<int> *pDup;
+        if (!msg.id().isEmpty()) {
+            QHash<TranslatorMessageIdPtr, int>::ConstIterator it =
+                    idRefs.constFind(TranslatorMessageIdPtr(msg));
+            if (it != idRefs.constEnd()) {
+                oi = *it;
+                omsg = &m_messages[oi];
+                pDup = &dups.byId;
+                goto gotDupe;
+            }
+        }
+        {
+            QHash<TranslatorMessageContentPtr, int>::ConstIterator it =
+                    contentRefs.constFind(TranslatorMessageContentPtr(msg));
+            if (it != contentRefs.constEnd()) {
+                oi = *it;
+                omsg = &m_messages[oi];
+                if (msg.id().isEmpty() || omsg->id().isEmpty()) {
+                    if (!msg.id().isEmpty() && omsg->id().isEmpty()) {
+                        omsg->setId(msg.id());
+                        idRefs[TranslatorMessageIdPtr(*omsg)] = oi;
+                    }
+                    pDup = &dups.byContents;
+                    goto gotDupe;
+                }
+                // This is really a content dupe, but with two distinct IDs.
             }
-            if (!omsg.isTranslated() && msg.isTranslated())
-                omsg.setTranslations(msg.translations());
-            m_messages.removeAt(i);
+        }
+        if (!msg.id().isEmpty())
+            idRefs[TranslatorMessageIdPtr(msg)] = i;
+        contentRefs[TranslatorMessageContentPtr(msg)] = i;
+        ++i;
+        continue;
+      gotDupe:
+        if (omsg->isUtf8() != msg.isUtf8() && !omsg->isNonUtf8()) {
+            // Dual-encoded message
+            omsg->setUtf8(true);
+            omsg->setNonUtf8(true);
         } else {
-            refs[msg] = i;
-            ++i;
+            // Duplicate
+            pDup->insert(oi);
         }
+        if (!omsg->isTranslated() && msg.isTranslated())
+            omsg->setTranslations(msg.translations());
+        m_messages.removeAt(i);
     }
     return dups;
 }
@@ -488,12 +550,14 @@ Translator::Duplicates Translator::resolveDuplicates()
 void Translator::reportDuplicates(const Duplicates &dupes,
                                   const QString &fileName, bool verbose)
 {
-    if (!dupes.byContents.isEmpty()) {
+    if (!dupes.byId.isEmpty() || !dupes.byContents.isEmpty()) {
         if (!verbose) {
             qWarning("Warning: dropping duplicate messages in '%s'\n(try -verbose for more info).",
                      qPrintable(fileName));
         } else {
             qWarning("Warning: dropping duplicate messages in '%s':", qPrintable(fileName));
+            foreach (int i, dupes.byId)
+                qWarning("\n* ID: %s", qPrintable(message(i).id()));
             foreach (int j, dupes.byContents) {
                 const TranslatorMessage &msg = message(j);
                 qWarning("\n* Context: %s\n* Source: %s",
diff --git a/tools/linguist/shared/translator.h b/tools/linguist/shared/translator.h
index b957fa2..eec704a 100644
--- a/tools/linguist/shared/translator.h
+++ b/tools/linguist/shared/translator.h
@@ -134,7 +134,7 @@ public:
     void dropUiLines();
     void makeFileNamesAbsolute(const QDir &originalPath);
 
-    struct Duplicates { QSet<int> byContents; };
+    struct Duplicates { QSet<int> byId, byContents; };
     Duplicates resolveDuplicates();
     void reportDuplicates(const Duplicates &dupes, const QString &fileName, bool verbose);