Remove tree undo and fix drop on root issue

novelwriter/gui/projtree.py

@@ -126,11 +126,6 @@ class GuiProjectView(QWidget):
         self.keyGoDown.setContext(Qt.ShortcutContext.WidgetShortcut)
         self.keyGoDown.activated.connect(lambda: self.projTree.moveToLevel(1))
 
-        self.keyUndoMv = QShortcut(self.projTree)
-        self.keyUndoMv.setKey("Ctrl+Shift+Z")
-        self.keyUndoMv.setContext(Qt.ShortcutContext.WidgetShortcut)
-        self.keyUndoMv.activated.connect(lambda: self.projTree.undoLastMove())
-
         self.keyContext = QShortcut(self.projTree)
         self.keyContext.setKey("Ctrl+.")
         self.keyContext.setContext(Qt.ShortcutContext.WidgetShortcut)
@@ -320,9 +315,6 @@ class GuiProjectToolBar(QWidget):
         self.aCollapse = self.mMore.addAction(self.tr("Collapse All"))
         self.aCollapse.triggered.connect(lambda: self.projTree.setExpandedFromHandle(None, False))
 
-        self.aMoreUndo = self.mMore.addAction(self.tr("Undo Move"))
-        self.aMoreUndo.triggered.connect(lambda: self.projTree.undoLastMove())
-
         self.aEmptyTrash = self.mMore.addAction(self.tr("Empty Trash"))
         self.aEmptyTrash.triggered.connect(lambda: self.projTree.emptyTrash())
 
@@ -475,7 +467,6 @@ class GuiProjectTree(QTreeWidget):
 
         # Internal Variables
         self._treeMap = {}
-        self._lastMove = {}
         self._timeChanged = 0.0
         self._popAlert = None
 
@@ -522,8 +513,8 @@ class GuiProjectTree(QTreeWidget):
         # But don't allow drop on root level
         # Due to a bug, this stops working somewhere between Qt 5.15.3
         # and 5.15.8, so this is also blocked in dropEvent (see #1569)
-        # trRoot = self.invisibleRootItem()
+        trRoot = self.invisibleRootItem()
-        # trRoot.setFlags(trRoot.flags() ^ Qt.ItemFlag.ItemIsDropEnabled)
+        trRoot.setFlags(trRoot.flags() ^ Qt.ItemFlag.ItemIsDropEnabled)
 
         # Cached values
         self._lblActive = self.tr("Active")
@@ -572,7 +563,6 @@ class GuiProjectTree(QTreeWidget):
         """Clear the GUI content and the related map."""
         self.clear()
         self._treeMap = {}
-        self._lastMove = {}
         self._timeChanged = 0.0
         return
 
@@ -743,7 +733,6 @@ class GuiProjectTree(QTreeWidget):
 
             cItem = pItem.takeChild(tIndex)
             pItem.insertChild(nIndex, cItem)
-            self._recordLastMove(cItem, pItem, tIndex)
 
         self._alertTreeChange(tHandle, flush=True)
         self.setCurrentItem(tItem)
@@ -932,7 +921,6 @@ class GuiProjectTree(QTreeWidget):
         trItemT.addChild(trItemC)
 
         self._postItemMove(tHandle)
-        self._recordLastMove(trItemS, trItemP, tIndex)
         self._alertTreeChange(tHandle, flush=flush)
 
         logger.debug("Moved item '%s' to Trash", tHandle)
@@ -1093,45 +1081,6 @@ class GuiProjectTree(QTreeWidget):
             logger.info("%d item(s) added to the project tree", count)
         return
 
