Skip to content

Refactor LogPage to use a QAbstractTableModel#2515

Open
ebuzerdrmz44 wants to merge 4 commits into
borgbase:masterfrom
ebuzerdrmz44:refactor/log-page-viewmodel
Open

Refactor LogPage to use a QAbstractTableModel#2515
ebuzerdrmz44 wants to merge 4 commits into
borgbase:masterfrom
ebuzerdrmz44:refactor/log-page-viewmodel

Conversation

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor

Description

Refactor LogPage to use the MVVM pattern by extracting the event log query into a new LogPageViewModel class. The view now delegates data fetching to the ViewModel instead of querying EventLogModel directly.

Related Issue

Part of Phase 5 for #2361

Motivation and Context

Continues the ViewModel migration established in the MiscTab PR. Extracts database queries from the view layer so LogPage focuses purely on UI concerns.

How Has This Been Tested?

Added unit tests in tests/unit/test_log_page_viewmodel.py.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@m3nu m3nu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this continues the Phase 5 direction and the tests here are notably clean: you assert content and ordering rather than just a count, and there's no leftover state, so the suite is a real safety net. No persistence-pattern concern on this one either, since LogPage is read-only.

Two things to resolve before we lock the pattern in, and they're the same open questions as the MiscTab PR (#2514) — so let's settle them once, across both, rather than per-PR:

1. The abstraction is very thin. get_event_logs() moves a single query verbatim, and the view still computes profile.id and hands it in. As-is it's a one-method wrapper around one query. That's fine if the ViewModel is going to grow into something that owns real presentation logic — but right now it's hard to see what it buys us over calling the model directly. Can you sketch what this VM will own once LogPage's logic (filtering, formatting, the richtext prep currently in the view) moves into it? That's what would justify the layer.

2. Naming/semantics — same decision as #2514. This is a stateless repository/service, not an MVVM ViewModel (no exposed state or commands). Whatever we choose becomes the convention for every tab, so I'd rather decide it deliberately than by merge order. I'll make a call on the #2361 thread about what "ViewModel" should mean in Vorta and how it composes with BaseTab; let's hold both this and #2514 until then so they land consistently.

Clean work on the tests regardless — appreciate it.

Replaces the short-lived LogPageViewModel introduced earlier in this branch.
@ebuzerdrmz44 ebuzerdrmz44 force-pushed the refactor/log-page-viewmodel branch from bf72331 to 1bac151 Compare May 31, 2026 11:55
@ebuzerdrmz44

ebuzerdrmz44 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Hey m3nu ,the small spike ready : LogPage refactored as QTableView + QAbstractTableModel. Query inlined per the rule of thumb.

Two decisions that'll set the pattern for the bigger tables (Archive + Source):

  1. .ui vs runtime: I declared QTableView in log_page.ui (dropped the blocks, since the model owns the headers). Same approach for Archive/Source, or prefer a runtime swap?
  2. Sorting: LogPage gets away with a plain QSortFilterProxyModel because all its columns are lex-sortable strings. However, Archive and Source have non-string data like size and file counts sizes (currently sort via custom QTableWidgetItem subclasses (SizeItem/FilesCount) ). For those, do you want a custom sort role (model exposes raw values via Qt.UserRole, proxy sorts on that) or lessThan on the model? I'll go with your call when I hit Archive so it's consistent.

These two unblock the conversion recipe.I will raise the specific details for Archive and Source (such as multi-role cells, mapping selection to the model, live cell updates, and sort persistence) in their respective PRs but if you have some suggestions, I'd like to hear them.

@ebuzerdrmz44 ebuzerdrmz44 requested a review from m3nu June 2, 2026 21:32
@ebuzerdrmz44 ebuzerdrmz44 changed the title Refactor LogPage to use MVVM pattern Refactor LogPage to use a QAbstractTableModel Jun 6, 2026

@m3nu m3nu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Review — LogPage → QAbstractTableModel

This correctly delivers the Phase 5 redirect (#2361): the table view becomes QTableView + a QAbstractTableModel in views/partials/, with the query inlined in the view per the rule of thumb. I traced the usual QTableWidgetQTableView failure modes and they're all handled:

  • i18n context preserved — the .ui top-level <class> is LogPage, so the old column strings were extracted under context LogPage. The model marks them with trans_late('LogPage', …) and resolves at runtime via translate('LogPage', …) — same context + source, so existing catalog entries are reused, nothing orphaned.
  • Sorting still wired.ui keeps sortingEnabled=true; the old manual setSortingEnabled(False)/restore is correctly replaced by beginResetModel/endResetModel.
  • Read-only — default flags() + view NoEditTriggers.
  • No stale call sites — all setItem/setRowCount/LogTableColumn removed; both test call-sites updated to .model().rowCount().

Two low-severity notes inline (non-blocking).

Answers to your two pattern questions (these set the recipe for Archive #2517 + Source)

  1. .ui vs runtime swap → declare QTableView in the .ui, as you did. Less boilerplate, keeps layout declarative; IconDelegate/setSortingEnabled on Archive carry over fine.
  2. Sorting for non-string columns (size/filecount) → use a custom sort role, not a lessThan proxy subclass. treemodel.py's FileTreeSortProxyModel.lessThan exists only because it needs cross-cutting folders-on-top + tree logic — flat tables don't. Have the model expose raw comparable values under a dedicated SortRole and call proxy.setSortRole(SortRole); the default lessThan then compares numerically. More unit-testable, and your #2517 spike already implements exactly this. Reserve a lessThan subclass only if a table later needs treemodel-style grouping.

Approving — supersedes my earlier (pre-reshape) change request.

if column == self.COL_REPOSITORY:
return row.repo_url
if column == self.COL_RETURNCODE:
return str(row.returncode)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [info] Returncode sorts lexicographically. The cell is rendered as a string, so the proxy compares returncode as text. It's an IntegerField(default=-1) (store/models.py:241), so values sort as '-1' < '128' < '2' rather than numerically. You already flagged the string-sort tradeoff for LogPage, so fine to defer — just noting returncode is the one column where it's actually wrong, and it's the exact case the Archive sort-role work will solve.

trans_late('LogPage', 'Returncode'),
)

def __init__(self, parent: Optional[Any] = None):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [nit] parent: Optional[Any] → prefer Optional[QObject] to match the typed style used in treemodel.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants