use perform_op() for deck drag&drop

ftl/core/browsing.ftl

@@ -151,3 +151,8 @@ browsing-changed-new-position =
         [one] Changed position of { $count } new card.
        *[other] Changed position of { $count } new cards.
     }
+browsing-reparented-decks =
+    { $count ->
+        [one] Renamed { $count } deck.
+       *[other] Renamed { $count } decks.
+    }

ftl/core/undo.ftl

@@ -19,5 +19,3 @@ undo-update-card = Update Card
 undo-update-deck = Update Deck
 undo-forget-card = Forget Card
 undo-set-flag = Set Flag
-# when dragging/dropping tags and decks in the sidebar
-undo-reparent = Change Parent

pylib/anki/decks.py

@@ -271,11 +271,13 @@ class DeckManager:
     # Drag/drop
     #############################################################
 
-    def drag_drop_decks(self, source_decks: List[DeckID], target_deck: DeckID) -> None:
+    def reparent(
-        """Rename one or more source decks that were dropped on `target_deck`.
+        self, deck_ids: Sequence[DeckID], new_parent: DeckID
-        If target_deck is 0, decks will be placed at the top level."""
+    ) -> OpChangesWithCount:
-        self.col._backend.drag_drop_decks(
+        """Rename one or more source decks that were dropped on `new_parent`.
-            source_deck_ids=source_decks, target_deck_id=target_deck
+        If new_parent is 0, decks will be placed at the top level."""
+        return self.col._backend.reparent_decks(
+            deck_ids=deck_ids, new_parent=new_parent
         )
 
     # legacy
@@ -286,7 +288,7 @@ class DeckManager:
             onto = 0
         else:
             onto = int(ontoDeckDid)
-        self.drag_drop_decks([int(draggedDeckDid)], onto)
+        self.reparent([int(draggedDeckDid)], onto)
 
     # Deck configurations
     #############################################################

qt/aqt/deck_ops.py

@@ -5,6 +5,7 @@ from __future__ import annotations
 
 from typing import Sequence
 
+from anki.decks import DeckID
 from anki.lang import TR
 from aqt import AnkiQt, QWidget
 from aqt.utils import tooltip, tr
@@ -14,7 +15,7 @@ def remove_decks(
     *,
     mw: AnkiQt,
     parent: QWidget,
-    deck_ids: Sequence[int],
+    deck_ids: Sequence[DeckID],
 ) -> None:
     mw.perform_op(
         lambda: mw.col.decks.remove(deck_ids),
@@ -22,3 +23,14 @@ def remove_decks(
             tr(TR.BROWSING_CARDS_DELETED, count=out.count), parent=parent
         ),
     )
+
+
+def reparent_decks(
+    *, mw: AnkiQt, parent: QWidget, deck_ids: Sequence[DeckID], new_parent: DeckID
+) -> None:
+    mw.perform_op(
+        lambda: mw.col.decks.reparent(deck_ids=deck_ids, new_parent=new_parent),
+        success=lambda out: tooltip(
+            tr(TR.BROWSING_REPARENTED_DECKS, count=out.count), parent=parent
+        ),
+    )

qt/aqt/deckbrowser.py

@@ -1,8 +1,8 @@
 # Copyright: Ankitects Pty Ltd and contributors
 # License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
 from __future__ import annotations
 
-from concurrent.futures import Future
 from copy import deepcopy
 from dataclasses import dataclass
 from typing import Any
@@ -13,7 +13,7 @@ from anki.decks import DeckTreeNode
 from anki.errors import DeckIsFilteredError
 from anki.utils import intTime
 from aqt import AnkiQt, gui_hooks
-from aqt.deck_ops import remove_decks
+from aqt.deck_ops import remove_decks, reparent_decks
 from aqt.qt import *
 from aqt.sound import av_player
 from aqt.toolbar import BottomBar
@@ -304,21 +304,7 @@ class DeckBrowser:
         self._renderPage(reuse=True)
 
     def _handle_drag_and_drop(self, source: int, target: int) -> None:
-        def process() -> None:
+        reparent_decks(mw=self.mw, parent=self.mw, deck_ids=[source], new_parent=target)
-            self.mw.col.decks.drag_drop_decks([source], target)
-
-        def on_done(fut: Future) -> None:
-            try:
-                fut.result()
-            except Exception as e:
-                showWarning(str(e))
-                return
-
-            self.mw.update_undo_actions()
-            gui_hooks.sidebar_should_refresh_decks()
-            self.show()
-
-        self.mw.taskman.with_progress(process, on_done)
 
     def _delete(self, did: int) -> None:
         remove_decks(mw=self.mw, parent=self.mw, deck_ids=[did])

qt/aqt/sidebar.py

@@ -3,7 +3,6 @@
 
 from __future__ import annotations
 
-from concurrent.futures import Future
 from enum import Enum, auto
 from typing import Dict, Iterable, List, Optional, Tuple, cast
 
@@ -16,7 +15,7 @@ from anki.tags import TagTreeNode
 from anki.types import assert_exhaustive
 from aqt import colors, gui_hooks
 from aqt.clayout import CardLayout
-from aqt.deck_ops import remove_decks
+from aqt.deck_ops import remove_decks, reparent_decks
 from aqt.models import Models
 from aqt.qt import *
 from aqt.tag_ops import remove_tags_for_all_notes, rename_tag, reparent_tags
@@ -604,30 +603,18 @@ class SidebarTreeView(QTreeView):
     def _handle_drag_drop_decks(
         self, sources: List[SidebarItem], target: SidebarItem
     ) -> bool:
-        source_ids = [
+        deck_ids = [
             source.id for source in sources if source.item_type == SidebarItemType.DECK
         ]
-        if not source_ids:
+        if not deck_ids:
             return False
 
-        def on_done(fut: Future) -> None:
+        new_parent = target.id
-            self.browser.model.endReset()
-            try:
-                fut.result()
-            except Exception as e:
-                showWarning(str(e))
-                return
-            self.refresh()
-            self.mw.deckBrowser.refresh()
-            self.mw.update_undo_actions()
 
-        def on_save() -> None:
+        reparent_decks(
-            self.browser.model.beginReset()
+            mw=self.mw, parent=self.browser, deck_ids=deck_ids, new_parent=new_parent
-            self.mw.taskman.with_progress(
+        )
-                lambda: self.col.decks.drag_drop_decks(source_ids, target.id), on_done
-            )
 
-        self.browser.editor.call_after_note_saved(on_save)
         return True
 
     def _handle_drag_drop_tags(

rslib/backend.proto

@@ -140,7 +140,7 @@ service DecksService {
   rpc GetDeckNames(GetDeckNamesIn) returns (DeckNames);
   rpc NewDeckLegacy(Bool) returns (Json);
   rpc RemoveDecks(DeckIDs) returns (OpChangesWithCount);
-  rpc DragDropDecks(DragDropDecksIn) returns (OpChanges);
+  rpc ReparentDecks(ReparentDecksIn) returns (OpChangesWithCount);
   rpc RenameDeck(RenameDeckIn) returns (OpChanges);
 }
 
@@ -1108,9 +1108,9 @@ message GetDeckNamesIn {
   bool include_filtered = 2;
 }
 
-message DragDropDecksIn {
+message ReparentDecksIn {
-  repeated int64 source_deck_ids = 1;
+  repeated int64 deck_ids = 1;
-  int64 target_deck_id = 2;
+  int64 new_parent = 2;
 }
 
 message NoteIsDuplicateOrEmptyOut {

rslib/src/backend/decks.rs

@@ -114,14 +114,14 @@ impl DecksService for Backend {
             .map(Into::into)
     }
 
-    fn drag_drop_decks(&self, input: pb::DragDropDecksIn) -> Result<pb::OpChanges> {
+    fn reparent_decks(&self, input: pb::ReparentDecksIn) -> Result<pb::OpChangesWithCount> {
-        let source_dids: Vec<_> = input.source_deck_ids.into_iter().map(Into::into).collect();
+        let deck_ids: Vec<_> = input.deck_ids.into_iter().map(Into::into).collect();
-        let target_did = if input.target_deck_id == 0 {
+        let new_parent = if input.new_parent == 0 {
             None
         } else {
-            Some(input.target_deck_id.into())
+            Some(input.new_parent.into())
         };
-        self.with_col(|col| col.drag_drop_decks(&source_dids, target_did))
+        self.with_col(|col| col.reparent_decks(&deck_ids, new_parent))
             .map(Into::into)
     }
 

rslib/src/decks/mod.rs

@@ -230,21 +230,21 @@ pub(crate) fn immediate_parent_name(machine_name: &str) -> Option<&str> {
 
 /// Determine name to rename a deck to, when `dragged` is dropped on `dropped`.
 /// `dropped` being unset represents a drop at the top or bottom of the deck list.
-/// The returned name should be used to rename `dragged`, and may be unchanged.
+/// The returned name should be used to rename `dragged`.
 /// Arguments are expected in 'machine' form with an \x1f separator.
-pub(crate) fn drag_drop_deck_name(dragged: &str, dropped: Option<&str>) -> String {
+pub(crate) fn reparented_name(dragged: &str, dropped: Option<&str>) -> Option<String> {
     let dragged_base = dragged.rsplit('\x1f').next().unwrap();
     if let Some(dropped) = dropped {
         if dropped.starts_with(dragged) {
             // foo onto foo::bar, or foo onto itself -> no-op
-            dragged.to_string()
+            None
         } else {
             // foo::bar onto baz -> baz::bar
-            format!("{}\x1f{}", dropped, dragged_base)
+            Some(format!("{}\x1f{}", dropped, dragged_base))
         }
     } else {
         // foo::bar onto top level -> bar
-        dragged_base.into()
+        Some(dragged_base.into())
     }
 }
 
@@ -618,59 +618,64 @@ impl Collection {
         self.update_single_deck_undoable(deck, original)
     }
 
-    pub fn drag_drop_decks(
+    pub fn reparent_decks(
         &mut self,
-        source_decks: &[DeckID],
+        deck_ids: &[DeckID],
-        target: Option<DeckID>,
+        new_parent: Option<DeckID>,
-    ) -> Result<OpOutput<()>> {
+    ) -> Result<OpOutput<usize>> {
-        let usn = self.usn()?;
+        self.transact(Op::ReparentDeck, |col| {
-        self.transact(Op::RenameDeck, |col| {
+            col.reparent_decks_inner(deck_ids, new_parent)
-            let target_deck;
+        })
-            let mut target_name = None;
+    }
-            if let Some(target) = target {
-                if let Some(target) = col.storage.get_deck(target)? {
-                    if target.is_filtered() {
-                        return Err(AnkiError::DeckIsFiltered);
-                    }
-                    target_deck = target;
-                    target_name = Some(target_deck.name.as_str());
-                }
-            }
 
-            for source in source_decks {
+    pub fn reparent_decks_inner(
-                if let Some(mut source) = col.storage.get_deck(*source)? {
+        &mut self,
-                    let new_name = drag_drop_deck_name(&source.name, target_name);
+        deck_ids: &[DeckID],
-                    if new_name == source.name {
+        new_parent: Option<DeckID>,
-                        continue;
+    ) -> Result<usize> {
-                    }
+        let usn = self.usn()?;
-                    let orig = source.clone();
+        let target_deck;
+        let mut target_name = None;
+        if let Some(target) = new_parent {
+            if let Some(target) = self.storage.get_deck(target)? {
+                if target.is_filtered() {
+                    return Err(AnkiError::DeckIsFiltered);
+                }
+                target_deck = target;
+                target_name = Some(target_deck.name.as_str());
+            }
+        }
+
+        let mut count = 0;
+        for deck in deck_ids {
+            if let Some(mut deck) = self.storage.get_deck(*deck)? {
+                if let Some(new_name) = reparented_name(&deck.name, target_name) {
+                    count += 1;
+                    let orig = deck.clone();
 
                     // this is basically update_deck_inner(), except:
                     // - we skip the normalization in prepare_for_update()
                     // - we skip the match_or_create_parents() step
+                    // - we skip the final create_missing_parents(), as we don't allow parent->child
+                    //   renames
 
-                    source.set_modified(usn);
+                    deck.set_modified(usn);
-                    source.name = new_name;
+                    deck.name = new_name;
-                    col.ensure_deck_name_unique(&mut source, usn)?;
+                    self.ensure_deck_name_unique(&mut deck, usn)?;
-                    col.rename_child_decks(&orig, &source.name, usn)?;
+                    self.rename_child_decks(&orig, &deck.name, usn)?;
-                    col.update_single_deck_undoable(&mut source, orig)?;
+                    self.update_single_deck_undoable(&mut deck, orig)?;
-
-                    // after updating, we need to ensure all grandparents exist, which may not be the case
-                    // in the parent->child case
-                    // FIXME: maybe we only need to do this once at the end of the loop?
-                    col.create_missing_parents(&source.name, usn)?;
                 }
             }
+        }
 
-            Ok(())
+        Ok(count)
-        })
     }
 }
 
 #[cfg(test)]
 mod test {
     use super::{human_deck_name_to_native, immediate_parent_name, normalize_native_name};
-    use crate::decks::drag_drop_deck_name;
+    use crate::decks::reparented_name;
     use crate::{
         collection::{open_test_collection, Collection},
         err::Result,
@@ -843,32 +848,31 @@ mod test {
         fn n(s: &str) -> String {
             s.replace(":", "\x1f")
         }
-        assert_eq!(drag_drop_deck_name("drag", Some("drop")), n("drop:drag"));
+        fn n_opt(s: &str) -> Option<String> {
-        assert_eq!(&drag_drop_deck_name("drag", None), "drag");
+            Some(n(s))
-        assert_eq!(&drag_drop_deck_name(&n("drag:child"), None), "child");
+        }
+
+        assert_eq!(reparented_name("drag", Some("drop")), n_opt("drop:drag"));
+        assert_eq!(reparented_name("drag", None), n_opt("drag"));
+        assert_eq!(reparented_name(&n("drag:child"), None), n_opt("child"));
         assert_eq!(
-            drag_drop_deck_name(&n("drag:child"), Some(&n("drop:deck"))),
+            reparented_name(&n("drag:child"), Some(&n("drop:deck"))),
-            n("drop:deck:child")
+            n_opt("drop:deck:child")
         );
         assert_eq!(
-            drag_drop_deck_name(&n("drag:child"), Some("drag")),
+            reparented_name(&n("drag:child"), Some("drag")),
-            n("drag:child")
+            n_opt("drag:child")
         );
         assert_eq!(
-            drag_drop_deck_name(&n("drag:child:grandchild"), Some("drag")),
+            reparented_name(&n("drag:child:grandchild"), Some("drag")),
-            n("drag:grandchild")
+            n_opt("drag:grandchild")
         );
-        // while the renaming code should be able to cope with renaming a parent to a child,
+        // drops to child not supported
-        // it's not often useful and can be difficult for the user to clean up if done accidentally,
-        // so it should be a no-op
         assert_eq!(
-            drag_drop_deck_name(&n("drag"), Some(&n("drag:child:grandchild"))),
+            reparented_name(&n("drag"), Some(&n("drag:child:grandchild"))),
-            n("drag")
+            None
         );
         // name doesn't change when deck dropped on itself
-        assert_eq!(
+        assert_eq!(reparented_name(&n("foo:bar"), Some(&n("foo:bar"))), None);
-            drag_drop_deck_name(&n("foo:bar"), Some(&n("foo:bar"))),
-            n("foo:bar")
-        );
     }
 }

rslib/src/ops.rs

@@ -15,6 +15,7 @@ pub enum Op {
     RemoveNote,
     RemoveTag,
     RenameDeck,
+    ReparentDeck,
     RenameTag,
     ReparentTag,
     ScheduleAsNew,
@@ -57,7 +58,8 @@ impl Op {
             Op::SortCards => TR::BrowsingReschedule,
             Op::RenameTag => TR::ActionsRenameTag,
             Op::RemoveTag => TR::ActionsRemoveTag,
-            Op::ReparentTag => TR::UndoReparent,
+            Op::ReparentTag => TR::ActionsRenameTag,
+            Op::ReparentDeck => TR::ActionsRenameDeck,
         };
 
         i18n.tr(key).to_string()