-    def undoLastMove(self) -> bool:
-        """Attempt to undo the last action."""
-        srcItem = self._lastMove.get("item", None)
-        dstItem = self._lastMove.get("parent", None)
-        dstIndex = self._lastMove.get("index", None)
-
-        srcOK = isinstance(srcItem, QTreeWidgetItem)
-        dstOk = isinstance(dstItem, QTreeWidgetItem)
-        if not srcOK or not dstOk or dstIndex is None:
-            logger.debug("No tree move to undo")
-            return False
-
-        if srcItem not in self._treeMap.values():
-            logger.warning("Source item no longer exists")
-            return False
-
-        if dstItem not in self._treeMap.values():
-            logger.warning("Previous parent item no longer exists")
-            return False
-
-        dstIndex = min(max(0, dstIndex), dstItem.childCount())
-        sHandle = srcItem.data(self.C_DATA, self.D_HANDLE)
-        dHandle = dstItem.data(self.C_DATA, self.D_HANDLE)
-        logger.debug("Moving item '%s' back to '%s', index %d", sHandle, dHandle, dstIndex)
-
-        self.propagateCount(sHandle, 0)
-        parItem = srcItem.parent()
-        srcIndex = parItem.indexOfChild(srcItem)
-        movItem = parItem.takeChild(srcIndex)
-        dstItem.insertChild(dstIndex, movItem)
-
-        self._postItemMove(sHandle)
-        self._alertTreeChange(sHandle, flush=True)
-
-        self.setCurrentItem(movItem)
-        self._lastMove = {}
-
-        return True
-
     def getSelectedHandle(self) -> str | None:
         """Get the currently selected handle. If multiple items are
         selected, return the first.
@@ -1468,30 +1417,25 @@ class GuiProjectTree(QTreeWidget):
         """Overload the drop item event to ensure the drag and drop
         action is allowed, and update relevant data.
         """
-        if not self.indexAt(event.pos()).isValid():
+        tItem = self.itemAt(event.pos())
-            # Make sure nothing can be dropped on invisible root
+        dropOn = self.dropIndicatorPosition() == QAbstractItemView.DropIndicatorPosition.OnItem
+        # Make sure nothing can be dropped on invisible root (see #1569)
+        if not tItem or tItem.parent() is None and not dropOn:
             logger.error("Invalid drop location")
             event.ignore()
             return
 
-        mItems: dict[str, tuple[QTreeWidgetItem, QTreeWidgetItem, bool]] = {}
+        mItems: dict[str, tuple[QTreeWidgetItem, bool]] = {}
         sItems = self.selectedItems()
         if sItems and (parent := sItems[0].parent()) and all(x.parent() is parent for x in sItems):
             for sItem in sItems:
-                if (pItem := sItem.parent()):
+                mHandle = str(sItem.data(self.C_DATA, self.D_HANDLE))
-                    mHandle = str(sItem.data(self.C_DATA, self.D_HANDLE))
+                mItems[mHandle] = (sItem, sItem.isExpanded())
-                    mItems[mHandle] = (sItem, pItem, sItem.isExpanded())
-                else:
-                    logger.error("Cannot drag and drop a root item")
-                    event.ignore()
-                    return
-
-            for mHandle in mItems:
                 self.propagateCount(mHandle, 0)
 
             super().dropEvent(event)
 
-            for mHandle, (sItem, pItem, isExpanded) in mItems.items():
+            for mHandle, (sItem, isExpanded) in mItems.items():
                 self._postItemMove(mHandle)
                 sItem.setExpanded(isExpanded)
                 self._alertTreeChange(mHandle, flush=False)
@@ -1530,8 +1474,8 @@ class GuiProjectTree(QTreeWidget):
                 SHARED.project.index.reIndexHandle(mHandle)
             self.setTreeItemValues(mHandle)
 
-        # Trigger dependent updates
+        # Update word count
-        self.propagateCount(tHandle, nwItemS.wordCount)
+        self.propagateCount(tHandle, nwItemS.wordCount, countChildren=True)
 
         return
 
@@ -1884,16 +1828,4 @@ class GuiProjectTree(QTreeWidget):
 
         return
 
-    def _recordLastMove(self, srcItem: QTreeWidgetItem,
-                        parItem: QTreeWidgetItem, parIndex: int) -> None:
-        """Record the last action so that it can be undone."""
-        prevItem = self._lastMove.get("item", None)
-        if prevItem is None or srcItem != prevItem:
-            self._lastMove = {
-                "item": srcItem,
-                "parent": parItem,
-                "index": parIndex,
-            }
-        return
-
 # END Class GuiProjectTree

tests/test_gui/test_gui_projtree.py

@@ -251,11 +251,6 @@ def testGuiProjTree_MoveItems(qtbot, monkeypatch, nwGUI, projPath, mockRnd):
         C.hChapterDir, C.hChapterDoc, C.hSceneDoc,
         "0000000000010", "0000000000012", "0000000000011",
     ]
-    assert projTree.undoLastMove() is True
-    assert projTree.getTreeFromHandle(C.hChapterDir) == [
-        C.hChapterDir, C.hChapterDoc, C.hSceneDoc,
-        "0000000000010", "0000000000011", "0000000000012",
-    ]
 
     # Root Folder
     # ===========
@@ -988,17 +983,14 @@ def testGuiProjTree_DragAndDrop(qtbot, monkeypatch, caplog, nwGUI: GuiMain, proj
     mouse = Qt.MouseButton.LeftButton
     modifier = Qt.KeyboardModifier.NoModifier
 
-    # Move an item
+    projTree.saveTreeOrder()
-    # The actual move is blocked, but the undo history should record
+    treeOrder = SHARED.project.tree._order
-    # the event, although without an actual move implied
+
-    assert projTree._lastMove == {}
+    # Move an item, but no selection
     event = QDropEvent(nPos, action, mime, mouse, modifier)
     projTree.dropEvent(event)
-    assert projTree._lastMove == {
+    projTree.saveTreeOrder()
-        "item": projTree._getTreeItem(C.hSceneDoc),
+    assert SHARED.project.tree._order == treeOrder
-        "parent": projTree._getTreeItem(C.hChapterDir),
-        "index": 1,
-    }
 
     # Invalid location
     caplog.clear()
@@ -1006,23 +998,19 @@ def testGuiProjTree_DragAndDrop(qtbot, monkeypatch, caplog, nwGUI: GuiMain, proj
     projTree.dropEvent(event)
     assert event.isAccepted() is False
     assert "Invalid drop location" in caplog.text
-
+    projTree.saveTreeOrder()
-    # No item selected
+    assert SHARED.project.tree._order == treeOrder
-    caplog.clear()
-    event = QDropEvent(nPos, action, mime, mouse, modifier)
-    projTree.clearSelection()
-    projTree.dropEvent(event)
-    assert event.isAccepted() is False
-    assert "Invalid drag and drop event" in caplog.text
 
     # Root item selected
     caplog.clear()
     event = QDropEvent(nPos, action, mime, mouse, modifier)
     projTree.clearSelection()
-    projTree.setSelectedHandle(C.hNovelRoot, True)
+    projTree._getTreeItem(C.hTitlePage).setSelected(True)  # type: ignore
+    projTree._getTreeItem(C.hNovelRoot).setSelected(True)  # type: ignore
     projTree.dropEvent(event)
     assert event.isAccepted() is False
-    assert "Invalid drag and drop event" in caplog.text
+    projTree.saveTreeOrder()
+    assert SHARED.project.tree._order == treeOrder
 
     # qtbot.stop()
 
@@ -1070,22 +1058,6 @@ def testGuiProjTree_Other(qtbot, monkeypatch, nwGUI: GuiMain, projPath, mockRnd)
     SHARED.project.tree[nHandle].setParent(C.hInvalid)  # type: ignore
     assert projTree.revealNewTreeItem(nHandle) is False
 
-    # Method: undoLastMove
-    # ====================
-
-    # Nothing to move
-    assert projTree.undoLastMove() is False
-
-    projTree._lastMove["item"] = QTreeWidgetItem()
-    projTree._lastMove["parent"] = QTreeWidgetItem()
-    projTree._lastMove["index"] = 0
-    assert projTree.undoLastMove() is False
-
-    projTree._lastMove["item"] = projTree._treeMap[C.hTitlePage]
-    projTree._lastMove["parent"] = QTreeWidgetItem()
-    projTree._lastMove["index"] = 0
-    assert projTree.undoLastMove() is False
-
     # Slot: _treeDoubleClick
     # ======================