Drop the unreachable commentColor back-compat in PdfViewerConfig

PdfViewerConfig::fromJson() seeded all three tools from a legacy shared
`commentColor` key when `tools` was absent. That branch cannot run.

ConfigMgr2::init() applies the user's document as an RFC 7386 merge patch on top
of the default-constructed MainConfig's toJson(), and toJson() always emits
`tools`. So by the time fromJson() sees the object, `tools` is present
unconditionally and the guard is always false. The branch only ever executed
from a direct fromJson() call -- which is exactly what its test did, so the gate
asserted the dead path rather than catching it.

Nothing is lost by removing it. `commentColor` existed only in the local
pdf.js-v6 commit and was never on master, so no config on disk anywhere carries
it. And absent-key safety for the replacement keys is now covered better than
the removed test covered it: testAbsentKeyKeepsTheCppDefaultForEveryField drops
each leaf key of the defaults document in turn and reloads through a real
ConfigMgr2::init(), which includes tools.*.color, tools.ink.width and
tools.freetext.fontSize.

The remaining `tools`-absent fallback is kept and its comment now says what it
is actually for: a direct fromJson() call, not a migration.

Full suite 216/216.

Author: tamlok

src/core/pdfviewerconfig.cpp

@@ -26,20 +26,9 @@ void PdfViewerConfig::fromJson(const QJsonObject &p_jobj) {
     m_toolOptions.insert(tool, ToolOptions());
   }
 
-  // Back-compat: the shared `commentColor` shipped in this branch before the
-  // per-tool split. Seeding all three from it stops an already-picked value
-  // from silently resetting to yellow.
-  const auto legacyColor = p_jobj.value(QStringLiteral("commentColor")).toString();
-  const bool hasTools = p_jobj.value(QStringLiteral("tools")).isObject();
-  if (!hasTools && CommentColor::isValid(legacyColor)) {
-    for (auto it = m_toolOptions.begin(); it != m_toolOptions.end(); ++it) {
-      it->m_color = legacyColor;
-    }
-    return;
-  }
-
-  // Absent key keeps the C++ default, which is what makes this safe to add
-  // without a config migration.
+  // Absent key keeps the C++ default. In practice ConfigMgr2::init() merges the
+  // user's document over the defaults document, so `tools` is always present by
+  // the time this runs; the fallback matters only for a direct fromJson() call.
   const auto toolsObj = p_jobj.value(QStringLiteral("tools")).toObject();
   for (const auto &tool : toolNames()) {
     m_toolOptions.insert(tool, toolOptionsFromJson(tool, toolsObj.value(tool).toObject()));

tests/core/test_configmgr2.cpp

@@ -1219,31 +1219,6 @@ void TestConfigMgr2::testPdfToolOptions_defaultsAndRoundTrips() {
     QCOMPARE(pdfConfig.getToolOptions(ink).m_color, CommentColor::defaultToken());
     QCOMPARE(pdfConfig.getToolOptions(ink).m_width, PdfInkAnchor::maxWidth());
   }
-
-  {
-    // Back-compat: the shared `commentColor` this branch shipped before the
-    // per-tool split seeds all three, so an already-picked value does not
-    // silently reset to yellow.
-    MainConfig config(m_configMgr);
-    auto &pdfConfig = config.getEditorConfig().getPdfViewerConfig();
-
-    QJsonObject obj;
-    obj.insert(QStringLiteral("commentColor"), QStringLiteral("pink"));
-    pdfConfig.fromJson(obj);
-    for (const auto &tool : PdfViewerConfig::toolNames()) {
-      QCOMPARE(pdfConfig.getToolOptions(tool).m_color, QStringLiteral("pink"));
-    }
-
-    // `tools` wins when both are present.
-    QJsonObject tools;
-    QJsonObject inkObj;
-    inkObj.insert(QStringLiteral("color"), QStringLiteral("green"));
-    tools.insert(PdfToolOptions::inkTool(), inkObj);
-    obj.insert(QStringLiteral("tools"), tools);
-    pdfConfig.fromJson(obj);
-    QCOMPARE(pdfConfig.getToolOptions(PdfToolOptions::inkTool()).m_color, QStringLiteral("green"));
-    QCOMPARE(pdfConfig.getToolOptions(highlight).m_color, CommentColor::defaultToken());
-  }
 }
 
 // The Task 0 normalization table, one row per class, asserted on BOTH the