Merge pull request #1392 from RumovZ/missing-row-handling

Handle missing rows consistently and speed up selections

qt/aqt/browser/browser.py

@@ -9,13 +9,7 @@ import aqt
 import aqt.forms
 from anki._legacy import deprecated
 from anki.cards import Card, CardId
-from anki.collection import (
+from anki.collection import Collection, Config, OpChanges, SearchNode
-    Collection,
-    Config,
-    OpChanges,
-    OpChangesWithCount,
-    SearchNode,
-)
 from anki.consts import *
 from anki.errors import NotFoundError
 from anki.lang import without_unicode_isolation
@@ -60,7 +54,6 @@ from aqt.utils import (
     saveState,
     showWarning,
     skip_if_selection_is_empty,
-    tooltip,
     tr,
 )
 
@@ -417,9 +410,7 @@ class Browser(QMainWindow):
         gui_hooks.editor_did_init.remove(add_preview_button)
 
     @ensure_editor_saved
-    def onRowChanged(
+    def on_row_changed(self) -> None:
-        self, _current: Optional[QItemSelection], _previous: Optional[QItemSelection]
-    ) -> None:
         """Update current note and hide/show editor."""
         if self._closeEventHasCleanedUp:
             return
@@ -628,17 +619,8 @@ class Browser(QMainWindow):
         if focus != self.form.tableView:
             return
 
-        nids = self.table.get_selected_note_ids()
+        nids = self.table.to_row_of_unselected_note()
-
+        remove_notes(parent=self, note_ids=nids).run_in_background()
-        def after_remove(changes: OpChangesWithCount) -> None:
-            tooltip(tr.browsing_cards_deleted(count=changes.count))
-            # select the next card if there is one
-            self.focusTo = self.editor.currentField
-            self.table.to_next_row()
-
-        remove_notes(parent=self, note_ids=nids).success(
-            after_remove
-        ).run_in_background()
 
     # legacy
 

qt/aqt/browser/table/model.py

@@ -4,7 +4,7 @@
 from __future__ import annotations
 
 import time
-from typing import Any, Dict, List, Optional, Sequence, Union, cast
+from typing import Any, Callable, Dict, List, Optional, Sequence, Union, cast
 
 import aqt
 from anki.cards import Card, CardId
