Support cancelling item removal from the script

Waits only for onItemsRemoved() handler to finish.

Makes the item removal more stable when items in the tab change during
the onItemsRemoved() call.

docs/scripting-api.rst

@@ -1637,9 +1637,13 @@ unlike in GUI, where row numbers start from 1 by default.
 
    The target tab is returned by `selectedTab()`.
 
-   The removed items can be accessed with `selectedItemsData()`,
+   The items scheduled for removal can be accessed with `selectedItemsData()`,
    `selectedItemData()`, `selectedItems()` and `ItemSelection().current()`.
 
+   If the exit code is non-zero (for example `fail()` is called), items will
+   not be removed. But this can also cause a new items not to be added if the
+   tab is full.
+
 .. js:function:: onItemsChanged()
 
    Called when data in items change.

plugins/itemsync/itemsync.cpp

@@ -465,7 +465,7 @@ bool ItemSyncSaver::canRemoveItems(const QList<QModelIndex> &indexList, QString
                 QMessageBox::Yes ) == QMessageBox::Yes;
 }
 
-void ItemSyncSaver::itemsRemovedByUser(const QList<QModelIndex> &indexList)
+void ItemSyncSaver::itemsRemovedByUser(const QList<QPersistentModelIndex> &indexList)
 {
     if ( m_tabPath.isEmpty() )
         return;

plugins/itemsync/itemsync.h

@@ -50,7 +50,7 @@ class ItemSyncSaver final : public QObject, public ItemSaverInterface
 
     bool canRemoveItems(const QList<QModelIndex> &indexList, QString *error) override;
 
-    void itemsRemovedByUser(const QList<QModelIndex> &indexList) override;
+    void itemsRemovedByUser(const QList<QPersistentModelIndex> &indexList) override;
 
     QVariantMap copyItem(const QAbstractItemModel &model, const QVariantMap &itemData) override;
 

src/gui/clipboardbrowser.cpp

@@ -9,9 +9,6 @@
 #include "common/temporaryfile.h"
 #include "common/textdata.h"
 #include "common/timer.h"
-#include "gui/clipboarddialog.h"
-#include "gui/iconfactory.h"
-#include "gui/icons.h"
 #include "gui/pixelratio.h"
 #include "gui/theme.h"
 #include "item/itemeditor.h"
@@ -541,17 +538,21 @@ void ClipboardBrowser::dropIndexes(const QModelIndexList &indexes)
 {
     auto toRemove = toPersistentModelIndexList(indexes);
     std::sort( std::begin(toRemove), std::end(toRemove) );
+    dropIndexes(toRemove);
+}
 
+void ClipboardBrowser::dropIndexes(const QList<QPersistentModelIndex> &indexes)
+{
     const QPersistentModelIndex current = currentIndex();
-    const int first = toRemove.value(0).row();
+    const int first = indexes.value(0).row();
 
     // Remove ranges of rows instead of a single rows.
-    for (auto it1 = std::begin(toRemove); it1 != std::end(toRemove); ) {
+    for (auto it1 = std::begin(indexes); it1 != std::end(indexes); ) {
         if ( it1->isValid() ) {
             const auto firstRow = it1->row();
             auto rowCount = 0;
 
-            for ( ++it1, ++rowCount; it1 != std::end(toRemove)
+            for ( ++it1, ++rowCount; it1 != std::end(indexes)
                   && it1->isValid()
                   && it1->row() == firstRow + rowCount; ++it1, ++rowCount ) {}
 
@@ -756,15 +757,28 @@ void ClipboardBrowser::removeIndexes(const QModelIndexList &indexes, QString *er
             *error = "No valid rows specified";
     }
 
-    if ( !m_itemSaver->canRemoveItems(indexes, error) )
+    if ( !canRemoveItems(indexes, error) )
         return;
 
-    m_itemSaver->itemsRemovedByUser(indexes);
+    auto toRemove = toPersistentModelIndexList(indexes);
+    std::sort( std::begin(toRemove), std::end(toRemove) );
 
-    dropIndexes(indexes);
+    QPointer<QObject> self(this);
+    bool canRemove = true;
+    emit runOnRemoveItemsHandler(toRemove, &canRemove);
+    if (!canRemove) {
+        COPYQ_LOG("Item removal cancelled from script");
+        return;
+    }
+    if (!self)
+        return;
+
+    m_itemSaver->itemsRemovedByUser(toRemove);
+
+    dropIndexes(toRemove);
 }
 
-bool ClipboardBrowser::canRemoveItems(const QModelIndexList &indexes, QString *error) const
+bool ClipboardBrowser::canRemoveItems(const QModelIndexList &indexes, QString *error)
 {
     Q_ASSERT(m_itemSaver);
 
@@ -1122,6 +1136,8 @@ void ClipboardBrowser::mouseMoveEvent(QMouseEvent *event)
 
         QWidget *target = qobject_cast<QWidget*>(drag->target());
 
+        QPointer<QObject> self(this);
+
         // Move items only if target is this app.
         if (target == this || target == viewport()) {
             moveIndexes(indexesToRemove, m_dragTargetRow, &m, MoveType::Absolute);
@@ -1130,6 +1146,9 @@ void ClipboardBrowser::mouseMoveEvent(QMouseEvent *event)
         {
             removeIndexes(selected);
         }
+
+        if (!self)
+            return;
     }
 
     // Clear drag indicator.

src/gui/clipboardbrowser.h

@@ -98,7 +98,7 @@ class ClipboardBrowser final : public QListView
 
         void removeIndexes(const QModelIndexList &indexes, QString *error = nullptr);
 
-        bool canRemoveItems(const QModelIndexList &indexes, QString *error = nullptr) const;
+        bool canRemoveItems(const QModelIndexList &indexes, QString *error = nullptr);
 
         /** Render preview image with items. */
         QPixmap renderItemPreview(const QModelIndexList &indexes, int maxWidth, int maxHeight);
@@ -223,6 +223,7 @@ class ClipboardBrowser final : public QListView
 
     signals:
         void itemsAboutToBeRemoved(const QModelIndex &parent, int first, int last);
+        void runOnRemoveItemsHandler(const QList<QPersistentModelIndex> &indexes, bool *canRemove);
 
         /** Show list request. */
         void requestShow(const ClipboardBrowser *self);
@@ -352,6 +353,7 @@ class ClipboardBrowser final : public QListView
 
         /// Removes indexes without notifying or asking plugins.
         void dropIndexes(const QModelIndexList &indexes);
+        void dropIndexes(const QList<QPersistentModelIndex> &indexes);
 
         void focusEditedIndex();
 

src/gui/mainwindow.cpp

@@ -1279,10 +1279,13 @@ void MainWindow::onBrowserCreated(ClipboardBrowser *browser)
     connect( browser, &ClipboardBrowser::itemWidgetCreated,
              this, &MainWindow::onItemWidgetCreated );
 
-    connect( browser, &ClipboardBrowser::itemsAboutToBeRemoved,
-             browser, [this, browser](const QModelIndex &, int first, int last) {
-                 if (isScriptOverridden(ScriptOverrides::OnItemsRemoved))
-                    runItemHandlerScript(QStringLiteral("onItemsRemoved()"), browser, first, last);
+    connect( browser, &ClipboardBrowser::runOnRemoveItemsHandler,
+             browser, [this, browser](const QList<QPersistentModelIndex> &indexes, bool *canRemove) {
+                 if (isScriptOverridden(ScriptOverrides::OnItemsRemoved)) {
+                     QVariantMap data = createDataMap(mimeCurrentTab, browser->tabName());
+                     addSelectionData(&data, indexes);
+                     *canRemove = runEventHandlerScript(QStringLiteral("onItemsRemoved()"), data);
+                 }
              } );
     connect( browser->model(), &QAbstractItemModel::rowsInserted,
              browser, [this, browser](const QModelIndex &, int first, int last) {
@@ -1299,7 +1302,7 @@ void MainWindow::onBrowserCreated(ClipboardBrowser *browser)
              } );
 
     if (isScriptOverridden(ScriptOverrides::OnItemsLoaded)) {
-        runEventHandlerScript(
+        runScript(
             QStringLiteral("onItemsLoaded()"),
             createDataMap(mimeCurrentTab, browser->tabName()));
     }
@@ -2463,10 +2466,10 @@ Action *MainWindow::runScript(const QString &script, const QVariantMap &data)
     return act;
 }
 
-void MainWindow::runEventHandlerScript(const QString &script, const QVariantMap &data)
+bool MainWindow::runEventHandlerScript(const QString &script, const QVariantMap &data)
 {
     if (m_maxEventHandlerScripts == 0)
-        return;
+        return false;
 
     --m_maxEventHandlerScripts;
     if (m_maxEventHandlerScripts == 0)
@@ -2478,6 +2481,7 @@ void MainWindow::runEventHandlerScript(const QString &script, const QVariantMap
     action->waitForFinished();
     setUpdatesEnabled(hasUpdatesEnabled);
     ++m_maxEventHandlerScripts;
+    return !action->actionFailed() && action->exitCode() == 0;
 }
 
 void MainWindow::runItemHandlerScript(
@@ -2493,7 +2497,7 @@ void MainWindow::runItemHandlerScript(
 
      QVariantMap data = createDataMap(mimeCurrentTab, browser->tabName());
      addSelectionData(&data, indexes);
-     runEventHandlerScript(script, data);
+     runScript(script, data);
 }
 
 int MainWindow::findTabIndex(const QString &name)

src/gui/mainwindow.h

@@ -38,11 +38,11 @@ struct MainWindowOptions;
 struct NotificationButton;
 
 Q_DECLARE_METATYPE(QPersistentModelIndex)
-Q_DECLARE_METATYPE(QList<QPersistentModelIndex>)
 
 #if QT_VERSION >= QT_VERSION_CHECK(6,0,0)
 using NativeEventResult = qintptr;
 #else
+Q_DECLARE_METATYPE(QList<QPersistentModelIndex>)
 using NativeEventResult = long;
 #endif
 
@@ -629,7 +629,7 @@ class MainWindow final : public QMainWindow
     const Theme &theme() const;
 
     Action *runScript(const QString &script, const QVariantMap &data = QVariantMap());
-    void runEventHandlerScript(const QString &script, const QVariantMap &data = QVariantMap());
+    bool runEventHandlerScript(const QString &script, const QVariantMap &data);
     void runItemHandlerScript(
         const QString &script, const ClipboardBrowser *browser, int firstRow, int lastRow);
 

src/item/itemsaverwrapper.cpp

@@ -27,7 +27,7 @@ bool ItemSaverWrapper::canMoveItems(const QList<QModelIndex> &indexList)
     return m_saver->canMoveItems(indexList);
 }
 
-void ItemSaverWrapper::itemsRemovedByUser(const QList<QModelIndex> &indexList)
+void ItemSaverWrapper::itemsRemovedByUser(const QList<QPersistentModelIndex> &indexList)
 {
     return m_saver->itemsRemovedByUser(indexList);
 }

src/item/itemsaverwrapper.h

@@ -15,7 +15,7 @@ class ItemSaverWrapper : public ItemSaverInterface
 
     bool canMoveItems(const QList<QModelIndex> &indexList) override;
 
-    void itemsRemovedByUser(const QList<QModelIndex> &indexList) override;
+    void itemsRemovedByUser(const QList<QPersistentModelIndex> &indexList) override;
 
     QVariantMap copyItem(const QAbstractItemModel &model, const QVariantMap &itemData) override;
 

src/item/itemwidget.cpp

@@ -14,6 +14,7 @@
 #include <QMimeData>
 #include <QModelIndex>
 #include <QMouseEvent>
+#include <QPersistentModelIndex>
 #include <QTextEdit>
 #include <QUrl>
 #include <QWidget>
@@ -197,7 +198,7 @@ bool ItemSaverInterface::canMoveItems(const QList<QModelIndex> &)
     return true;
 }
 
-void ItemSaverInterface::itemsRemovedByUser(const QList<QModelIndex> &)
+void ItemSaverInterface::itemsRemovedByUser(const QList<QPersistentModelIndex> &)
 {
 }
 

src/item/itemwidget.h

@@ -17,6 +17,7 @@ class TestInterface;
 class QAbstractItemModel;
 class QIODevice;
 class QModelIndex;
+class QPersistentModelIndex;
 class QSettings;
 class QTextEdit;
 class QWidget;
@@ -204,7 +205,7 @@ class ItemSaverInterface
     /**
      * Called when items are being deleted by user.
      */
-    virtual void itemsRemovedByUser(const QList<QModelIndex> &indexList);
+    virtual void itemsRemovedByUser(const QList<QPersistentModelIndex> &indexList);
 
     /**
      * Return copy of items data.

src/scriptable/scriptable.cpp

@@ -345,7 +345,7 @@ bool isGuiApplication()
 
 bool isOverridden(const QJSValue &globalObject, const QString &property)
 {
-    return globalObject.property(property).property(QStringLiteral("_copyq")).toInt() != 1;
+    return globalObject.property(property).property(QStringLiteral("_copyq")).toString() != property;
 }
 
 } // namespace
@@ -393,7 +393,7 @@ Scriptable::Scriptable(
                 "if (_copyqHasUncaughtException) throw _copyqUncaughtException;"
                 "return v;"
             "};"
-            "f._copyq = 1;"
+            "f._copyq = name;"
             "return f;"
         "})"
     ));

src/tests/tests.cpp

@@ -4019,6 +4019,42 @@ void Tests::scriptOnItemsRemoved()
     RUN("tab" << tab1 << "add(3,2,1,0)", "");
     RUN("tab" << tab1 << "remove(1,2)", "");
     WAIT_ON_OUTPUT("separator" << "," << "read(0,1,2,)", "R1:2,R0:1,");
+
+    // Cancel item removal
+    const auto script2 = R"(
+        setCommands([
+            {
+                isScript: true,
+                cmd: "global.onItemsRemoved = global.fail",
+            },
+        ])
+        )";
+    RUN(script2, "");
+    const auto tab2 = testTab(2);
+    RUN("tab" << tab2 << "add(3,2,1,0)", "");
+    RUN("tab" << tab2 << "remove(1,2)", "");
+    waitFor(1000);
+    RUN("tab" << tab2 << "separator" << "," << "read(0,1,2,3,4)", "0,1,2,3,");
+
+    // Avoid crash if the tab itself is removed while removing items
+    const auto script3 = R"(
+        setCommands([
+            {
+                isScript: true,
+                cmd: `
+                  global.onItemsRemoved = function() {
+                    removeTab(selectedTab())
+                  }
+                `
+            },
+        ])
+        )";
+    RUN(script3, "");
+    const auto tab3 = testTab(3);
+    RUN("tab" << tab3 << "add(3,2,1,0)", "");
+    RUN("tab" << tab3 << "remove(1,2)", "");
+    waitFor(1000);
+    RUN("tab" << tab3 << "separator" << "," << "read(0,1,2,3,4)", ",,,,");
 }
 
 void Tests::scriptOnItemsAdded()
@@ -4029,10 +4065,10 @@ void Tests::scriptOnItemsAdded()
                 isScript: true,
                 cmd: `
                   global.onItemsAdded = function() {
-                    if (selectedTab() == tab()[0]) abort();
-                    items = ItemSelection().current().items();
-                    tab(tab()[0]);
-                    add("A:" + str(items[0][mimeText]));
+                    sel = ItemSelection().current();
+                    items = sel.items();
+                    items[0][mimeText] = "A:" + str(items[0][mimeText])
+                    sel.setItems(items);
                   }
                 `
             },
@@ -4041,7 +4077,7 @@ void Tests::scriptOnItemsAdded()
     RUN(script, "");
     const auto tab1 = testTab(1);
     RUN("tab" << tab1 << "add(1,0)", "");
-    WAIT_ON_OUTPUT("separator" << "," << "read(0,1,2)", "A:0,A:1,");
+    WAIT_ON_OUTPUT("tab" << tab1 << "separator" << "," << "read(0,1,2)", "A:0,A:1,");
 }
 
 void Tests::scriptOnItemsChanged()
@@ -4103,16 +4139,20 @@ void Tests::scriptEventMaxRecursion()
         setCommands([
             {
                 isScript: true,
-                cmd: 'global.onItemsAdded = function() { add("A") }'
+                cmd: `global.onItemsRemoved = function() {
+                    const toRemove = str(selectedItemData(0)[mimeText]);
+                    const newItem = (toRemove == "X") ? "A" : ("WRONG:" + toRemove);
+                    add(newItem);
+                    remove(size()-1);
+                }`
             },
         ])
         )";
     RUN(script, "");
-    RUN("add(1,0)", "");
-    WAIT_ON_OUTPUT("length", "22\n");
+    RUN("add('X'); remove(0)", "");
+    WAIT_ON_OUTPUT("separator" << "," << "read(0,1,2,3,4,5,6,7,8,9,10)", "A,A,A,A,A,A,A,A,A,A,");
     waitFor(200);
-    RUN("separator" << "," << "read(0,1,2)", "A,A,A");
-    RUN("length", "22\n");
+    RUN("separator" << "," << "read(0,1,2,3,4,5,6,7,8,9,10)", "A,A,A,A,A,A,A,A,A,A,");
 }
 
 void Tests::scriptSlowCollectOverrides()