add ArchiveTableModel (QAbstractTableModel) spike#2517
Conversation
Self-contained table model for archive rows: time/size/duration formatting, trigger icons, sort-role keys, and an editable name column, with isolated unit tests. Not yet wired into ArchiveTab.
|
Quick question on sorting before I wire this in: custom |
|
m3nu small heads-up: I've got finals the next few days, back at full pace June 11.After that I will work through the Archive and Source tabs ( and any other tabs that need it) per your reviews here and on the others. #2514 and #2515 are reshaped and ready for re-review whenever you get a chance. No rush , thanks! |
There was a problem hiding this comment.
🔍 Review — ArchiveTableModel spike
The shape is right and this is the correct foundation for the wiring PR. I diffed the model against the live archive_tab.py:295-340 formatting and fidelity is good; the SortRole design is exactly what we settled on in #2515 and even fixes a latent bug (durations >24h render as '1 day, …' and sort wrong as text today — raw-seconds sort keys order them correctly). Tests (15, widget-free) are the bar I'd want for the other tables.
Six findings inline, two of which are important to resolve before the wiring is written against this API:
- 🔴 P1 (data-safety):
archive_at(row)is source-indexed; once the planned sort proxy is added it will target the wrong archive for rename/mount/delete. Use a custom data role instead. - 🟡 P2 (perf regression): the trigger icon is re-rendered from disk on every paint.
- 🟡 P3: the "Trigger" column header is silently dropped.
- 🔵 P4–P6: a duplicated constant, a missed alignment parity, and a typing nit.
Not approving since this is a draft — leaving as a comment. Shape-wise: 👍, address P1/P2 in the wiring PR and the recipe is solid.
| self._fixed_unit = None | ||
| self.endResetModel() | ||
|
|
||
| def archive_at(self, row: int) -> Optional[ArchiveModel]: |
There was a problem hiding this comment.
🔴 [P1 — data-safety] archive_at(row) takes a source row, but the wiring will feed it proxy rows.
This method is correct in isolation, but the PR's deferred recipe ("re-route the ~10 archiveTable.item(row, col) reads to model.archive_at(row)") will be wrong once ArchiveSortProxyModel is in place: selectionModel().selectedRows() returns proxy indices, and archive_at(proxy_row) indexes self._rows in source order. After any sort, that resolves to the wrong archive for rename (archive_tab.py:466,496,591), delete (701-702), and diff (757-758) — i.e. acting on the wrong backup.
The repo already establishes the rule that proxy indices must be mapped first — see diff_result.py:180 and extract.py:174 (index = self.sortproxy.mapToSource(index)).
Recommended: expose the archive through a custom data role rather than a raw-row accessor, e.g. ArchiveRole = Qt.ItemDataRole.UserRole + 1 returned from data(). Callers then use index.data(ArchiveRole) on the proxy index, and QSortFilterProxyModel auto-forwards through mapToSource — there's no raw row to get wrong, and it's symmetric with the existing SortRole. (If you keep archive_at, every one of the ~10 call sites must proxy.mapToSource() first, and rename it / document it as source-indexed.)
| return row.trigger or '' | ||
| return None | ||
|
|
||
| def _trigger_icon(self, row: ArchiveModel) -> Any: |
There was a problem hiding this comment.
🟡 [P2 — perf regression] Trigger icon is re-rendered from disk on every paint.
get_colored_icon (views/utils.py:10) does open() + read + QImage.fromData (SVG parse) + scaledToHeight(128) + wrap, with no caching. data() calls this on every DecorationRole request, which Qt fires per repaint/scroll for each visible trigger cell — and it renders a 128px image for a cell-sized icon. This is a regression from the legacy path, which built the icon once per populate and stored it in the QTableWidgetItem.
Recommended: cache the two icons (clock-o, user) on the model, keyed on dark-mode state, and rebuild them on QEvent.PaletteChange (the icons are theme-colored, so a plain permanent cache would go stale on a light/dark switch).
| trans_late('Form', 'Duration'), | ||
| trans_late('Form', 'Mount Point'), | ||
| trans_late('Form', 'Name'), | ||
| '', # trigger icon column has no header label |
There was a problem hiding this comment.
🟡 [P3 — behavior] The "Trigger" column header is silently dropped. The live .ui defines a Trigger header for column 5 (archive_tab.ui:175); _HEADERS[5] = '' removes it. That's a real UX change, not a no-op, and it diverges from the PR's "reuse existing catalog entries" claim. Either keep trans_late('Form', 'Trigger'), or make the icon-only-no-header decision explicit in the description.
| from vorta.views.utils import get_colored_icon | ||
|
|
||
| #: Decimal digits shown in the size column. | ||
| SIZE_DECIMAL_DIGITS = 1 |
There was a problem hiding this comment.
🔵 [P4 — maintainability] SIZE_DECIMAL_DIGITS is now defined here and at archive_tab.py:56. On wiring, have one own it and the other import it so they can't drift.
| return row.name # pre-fill the rename editor with the current name | ||
| if role == self.SortRole: | ||
| return self._sort_data(row, column) | ||
| if role == Qt.ItemDataRole.DecorationRole and column == self.COL_TRIGGER: |
There was a problem hiding this comment.
🔵 [P5 — parity] The legacy user-trigger item sets AlignRight (archive_tab.py:339); the model exposes no TextAlignmentRole. Negligible since the cell is icon-only, but worth a line on the wiring parity checklist so it's a conscious drop rather than an accidental one.
| '', # trigger icon column has no header label | ||
| ) | ||
|
|
||
| def __init__(self, parent: Optional[Any] = None): |
There was a problem hiding this comment.
🔵 [P6 — nit] parent: Optional[Any] → prefer Optional[QObject], matching the typed style in treemodel.py (same nit I left on #2515).
|
Thanks for the review m3nu , all six findings are addressed .
P4/P5 are handled in the wiring, as you suggested: it single-sources SIZE_DECIMAL_DIGITS from the model (P4), and the AlignRight drop is intentional , the Trigger cell is icon-only and IconDelegate centers it (P5). The wiring is implemented and green (Phase 4 part 1: QTableView + ArchiveTableModel fully wired into archive_tab.py. I'll open it as a follow-up PR once this spike lands. Phase 4 part 2 (peeling rename/diff/context-menu off the orchestrator) will be a separate slice. |
2 participants
A small spike of the Archive table as a
QAbstractTableModel, opened as a draft to confirm the shape before going deep on the view rewiring (as offered in the Phase 5 #2361 thread).Adds, and nothing else (the live Archive tab is untouched):
src/vorta/views/partials/archive_table_model.py,ArchiveTableModel, mirroring the existingEventLogTableModel. Owns the per-row presentation currently inline inArchiveTab.populate_from_profile: time/size/duration formatting, trigger icon + tooltip, raw sort-role keys, an editable name column, plusset_rows(...)andarchive_at(row).tests/unit/test_archive_table_model.py15 isolated unit tests (content + ordering, sort role, fixed/dynamic units, editable flag, setData/dataChanged, icons/tooltips,headers).Deliberately deferred to the wiring PR:
QTableWidget→QTableViewand callmodel.set_rows(...)frompopulate_from_profile.ArchiveSortProxyModel(QSortFilterProxyModel)overridinglessThan()(matchingtreemodel.py), reading the model'sSortRole.archiveTable.item(row, col)reads tomodel.archive_at(row).Conventions: no DB reads in the model (the
enable_fixed_unitssetting is injected by the view viaset_rows(..., use_fixed_units=...)); translation contexts reuse existing catalog entries (Formheaders,ArchiveTabtooltips).Related Issue
Phase 5 of the views refactor (#2361) the redirect to
QAbstractTableModel+views/partials/.Motivation and Context
Per the Phase 5 redirect, tabular views (Archive, Log) should become
QAbstractTableModel+QTableViewrather than ViewModels. This spike extracts the Archive table's presentation logic into a self-contained, idiomatic model so the shape can be reviewed before the largerarchive_tab.pymigration.How Has This Been Tested?
uv run nox -r -- tests/unit/test_archive_table_model.py. The model is exercised directly in isolation (no widget). No behavior change to the live app, since the model isn't wired in yet.Types of changes
Checklist:
I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.