Find built-in tool corresponding to quick tool at runtime

In this way it is possible to drop the `sourceId` attribute from the quick tools definition. This simplifies the code logic and makes it easier to update user settings from the previous version of Okular (because there is no need to add the attribute `sourceId`

This also fixes the crash due to the fact that `sourceId` was not correctly created when a quick annotation is created from the Annotation page of Okualr Settings.

BUG: 424810
FIXED-IN: 1.11.0

autotests/CMakeLists.txt

@@ -90,7 +90,7 @@ if(KF5Activities_FOUND AND BUILD_DESKTOP)
 endif()
 
 if(BUILD_DESKTOP)
-    ecm_add_test(annotationtoolbartest.cpp ../shell/okular_main.cpp ../shell/shellutils.cpp ../shell/shell.cpp
+    ecm_add_test(annotationtoolbartest.cpp ../shell/okular_main.cpp ../shell/shellutils.cpp ../shell/shell.cpp closedialoghelper.cpp
         TEST_NAME "annotationtoolbartest"
         LINK_LIBRARIES Qt5::Test okularpart
     )

autotests/annotationtoolbartest.cpp

@@ -26,6 +26,7 @@
 #include "../shell/shell.h"
 #include "../shell/shellutils.h"
 #include "../ui/pageview.h"
+#include "closedialoghelper.h"
 
 namespace Okular
 {
@@ -146,7 +147,7 @@ void AnnotationToolBarTest::testAnnotationToolBar()
     QVERIFY(annToolBar);
 
     // Check config action default enabled states
-    QAction *aQuickTools = part->actionCollection()->action(QStringLiteral("annotation_favorites"));
+    KSelectAction *aQuickTools = qobject_cast<KSelectAction *>(part->actionCollection()->action(QStringLiteral("annotation_favorites")));
     QAction *aAddToQuickTools = part->actionCollection()->action(QStringLiteral("annotation_bookmark"));
     QAction *aAdvancedSettings = part->actionCollection()->action(QStringLiteral("annotation_settings_advanced"));
     QAction *aContinuousMode = part->actionCollection()->action(QStringLiteral("annotation_settings_pin"));
@@ -157,7 +158,7 @@ void AnnotationToolBarTest::testAnnotationToolBar()
 
     // Ensure that the 'Quick Annotations' action is correctly populated
     // (at least the 'Configure Annotations...' action must be present)
-    QVERIFY(!qobject_cast<KSelectAction *>(aQuickTools)->actions().isEmpty());
+    QVERIFY(!aQuickTools->actions().isEmpty());
 
     // Test annotation toolbar visibility triggers
     QAction *toggleAnnotationToolBar = part->actionCollection()->action(QStringLiteral("mouse_toggle_annotate"));
@@ -224,6 +225,22 @@ void AnnotationToolBarTest::testAnnotationToolBar()
     aContinuousMode->trigger();
     QCOMPARE(simulateAddPopupAnnotation(part, mouseX, mouseY), true);
     QCOMPARE(simulateAddPopupAnnotation(part, mouseX, mouseY), false);
+
+    // Test adding a tool to the quick tool list using the bookmark action
+    QScopedPointer<TestingUtils::CloseDialogHelper> closeDialogHelper;
+    closeDialogHelper.reset(new TestingUtils::CloseDialogHelper(QDialogButtonBox::Ok));
+    QAction *aEllipse = part->actionCollection()->action(QStringLiteral("annotation_ellipse"));
+    aEllipse->trigger();
+    QVERIFY(aEllipse->isChecked());
+    int quickActionCount = aQuickTools->actions().size();
+    aAddToQuickTools->trigger();
+    QCOMPARE(aQuickTools->actions().size(), quickActionCount + 1);
+    // Test that triggering a Quick Annotation action checks the corresponding built-in annotation action
+    aQuickTools->actions().at(5)->trigger();
+    QVERIFY(aPopupNote->isChecked());
+    // Test again for tool just added to the quick tools using the bookmark button
+    aQuickTools->actions().at(6)->trigger();
+    QVERIFY(aEllipse->isChecked());
 }
 
 void AnnotationToolBarTest::testAnnotationToolBar_data()

ui/annotationactionhandler.cpp

@@ -74,7 +74,15 @@ public:
     }
 
     QAction *selectActionItem(KSelectAction *aList, QAction *aCustomCurrent, double value, const QList<double> &defaultValues, const QIcon &icon, const QString &label);
-    void selectStampActionItem(const QString &stampIconName);
+    /**
+     * @short Adds a custom stamp annotation action to the stamp list when the stamp is not a default stamp
+     *
+     * When stampIconName cannot be found among the default stamps, this method creates a new action
+     * for the custom stamp annotation and adds it to the stamp action combo box. If a custom action
+     * is already present in the list, it is removed before adding the new custom action. If stampIconName
+     * matches a default stamp, any existing stamp annotation action is removed.
+     */
+    void maybeUpdateCustomStampAction(const QString &stampIconName);
     void parseTool(int toolID);
 
     void updateConfigActions(const QString &annotType = QLatin1String(""));
@@ -168,7 +176,7 @@ QAction *AnnotationActionHandlerPrivate::selectActionItem(KSelectAction *aList,
     return aCustom;
 }
 
-void AnnotationActionHandlerPrivate::selectStampActionItem(const QString &stampIconName)
+void AnnotationActionHandlerPrivate::maybeUpdateCustomStampAction(const QString &stampIconName)
 {
     auto it = std::find_if(StampAnnotationWidget::defaultStamps.begin(), StampAnnotationWidget::defaultStamps.end(), [&stampIconName](const QPair<QString, QString> &element) { return element.second == stampIconName; });
     bool defaultStamp = it != StampAnnotationWidget::defaultStamps.end();
@@ -242,7 +250,7 @@ void AnnotationActionHandlerPrivate::parseTool(int toolID)
     // if the tool is a custom stamp, insert a new action in the stamp list
     if (annotType == QStringLiteral("stamp")) {
         QString stampIconName = annElement.attribute(QStringLiteral("icon"));
-        selectStampActionItem(stampIconName);
+        maybeUpdateCustomStampAction(stampIconName);
     }
 
     updateConfigActions(annotType);
@@ -480,11 +488,28 @@ void AnnotationActionHandlerPrivate::slotStampToolSelected(const QString &stamp)
 void AnnotationActionHandlerPrivate::slotQuickToolSelected(int favToolID)
 {
     int toolID = annotator->setQuickTool(favToolID); // always triggers an unuseful reparsing
-    QAction *favToolAction = agTools->actions().at(toolID - 1);
+    int indexOfActionInGroup = toolID - 1;
+    if (toolID == PageViewAnnotator::STAMP_TOOL_ID) {
+        // if the quick tool is a stamp we need to find its corresponding built-in tool action and select it
+        QDomElement favToolElement = annotator->quickTool(favToolID);
+        QDomElement engineElement = favToolElement.firstChildElement(QStringLiteral("engine"));
+        QDomElement annotationElement = engineElement.firstChildElement(QStringLiteral("annotation"));
+        QString stampIconName = annotationElement.attribute(QStringLiteral("icon"));
+
+        auto it = std::find_if(StampAnnotationWidget::defaultStamps.begin(), StampAnnotationWidget::defaultStamps.end(), [&stampIconName](const QPair<QString, QString> &element) { return element.second == stampIconName; });
+        if (it != StampAnnotationWidget::defaultStamps.end()) {
+            int stampActionIndex = std::distance(StampAnnotationWidget::defaultStamps.begin(), it);
+            indexOfActionInGroup = PageViewAnnotator::STAMP_TOOL_ID + stampActionIndex - 1;
+        } else {
+            maybeUpdateCustomStampAction(stampIconName);
+            indexOfActionInGroup = agTools->actions().size() - 1;
+        }
+    }
+    QAction *favToolAction = agTools->actions().at(indexOfActionInGroup);
     if (!favToolAction->isChecked()) {
         // action group workaround: activates the action slot calling selectTool
         //                          when new tool if different from the selected one
-        favToolAction->setChecked(true);
+        favToolAction->trigger();
     } else {
         selectTool(toolID);
     }

ui/data/tools.xml

@@ -1,5 +1,10 @@
 <!DOCTYPE okularAnnotatingTools>
 <!--
+Tool
+  [id]              {int = unique id identifying the built-in tool}
+  [name]            {string = name of the tool displayed when configuring it}
+  [type]            {highlight, underline, squiggly, strikeout, typewriter, note-inline, note-linked, ink, straight-line, rectangle, ellipse, polygon, stamp}
+
 Engine
   TextSelector
     [color]         {RGB HEX}

ui/data/toolsQuick.xml

@@ -1,31 +1,37 @@
 <!DOCTYPE okularQuickAnnotatingTools>
+<!--
+Refer to tools.xml for the documentation of the attribues not documented here.
+
+Tool
+  [id]              {int = unique id identifying the quick tool}
+-->
 <quickAnnotatingTools>
-    <tool id="1" sourceId="1" type="highlight" name="Yellow Highlighter">
+    <tool id="1" type="highlight" name="Yellow Highlighter">
         <engine type="TextSelector" color="#ffff00">
             <annotation type="Highlight" color="#ffffff00"/>
         </engine>
     </tool>
-    <tool id="2" sourceId="1" type="highlight" name="Green Highlighter">
+    <tool id="2" type="highlight" name="Green Highlighter">
         <engine type="TextSelector" color="#00ff00">
             <annotation type="Highlight" color="#ff00ff00"/>
         </engine>
     </tool>
-    <tool id="3" sourceId="2" type="underline">
+    <tool id="3" type="underline">
         <engine type="TextSelector" color="#ff0000">
             <annotation type="Underline" color="#ffff0000"/>
         </engine>
     </tool>
-    <tool id="4" sourceId="5" type="typewriter" name="Insert Text">
+    <tool id="4" type="typewriter" name="Insert Text">
         <engine type="PickPoint" block="true">
             <annotation type="Typewriter" color="#00ffffff" textColor="#000000" width="0"/>
         </engine>
     </tool>
-    <tool id="5" sourceId="6" type="note-inline">
+    <tool id="5" type="note-inline">
         <engine type="PickPoint" color="#ffff00" hoverIcon="tool-note-inline" block="true">
             <annotation type="FreeText" color="#ffffff00" textColor="#ff000000"/>
         </engine>
     </tool>
-    <tool id="6" sourceId="7" type="note-linked">
+    <tool id="6" type="note-linked">
         <engine type="PickPoint" color="#ffff00" hoverIcon="tool-note">
             <annotation type="Text" color="#ffffff00" icon="Note" />
         </engine>

ui/pageviewannotator.cpp

@@ -690,6 +690,21 @@ public:
         return !oldTool.isNull();
     }
 
+    int findToolId(const QString &type)
+    {
+        int toolID = -1;
+        // FIXME: search from left. currently searching from right side as a workaround to avoid matching
+        // straight line tools to the arrow tool, which is also of type straight-line
+        QDomElement toolElement = m_toolsDefinition.documentElement().lastChildElement();
+        while (!toolElement.isNull() && toolElement.attribute(QStringLiteral("type")) != type) {
+            toolElement = toolElement.previousSiblingElement();
+        }
+        if (!toolElement.isNull()) {
+            toolID = toolElement.attribute(QStringLiteral("id")).toInt();
+        }
+        return toolID;
+    }
+
 private:
     QDomDocument m_toolsDefinition;
     int m_toolsCount;
@@ -1262,8 +1277,8 @@ int PageViewAnnotator::setQuickTool(int favToolID)
 {
     int toolId = -1;
     QDomElement favToolElement = m_quickToolsDefinition->tool(favToolID);
-    if (!favToolElement.isNull() && favToolElement.hasAttribute(QStringLiteral("sourceId"))) {
-        toolId = favToolElement.attribute(QStringLiteral("sourceId")).toInt();
+    if (!favToolElement.isNull()) {
+        toolId = m_toolsDefinition->findToolId(favToolElement.attribute(QStringLiteral("type")));
         if (m_toolsDefinition->updateTool(favToolElement, toolId))
             saveAnnotationTools();
     }
@@ -1353,7 +1368,6 @@ void PageViewAnnotator::addToQuickAnnotations()
     // store name attribute only if the user specified a customized name
     if (!itemText.isEmpty())
         toolElement.setAttribute(QStringLiteral("name"), itemText);
-    toolElement.setAttribute(QStringLiteral("sourceId"), sourceToolElement.attribute(QStringLiteral("id")));
     m_quickToolsDefinition->appendTool(toolElement);
     saveAnnotationTools();
 }