From 72f5a2db089178c1e9eca82ba5a9a97fff2a49dc Mon Sep 17 00:00:00 2001 From: Simone Gaiarin Date: Wed, 5 Aug 2020 22:49:56 +0000 Subject: [PATCH] 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 | 2 +- autotests/annotationtoolbartest.cpp | 21 +++++++++++++++-- ui/annotationactionhandler.cpp | 35 ++++++++++++++++++++++++----- ui/data/tools.xml | 5 +++++ ui/data/toolsQuick.xml | 18 ++++++++++----- ui/pageviewannotator.cpp | 20 ++++++++++++++--- 6 files changed, 84 insertions(+), 17 deletions(-) diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt index 4a0ab2260..7ce898dda 100644 --- a/autotests/CMakeLists.txt +++ b/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 ) diff --git a/autotests/annotationtoolbartest.cpp b/autotests/annotationtoolbartest.cpp index ce30e3f33..cc54f90dd 100644 --- a/autotests/annotationtoolbartest.cpp +++ b/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(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(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 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() diff --git a/ui/annotationactionhandler.cpp b/ui/annotationactionhandler.cpp index f1cf7c6c8..e860b20d8 100644 --- a/ui/annotationactionhandler.cpp +++ b/ui/annotationactionhandler.cpp @@ -74,7 +74,15 @@ public: } QAction *selectActionItem(KSelectAction *aList, QAction *aCustomCurrent, double value, const QList &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 &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 &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); } diff --git a/ui/data/tools.xml b/ui/data/tools.xml index 329f253c7..0b9df17a8 100644 --- a/ui/data/tools.xml +++ b/ui/data/tools.xml @@ -1,5 +1,10 @@ - + - + - + - + - + - + diff --git a/ui/pageviewannotator.cpp b/ui/pageviewannotator.cpp index 005fc7433..9ac5ce06a 100644 --- a/ui/pageviewannotator.cpp +++ b/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(); }