Invalid sorting (#2709)

* Rollback if toggling state fails Previously, if the search triggered by a state toggle failed, the switch and the model would move to the new state, while the table would remain in the previous state. * Fix reversed sort orders of FSRS columns * Add sep. default sort orders for notes and cards * Add test for consistent default sort orders * Add launch config for debugging in VSC * Extend launch config for macOS and Linux
2023-10-07 08:36:15 +02:00 · 2023-10-07 08:36:15 +02:00 · 23f29a6ecc
commit 23f29a6ecc
parent 9db35fee6e
10 changed files with 160 additions and 41 deletions
--- a/.vscode.dist/launch.json
+++ b/.vscode.dist/launch.json
@ -0,0 +1,27 @@
+{
+    // Use IntelliSense to learn about possible attributes.
+    // Hover to view descriptions of existing attributes.
+    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+    "version": "0.2.0",
+    "configurations": [
+        {
+            "name": "Run",
+            "type": "python",
+            "request": "launch",
+            "program": "tools/run.py",
+            "console": "integratedTerminal",
+            "cwd": "${workspaceFolder}",
+            "python": "${workspaceFolder}/out/pyenv/bin/python",
+            "windows": {
+                "python": "${workspaceFolder}/out/pyenv/scripts/python.exe"
+            },
+            "env": {
+                "PYTHONWARNINGS": "default",
+                "PYTHONPYCACHEPREFIX": "out/pycache",
+                "ANKIDEV": "1"
+            },
+            "justMyCode": true,
+            "preLaunchTask": "ninja"
+        }
+    ]
+}
--- a/.vscode.dist/tasks.json
+++ b/.vscode.dist/tasks.json
@ -0,0 +1,22 @@
+{
+    "version": "2.0.0",
+    "tasks": [
+        {
+            "label": "ninja",
+            "command": "ninja",
+            "args": [
+                "pylib",
+                "qt"
+            ],
+            "windows": {
+                "command": "bash",
+                "args": [
+                    "ninja",
+                    "pylib",
+                    "qt",
+                    "extract:win_amd64_audio"
+                ]
+            }
+        }
+    ]
+}
--- a/proto/anki/search.proto
+++ b/proto/anki/search.proto
@ -161,7 +161,8 @@ message BrowserColumns {
    string cards_mode_label = 2;
    string notes_mode_label = 3;
    // The default sort order
-    Sorting sorting = 4;
+    Sorting sorting_cards = 4;
+    Sorting sorting_notes = 9;
    bool uses_cell_font = 5;
    Alignment alignment = 6;
    string cards_mode_tooltip = 7;
--- a/pylib/anki/collection.py
+++ b/pylib/anki/collection.py
@ -719,8 +719,9 @@ class Collection(DeprecatedNamesMixin):

        If order is a BrowserColumns.Column that supports sorting, sort using that
        column. All available columns are available through col.all_browser_columns()
-        or browser.table._model.columns and support sorting unless column.sorting
-        is set to BrowserColumns.SORTING_NONE.
+        or browser.table._model.columns and support sorting cards unless column.sorting_cards
+        is set to BrowserColumns.SORTING_NONE, .SORTING_NOTES_ASCENDING, or
+        .SORTING_NOTES_DESCENDING.

        The reverse argument only applies when a BrowserColumns.Column is provided;
        otherwise the collection config defines whether reverse is set or not.
@ -762,13 +763,14 @@ class Collection(DeprecatedNamesMixin):
            order = self.get_browser_column(self.get_config(sort_key))
            reverse_key = BrowserConfig.sort_backwards_key(finding_notes)
            reverse = self.get_config(reverse_key)
-        if isinstance(order, BrowserColumns.Column):
-            if order.sorting != BrowserColumns.SORTING_NONE:
-                return search_pb2.SortOrder(
-                    builtin=search_pb2.SortOrder.Builtin(
-                        column=order.key, reverse=reverse
-                    )
-                )
+        if (
+            isinstance(order, BrowserColumns.Column)
+            and (order.sorting_notes if finding_notes else order.sorting_cards)
+            is not BrowserColumns.SORTING_NONE
+        ):
+            return search_pb2.SortOrder(
+                builtin=search_pb2.SortOrder.Builtin(column=order.key, reverse=reverse)
+            )

        # eg, user is ordering on an add-on field with the add-on not installed
        print(f"{order} is not a valid sort order.")
--- a/qt/aqt/browser/browser.py
+++ b/qt/aqt/browser/browser.py
@ -25,6 +25,7 @@ from anki.tags import MARKED_TAG
 from anki.utils import is_mac
 from aqt import AnkiQt, gui_hooks
 from aqt.editor import Editor
+from aqt.errors import show_exception
 from aqt.exporting import ExportDialog as LegacyExportDialog
 from aqt.import_export.exporting import ExportDialog
 from aqt.operations.card import set_card_deck, set_card_flag
@ -514,7 +515,7 @@ class Browser(QMainWindow):
    def setup_table(self) -> None:
        self.table = Table(self)
        self.table.set_view(self.form.tableView)
-        switch = Switch(12, tr.browsing_cards(), tr.browsing_notes())
+        self._switch = switch = Switch(12, tr.browsing_cards(), tr.browsing_notes())
        switch.setChecked(self.table.is_notes_mode())
        switch.setToolTip(tr.browsing_toggle_showing_cards_notes())
        qconnect(self.form.action_toggle_mode.triggered, switch.toggle)
@ -610,8 +611,16 @@ class Browser(QMainWindow):
    @ensure_editor_saved
    def on_table_state_changed(self, checked: bool) -> None:
        self.mw.progress.start()
-        self.table.toggle_state(checked, self._lastSearchTxt)
-        self.mw.progress.finish()
+        try:
+            self.table.toggle_state(checked, self._lastSearchTxt)
+        except Exception as err:
+            self.mw.progress.finish()
+            self._switch.blockSignals(True)
+            self._switch.toggle()
+            self._switch.blockSignals(False)
+            show_exception(parent=self, exception=err)
+        else:
+            self.mw.progress.finish()

    # Sidebar
    ######################################################################
--- a/qt/aqt/browser/table/model.py
+++ b/qt/aqt/browser/table/model.py
@ -238,9 +238,16 @@ class DataModel(QAbstractTableModel):
    ######################################################################

    def toggle_state(self, context: SearchContext) -> ItemState:
-        self.beginResetModel()
+        self.begin_reset()
        self._state = self._state.toggle_state()
-        self.search(context)
+        try:
+            self._search_inner(context)
+        except:
+            # rollback to prevent inconsistent state
+            self._state = self._state.toggle_state()
+            raise
+        finally:
+            self.end_reset()
        return self._state

    # Rows
@ -248,24 +255,27 @@ class DataModel(QAbstractTableModel):
    def search(self, context: SearchContext) -> None:
        self.begin_reset()
        try:
-            if context.order is True:
-                try:
-                    context.order = self.columns[self._state.sort_column]
-                except KeyError:
-                    # invalid sort column in config
-                    context.order = self.columns["noteCrt"]
-                context.reverse = self._state.sort_backwards
-            gui_hooks.browser_will_search(context)
-            if context.ids is None:
-                context.ids = self._state.find_items(
-                    context.search, context.order, context.reverse
-                )
-            gui_hooks.browser_did_search(context)
-            self._items = context.ids
-            self._rows = {}
+            self._search_inner(context)
        finally:
            self.end_reset()

+    def _search_inner(self, context: SearchContext) -> None:
+        if context.order is True:
+            try:
+                context.order = self.columns[self._state.sort_column]
+            except KeyError:
+                # invalid sort column in config
+                context.order = self.columns["noteCrt"]
+            context.reverse = self._state.sort_backwards
+        gui_hooks.browser_will_search(context)
+        if context.ids is None:
+            context.ids = self._state.find_items(
+                context.search, context.order, context.reverse
+            )
+        gui_hooks.browser_did_search(context)
+        self._items = context.ids
+        self._rows = {}
+
    def reverse(self) -> None:
        self.beginResetModel()
        self._items = list(reversed(self._items))
@ -362,7 +372,8 @@ def addon_column_fillin(key: str) -> Column:
        key=key,
        cards_mode_label=f"{tr.browsing_addon()} ({key})",
        notes_mode_label=f"{tr.browsing_addon()} ({key})",
-        sorting=Columns.SORTING_NONE,
+        sorting_cards=Columns.SORTING_NONE,
+        sorting_notes=Columns.SORTING_NONE,
        uses_cell_font=False,
        alignment=Columns.ALIGNMENT_CENTER,
    )
--- a/qt/aqt/browser/table/table.py
+++ b/qt/aqt/browser/table/table.py
@ -488,14 +488,15 @@ class Table:

    def _on_sort_column_changed(self, section: int, order: Qt.SortOrder) -> None:
        column = self._model.column_at_section(section)
-        if column.sorting == Columns.SORTING_NONE:
+        sorting = column.sorting_notes if self.is_notes_mode() else column.sorting_cards
+        if sorting is Columns.SORTING_NONE:
            showInfo(tr.browsing_sorting_on_this_column_is_not())
            self._set_sort_indicator()
            return
        if self._state.sort_column != column.key:
            self._state.sort_column = column.key
            # numeric fields default to descending
-            if column.sorting == Columns.SORTING_DESCENDING:
+            if sorting is Columns.SORTING_DESCENDING:
                order = Qt.SortOrder.DescendingOrder
            self._state.sort_backwards = order == Qt.SortOrder.DescendingOrder
            self.browser.search()
--- a/rslib/src/browser_table.rs
+++ b/rslib/src/browser_table.rs
@ -204,7 +204,15 @@ impl Column {
        .into()
    }

-    pub fn default_order(self) -> anki_proto::search::browser_columns::Sorting {
+    pub fn default_cards_order(self) -> anki_proto::search::browser_columns::Sorting {
+        self.default_order(false)
+    }
+
+    pub fn default_notes_order(self) -> anki_proto::search::browser_columns::Sorting {
+        self.default_order(true)
+    }
+
+    fn default_order(self, notes: bool) -> anki_proto::search::browser_columns::Sorting {
        use anki_proto::search::browser_columns::Sorting;
        match self {
            Column::Question | Column::Answer | Column::Custom => Sorting::None,
@ -219,10 +227,14 @@ impl Column {
            | Column::Interval
            | Column::NoteCreation
            | Column::NoteMod
-            | Column::Stability
-            | Column::Difficulty
-            | Column::Retrievability
            | Column::Reps => Sorting::Descending,
+            Column::Stability | Column::Difficulty | Column::Retrievability => {
+                if notes {
+                    Sorting::None
+                } else {
+                    Sorting::Descending
+                }
+            }
        }
    }

--- a/rslib/src/search/mod.rs
+++ b/rslib/src/search/mod.rs
@ -374,10 +374,10 @@ fn card_order_from_sort_column(column: Column, timing: SchedTimingToday) -> Cow<
        Column::SortField => "n.sfld collate nocase asc, c.ord asc".into(),
        Column::Tags => "n.tags asc".into(),
        Column::Answer | Column::Custom | Column::Question => "".into(),
-        Column::Stability => "extract_fsrs_variable(c.data, 's') desc".into(),
-        Column::Difficulty => "extract_fsrs_variable(c.data, 'd') desc".into(),
+        Column::Stability => "extract_fsrs_variable(c.data, 's') asc".into(),
+        Column::Difficulty => "extract_fsrs_variable(c.data, 'd') asc".into(),
        Column::Retrievability => format!(
-            "extract_fsrs_retrievability(c.data, c.due, c.ivl, {})",
+            "extract_fsrs_retrievability(c.data, c.due, c.ivl, {}) asc",
            timing.days_elapsed
        )
        .into(),
@ -434,3 +434,36 @@ fn prepare_sort(col: &mut Collection, column: Column, item_type: ReturnItemType)

    Ok(())
 }
+
+#[cfg(test)]
+mod test {
+    use anki_proto::search::browser_columns::Sorting;
+    use strum::IntoEnumIterator;
+
+    use super::*;
+
+    impl SchedTimingToday {
+        pub(crate) fn zero() -> Self {
+            SchedTimingToday {
+                now: TimestampSecs(0),
+                days_elapsed: 0,
+                next_day_at: TimestampSecs(0),
+            }
+        }
+    }
+
+    #[test]
+    fn column_default_sort_order_should_match_order_by_clause() {
+        let timing = SchedTimingToday::zero();
+        for column in Column::iter() {
+            assert_eq!(
+                card_order_from_sort_column(column, timing).is_empty(),
+                matches!(column.default_cards_order(), Sorting::None)
+            );
+            assert_eq!(
+                note_order_from_sort_column(column).is_empty(),
+                matches!(column.default_notes_order(), Sorting::None)
+            );
+        }
+    }
+}
--- a/rslib/src/search/service/browser_table.rs
+++ b/rslib/src/search/service/browser_table.rs
@ -13,7 +13,8 @@ impl browser_table::Column {
            key: self.to_string(),
            cards_mode_label: self.cards_mode_label(i18n),
            notes_mode_label: self.notes_mode_label(i18n),
-            sorting: self.default_order() as i32,
+            sorting_cards: self.default_cards_order() as i32,
+            sorting_notes: self.default_notes_order() as i32,
            uses_cell_font: self.uses_cell_font(),
            alignment: self.alignment() as i32,
            cards_mode_tooltip: self.cards_mode_tooltip(i18n),