@@ -32,7 +32,13 @@ class DataModel(QAbstractTableModel):
     _stale_cutoff -- A threshold to decide whether a cached row has gone stale.
     """
 
-    def __init__(self, col: Collection, state: ItemState) -> None:
+    def __init__(
+        self,
+        col: Collection,
+        state: ItemState,
+        row_state_will_change_callback: Callable,
+        row_state_changed_callback: Callable,
+    ) -> None:
         QAbstractTableModel.__init__(self)
         self.col: Collection = col
         self.columns: Dict[str, Column] = dict(
@@ -44,6 +50,8 @@ class DataModel(QAbstractTableModel):
         self._rows: Dict[int, CellRow] = {}
         self._block_updates = False
         self._stale_cutoff = 0.0
+        self._on_row_state_will_change = row_state_will_change_callback
+        self._on_row_state_changed = row_state_changed_callback
         self._want_tooltips = aqt.mw.pm.show_browser_table_tooltips()
 
     # Row Object Interface
@@ -59,15 +67,34 @@ class DataModel(QAbstractTableModel):
         if row := self._rows.get(item):
             if not self._block_updates and row.is_stale(self._stale_cutoff):
                 # need to refresh
-                self._rows[item] = self._fetch_row_from_backend(item)
+                return self._fetch_row_and_update_cache(index, item, row)
-                return self._rows[item]
             # return row, even if it's stale
             return row
         if self._block_updates:
             # blank row until we unblock
             return CellRow.placeholder(self.len_columns())
         # missing row, need to build
-        self._rows[item] = self._fetch_row_from_backend(item)
+        return self._fetch_row_and_update_cache(index, item, None)
+
+    def _fetch_row_and_update_cache(
+        self, index: QModelIndex, item: ItemId, old_row: Optional[CellRow]
+    ) -> CellRow:
+        """Fetch a row from the backend, add it to the cache and return it.
+        Then fire callbacks if the row is being deleted or restored.
+        """
+        new_row = self._fetch_row_from_backend(item)
+        # row state has changed if existence of cached and fetched counterparts differ
+        # if the row was previously uncached, it is assumed to have existed
+        state_change = (
+            new_row.is_deleted
+            if old_row is None
+            else old_row.is_deleted != new_row.is_deleted
+        )
+        if state_change:
+            self._on_row_state_will_change(index, not new_row.is_deleted)
+        self._rows[item] = new_row
+        if state_change:
+            self._on_row_state_changed(index, not new_row.is_deleted)
         return self._rows[item]
 
     def _fetch_row_from_backend(self, item: ItemId) -> CellRow:
@@ -92,6 +119,10 @@ class DataModel(QAbstractTableModel):
         )
         return row
 
+    def get_cached_row(self, index: QModelIndex) -> Optional[CellRow]:
+        """Get row if it is cached, regardless of staleness."""
+        return self._rows.get(self.get_item(index))
+
     # Reset
 
     def mark_cache_stale(self) -> None:
@@ -153,6 +184,11 @@ class DataModel(QAbstractTableModel):
     def get_note_ids(self, indices: List[QModelIndex]) -> Sequence[NoteId]:
         return self._state.get_note_ids(self.get_items(indices))
 
+    def get_note_id(self, index: QModelIndex) -> Optional[NoteId]:
+        if nid_list := self._state.get_note_ids([self.get_item(index)]):
+            return nid_list[0]
+        return None
+
     # Get row numbers from items
 
     def get_item_row(self, item: ItemId) -> Optional[int]:
@@ -175,6 +211,8 @@ class DataModel(QAbstractTableModel):
 
     def get_card(self, index: QModelIndex) -> Optional[Card]:
         """Try to return the indicated, possibly deleted card."""
+        if not index.isValid():
+            return None
         try:
             return self._state.get_card(self.get_item(index))
         except NotFoundError:
@@ -182,6 +220,8 @@ class DataModel(QAbstractTableModel):
 
     def get_note(self, index: QModelIndex) -> Optional[Note]:
         """Try to return the indicated, possibly deleted note."""
+        if not index.isValid():
+            return None
         try:
             return self._state.get_note(self.get_item(index))
         except NotFoundError:
@@ -295,8 +335,10 @@ class DataModel(QAbstractTableModel):
         return None
 
     def flags(self, index: QModelIndex) -> Qt.ItemFlags:
-        if self.get_row(index).is_deleted:
+        # shortcut for large selections (Ctrl+A) to avoid fetching large numbers of rows at once
-            return Qt.ItemFlags(Qt.NoItemFlags)
+        if row := self.get_cached_row(index):
+            if row.is_deleted:
+                return Qt.ItemFlags(Qt.NoItemFlags)
         return cast(Qt.ItemFlags, Qt.ItemIsEnabled | Qt.ItemIsSelectable)
 
 

qt/aqt/browser/table/table.py

@@ -39,8 +39,17 @@ class Table:
             if self.col.get_config_bool(Config.Bool.BROWSER_TABLE_SHOW_NOTES_MODE)
             else CardState(self.col)
         )
-        self._model = DataModel(self.col, self._state)
+        self._model = DataModel(
+            self.col,
+            self._state,
+            self._on_row_state_will_change,
+            self._on_row_state_changed,
+        )
         self._view: Optional[QTableView] = None
+        # cached for performance
+        self._len_selection = 0
+        self._selected_rows: Optional[List[QModelIndex]] = None
+        # temporarily set for selection preservation
         self._current_item: Optional[ItemId] = None
         self._selected_items: Sequence[ItemId] = []
 
@@ -60,8 +69,11 @@ class Table:
     def len(self) -> int:
         return self._model.len_rows()
 
-    def len_selection(self) -> int:
+    def len_selection(self, refresh: bool = False) -> int:
-        return len(self._view.selectionModel().selectedRows())
+        # `len(self._view.selectionModel().selectedRows())` is slow for large
+        # selections, because Qt queries flags() for every selected cell, so we
+        # calculate the number of selected rows ourselves
+        return self._len_selection
 
     def has_current(self) -> bool:
         return self._view.selectionModel().currentIndex().isValid()
@@ -78,13 +90,9 @@ class Table:
     # Get objects
 
     def get_current_card(self) -> Optional[Card]:
-        if not self.has_current():
-            return None
         return self._model.get_card(self._current())
 
     def get_current_note(self) -> Optional[Note]:
-        if not self.has_current():
-            return None
         return self._model.get_note(self._current())
 
     def get_single_selected_card(self) -> Optional[Card]:
@@ -111,6 +119,8 @@ class Table:
         self._view.selectAll()
 
     def clear_selection(self) -> None:
+        self._len_selection = 0
+        self._selected_rows = None
         self._view.selectionModel().clear()
 
     def invert_selection(self) -> None:
@@ -126,10 +136,12 @@ class Table:
 
     def select_single_card(self, card_id: CardId) -> None:
         """Try to set the selection to the item corresponding to the given card."""
-        self.clear_selection()
+        self._reset_selection()
         if (row := self._model.get_card_row(card_id)) is not None:
             self._view.selectRow(row)
             self._scroll_to_row(row, scroll_even_if_visible=True)
+        else:
+            self.browser.on_row_changed()
 
     # Reset
 
@@ -201,6 +213,43 @@ class Table:
     def to_last_row(self) -> None:
         self._move_current_to_row(self._model.len_rows() - 1)
 
+    def to_row_of_unselected_note(self) -> Sequence[NoteId]:
+        """Select and set focus to a row whose note is not selected, trying
+        the rows below the bottomost, then above the topmost selected row.
+        If that's not possible, clear selection.
+        Return previously selected note ids.
+        """
+        nids = self.get_selected_note_ids()
+
+        bottom = max(r.row() for r in self._selected()) + 1
+        for row in range(bottom, self.len()):
+            index = self._model.index(row, 0)
+            if self._model.get_row(index).is_deleted:
+                continue
+            if self._model.get_note_id(index) in nids:
+                continue
+            self._move_current_to_row(row)
+            return nids
+
+        top = min(r.row() for r in self._selected()) - 1
+        for row in range(top, -1, -1):
+            index = self._model.index(row, 0)
+            if self._model.get_row(index).is_deleted:
+                continue
+            if self._model.get_note_id(index) in nids:
+                continue
+            self._move_current_to_row(row)
+            return nids
+
+        self._reset_selection()
+        self.browser.on_row_changed()
+        return nids
+
+    def clear_current(self) -> None:
+        self._view.selectionModel().setCurrentIndex(
+            QModelIndex(), QItemSelectionModel.NoUpdate
+        )
+
     # Private methods
     ######################################################################
 
@@ -210,7 +259,9 @@ class Table:
         return self._view.selectionModel().currentIndex()
 
     def _selected(self) -> List[QModelIndex]:
-        return self._view.selectionModel().selectedRows()
+        if self._selected_rows is None:
+            self._selected_rows = self._view.selectionModel().selectedRows()
+        return self._selected_rows
 
     def _set_current(self, row: int, column: int = 0) -> None:
         index = self._model.index(
@@ -218,6 +269,15 @@ class Table:
         )
         self._view.selectionModel().setCurrentIndex(index, QItemSelectionModel.NoUpdate)
 
+    def _reset_selection(self) -> None:
+        """Remove selection and focus without emitting signals.
+        If no selection change is triggerd afterwards, `browser.on_row_changed()`
+        must be called.
+        """
+        self._view.selectionModel().reset()
+        self._len_selection = 0
+        self._selected_rows = None
+
     def _select_rows(self, rows: List[int]) -> None:
         selection = QItemSelection()
         for row in rows:
@@ -269,7 +329,7 @@ class Table:
         self._view.selectionModel()
         self._view.setItemDelegate(StatusDelegate(self.browser, self._model))
         qconnect(
-            self._view.selectionModel().selectionChanged, self.browser.onRowChanged
+            self._view.selectionModel().selectionChanged, self._on_selection_changed
         )
         self._view.setWordWrap(False)
         self._view.setHorizontalScrollMode(QAbstractItemView.ScrollPerPixel)
@@ -315,6 +375,59 @@ class Table:
 
     # Slots
 
+    def _on_selection_changed(
+        self, selected: QItemSelection, deselected: QItemSelection
+    ) -> None:
+        # `selection.indexes()` calls `flags()` for all the selection's indexes,
+        # whereas `selectedRows()` calls it for the indexes of the resulting selection.
+        # Both may be slow, so we try to optimise.
+        if KeyboardModifiersPressed().shift or KeyboardModifiersPressed().control:
+            # Current selection is modified. The number of added/removed rows is
+            # usually smaller than the number of rows in the resulting selection.
+            self._len_selection += (
+                len(selected.indexes()) - len(deselected.indexes())
+            ) // self._model.len_columns()
+        else:
+            # New selection is created. Usually a single row or none at all.
+            self._len_selection = len(self._view.selectionModel().selectedRows())
+        self._selected_rows = None
+        self.browser.on_row_changed()
+
+    def _on_row_state_will_change(self, index: QModelIndex, was_restored: bool) -> None:
+        if not was_restored:
+            row_changed = False
+            if self._view.selectionModel().isSelected(index):
+                # calculate directly, because 'self.len_selection()' is slow and
+                # this method will be called a lot if a lot of rows were deleted
+                self._len_selection -= 1
+                row_changed = True
+                self._selected_rows = None
+            if index.row() == self._current().row():
+                # avoid focus on deleted (disabled) rows
+                self.clear_current()
+                row_changed = True
+            if row_changed:
+                self.browser.on_row_changed()
+
+    def _on_row_state_changed(self, index: QModelIndex, was_restored: bool) -> None:
+        if was_restored:
+            if self._view.selectionModel().isSelected(index):
+                self._len_selection += 1
+                self._selected_rows = None
+            if not self._current().isValid() and self.len_selection() == 0:
+                # restore focus for convenience
+                self._select_rows([index.row()])
+                self._set_current(index.row())
+                self._scroll_to_row(index.row())
+                # row change has been triggered
+                return
+            self.browser.on_row_changed()
+        # Workaround for a bug where the flags for the first column don't update
+        # automatically (due to the shortcut in 'model.flags()')
+        top_left = self._model.index(index.row(), 0)
+        bottom_right = self._model.index(index.row(), self._model.len_columns() - 1)
+        self._model.dataChanged.emit(top_left, bottom_right)  # type: ignore
+
     def _on_context_menu(self, _point: QPoint) -> None:
         menu = QMenu()
         if self.is_notes_mode():
@@ -400,7 +513,7 @@ class Table:
         """Restore the saved selection and current element as far as possible and scroll to the
         new current element. Clear the saved selection.
         """
-        self.clear_selection()
+        self._reset_selection()
         if not self._model.is_empty():
             rows, current = new_selected_and_current()
             rows = self._qualify_selected_rows(rows, current)
@@ -410,7 +523,7 @@ class Table:
             self._scroll_to_row(current)
         if self.len_selection() == 0:
             # no row change will fire
-            self.browser.onRowChanged(QItemSelection(), QItemSelection())
+            self.browser.on_row_changed()
         self._selected_items = []
         self._current_item = None