refactor(source): convert source files table to QTableView + QAbstractTableModel

[ebuzerdrmz44]

Description

Migrates the Source tab's file list from an item-based QTableWidget to a QTableView backed by a dedicated QAbstractTableModel (SourceFilesModel in views/partials/).

source_tab.py shrinks substantially: the imperative "disable sort → setItem loop → re-enable sort" choreography in add_source_to_table, set_path_info, update_path_info, and populate_from_profile is replaced by model calls. Per-row size/count recalculation now updates the backing SourceFileModel in place and emits dataChanged, instead of poking QTableWidgetItems by row index.

Related Issue

Part of #2361 (finish the views refactor / Phase 5 table-model migration).

Motivation and Context

QTableWidget couples data and presentation and forces row-index bookkeeping, which is exactly what caused the historical mid-recalculation crash (#1080 / #2435). Moving to QTableView + QAbstractTableModel makes the model the single source of truth and lets sort/selection compose through a QSortFilterProxyModel.

Key design points:

• Numeric-safe sorting via a shared SortProxyModel. The model exposes raw comparable values (dir_size as int, file-count as int) under a SortRole (Qt.UserRole); SortProxyModel.lessThan compares those keys in Python so sizes ≥ 2 GiB don't hit Qt's C++ 32-bit int truncation. The old SizeItem/FilesCount __lt__ hacks are gone.
• Selection via a SourceRole (UserRole + 1), returned from data(). Callers resolve the backing object with index.data(SourceRole) on proxy indices, which QSortFilterProxyModel auto-forwards through mapToSource.
• Themed path icons are cached on the model, keyed on dark-mode state and invalidated on theme switch, rather than re-rendered from disk on every paint.
• Path keying for recalculation results: set_path_info matches on dir and returns None when the source was removed mid-calculation, so the caller skips persistence.
• i18n preserved: headers keep the .ui's Form context (and "Calculating…" its SourceTab context), so existing catalog entries are reused.

Rebased on #2519 — shared sort proxy + SizeItem removed

#2519 (Archive tab, Phase 4 pt 1) merged first, so this PR is rebased onto it and folds in the cleanup that merge triggered:

• Extracted one shared sort proxy into views/partials/sort_proxy.py: a single generic SortProxyModel keying off Qt.UserRole, now used by both the Archive and Source tables. This removes the near-duplicate per-model proxy classes (ArchiveSortProxyModel and this PR's original SortProxyModel) before the remaining tab migrations spawn a third copy. It also fixes the both-None strict-weak-ordering nit deferred from Wire ArchiveTableModel into ArchiveTab (QTableView migration, Phase 4 pt 1) #2519 (if lv is None: return rv is not None).
• Deleted the now-dead SizeItem class and its orphaned imports (QTableWidgetItem, sort_sizes) from source_tab.py — archive_tab.py no longer references it after Wire ArchiveTableModel into ArchiveTab (QTableView migration, Phase 4 pt 1) #2519.
• Dropped a dead SIZE_DECIMAL_DIGITS import left in archive_tab.py by Wire ArchiveTableModel into ArchiveTab (QTableView migration, Phase 4 pt 1) #2519.
• Moved updateThreads from a class attribute to an instance attribute so worker lists aren't shared across SourceTab instances.

How Has This Been Tested?

• New tests/unit/test_source_files_table_model.py, including a > 2 GiB size-sort regression test that runs through SortProxyModel.
• Existing tests/unit/test_source.py updated; tests/unit/test_archive_table_model.py switched to the shared proxy.

Types of changes

• Refactor (non-breaking change; no user-facing behavior change)

Checklist:

• My code follows the code style of this project.
• I have added tests to cover my changes.
• All new and existing tests passed.

[ebuzerdrmz44]

Added SortProxyModel to fix the same ≥2 GiB size-sort overflow caught in #2519 .

This is intentionally a near-duplicate of #2519's ArchiveSortProxyModel. I kept them per-PR so neither blocks the other . Not sure that's the right call long-term though: how do you think we should handle the duplication? Dedupe into one shared proxy now, or leave it until the first of the two merges and clean it up then?

[m3nu]

🔍 Code Review

PR: #2523 — refactor(source): convert source files table to QTableView + QAbstractTableModel
Branch: refactor/source-table-model → master
Changes: +465 / −178 across 5 files

📋 Summary

Solid migration that honors every #2361 Phase 5 decision — but #2519 merged earlier today, which answers two open questions in this PR and calls for one cleanup round before merge.

✅ What's Good

• SourceRole resolves backing objects through the proxy on the destructive source_remove path — exactly the mapToSource footgun the #2361 decisions warn about.
• Path-keyed recalculation with a None return kills the #1080/#2435 row-index crash class, with a test for the removed-mid-calculation case.
• The SortProxyModel correctly follows the post-#2519 pattern, and the >2 GiB sort test covers the exact truncation regression that pattern exists for.
• i18n contexts check out (Form matches the .ui <class>; Calculating… keeps SourceTab), no DB reads in the model, thorough model tests.

🔍 Review Details

4 inline comment(s).

Severity	Count
🟡 Warning	2
🔵 Info	2

Re: your dedup question in the comments — merge order answered it: #2519 landed first, so I'd consolidate in this PR. Extract a shared generic proxy (both SortRoles are Qt.UserRole) into e.g. views/partials/sort_proxy.py and use it from both tables, before the smaller-tab migrations create a third copy. Details inline.

One non-inline nit: the PR description still says sorting is "not a lessThan proxy subclass" — stale relative to the code, worth updating so future archaeology isn't misled.

📊 Verdict: COMMENT 💬

Not merging as-is, but these are consequences of merge timing, not defects. One cleanup round and this is good to go.

---

🤖 Reviewed by Claude Code

[m3nu]

🟡 [dead-code] SizeItem is now dead code — #2519 landed first

Your cross-PR note assigns SizeItem cleanup to "whichever of the two lands second". #2519 merged today and current master's archive_tab.py no longer imports SizeItem — so the cleanup now belongs to this PR.

Fix: Delete the SizeItem class, plus the imports that become orphaned with it: QTableWidgetItem and sort_sizes. Note the ruff config ignores F401, so lint won't flag the dead imports — needs the manual sweep.

[m3nu]

🟡 [duplication] Consolidate with ArchiveSortProxyModel (now on master)

This is a line-for-line duplicate of ArchiveSortProxyModel (archive_table_model.py:199), and per your PR comment the plan was to dedupe once the first of the two merged — #2519 has now merged.

Since both models define SortRole = Qt.ItemDataRole.UserRole, the proxy doesn't actually need a per-model class at all.

Fix: Extract one shared proxy, e.g. views/partials/sort_proxy.py:

class SortProxyModel(QSortFilterProxyModel):
    """Sort proxy comparing `Qt.UserRole` keys in Python to avoid Qt's 32-bit int truncation."""

    def lessThan(self, left: QModelIndex, right: QModelIndex) -> bool:
        lv = left.data(Qt.ItemDataRole.UserRole)
        rv = right.data(Qt.ItemDataRole.UserRole)
        if lv is None:
            return rv is not None
        if rv is None:
            return False
        return lv < rv

and use it here and in archive_table_model.py (drop ArchiveSortProxyModel or alias it). This prevents a third copy appearing in the upcoming smaller-tab migrations.

[m3nu]

🔵 [strict-weak-ordering] Both-None case violates strict weak ordering

When lv and rv are both None, lessThan(a, b) and lessThan(b, a) both return True, which violates the strict-weak-ordering contract Qt's sort relies on and can produce unstable orderings.

Fix:

if lv is None:
    return rv is not None

This is the same nit deferred from #2519 — fixing it once in the shared proxy (see comment above) resolves both tables.

[m3nu]

🔵 [behavior] Full repopulate drops "Calculating…" placeholders on other rows

populate_from_profile() → set_rows() clears the model's _calculating set. So removing one source while others are mid-recalculation blanks their "Calculating…" cells (the data still lands correctly when each worker finishes — only the placeholder is lost). It also resets any ad-hoc sort back to the saved settings.

The old code removed rows without touching the rest of the table.

Suggestion (fine as a follow-up): add a targeted remove_source(source) to the model using beginRemoveRows/endRemoveRows instead of repopulating, leaving _calculating and the current sort untouched.

[ebuzerdrmz44]

on the Calculating…-placeholder-on-repopulate point . I will add the targeted remove_source() (beginRemoveRows/endRemoveRows) in a separate small PR rather than widen this one.

[m3nu]

Reviewed in depth — the migration is correct and pattern-faithful: model in partials/, raw SortRole keys, every index access resolved via index.data(SourceRole) so the proxy footgun is handled, and per-path (rather than per-row-index) async updates kill the #1080/#2435 crash class outright. Nice catches along the way: updateThreads was a shared class attribute on master, and the shared SortProxyModel extraction includes the both-None strict-weak-ordering fix. The 18 new model tests are solid.

Two asks before merge:

1. Guard against overlapping recalculations of the same path. _discard_update_thread disconnects and drops all threads whose objectName() matches the path. If the same path is recalculated twice concurrently (double-click on Update), the first result to arrive drops the only reference to the still-running second thread — "QThread destroyed while running" territory. Simplest fix: skip spawning in update_path_info when the path is already pending.
2. Add a remove-under-sort test: sort by size descending, select the first visual row, call source_remove(), assert the correct DB record was deleted. That's the exact proxy/mapToSource scenario from #2361 and the one convention-critical path currently untested.

Known and fine to defer to the beginRemoveRows follow-up you already mentioned: the full model reset in source_remove wipes other rows' "Calculating…" placeholders and the selection.

Merge order: this lands after #2525/#2526 and will need a rebase around the archive_tab.py import block (this PR deletes ArchiveSortProxyModel).

Take-or-leave nits: pass a roles list on the dataChanged emits (the archive model does), type the icon cache Dict[str, QIcon], header.setVisible(True) is redundant now that the .ui attribute is gone, and there's a stray blank line in the sort_proxy.py docstring.

[ebuzerdrmz44]

addressed , waiting for #2526 to merge so that we can marge this too after rebase

[m3nu]

Content review passed — both asks from my last round are addressed in f47320bc. Holding at COMMENT rather than approving only because of two mechanical blockers, not defects: the branch is CONFLICTING against master, and CI has never run on it, so the "all tests passed" checklist item is currently unverified by the pipeline.

Verified addressed:

• Duplicate-recalc guard — update_path_info now early-returns when a thread with the same objectName() is already in self.updateThreads. That closes the "QThread destroyed while running" window in _discard_update_thread.
• test_source_remove_under_sort — sorts by size descending, selects visual row 0, asserts the large record is the one deleted. Exactly the proxy/mapToSource scenario from the #2361 decisions.
• All the take-or-leave nits landed: roles lists on the dataChanged emits, typed Dict[str, QIcon] icon cache, redundant header.setVisible(True) dropped, docstring cleaned.

Things I checked independently that hold up:

• i18n context is right — source_tab.ui has <class>Form</class>, so trans_late('Form', 'Path'/'Size'/'File Count') reuses the existing catalog entries, and Calculating… keeps the SourceTab context that self.tr() produced before.
• QtCore is still used at source_tab.py:367 (WindowType.Sheet), so dropping the findItems call doesn't orphan the import.
• Sort persistence is intact: header.sortIndicatorChanged → update_sort_order is still wired, and sortItems → sortByColumn is the correct QTableView equivalent.
• set_icons() swapping the per-row SourceFileModel.get() loop for a viewport().update() is a strict improvement — it drops N DB queries per theme switch, and the dark-mode-keyed icon cache handles invalidation.
• Deleting SizeItem.__lt__ / FilesCount.__lt__ is a real correctness win beyond the sorting fix: FilesCount.__lt__ returned 1/0 for non-numeric text, which is not a valid strict weak ordering.

To merge:

1. Rebase onto master once #2526 lands (the conflict is the archive_tab.py import block — this PR removes ArchiveSortProxyModel and SIZE_DECIMAL_DIGITS).
2. Get CI green.

I'll approve on a clean rebase with green checks — no further content review needed.

One new nit worth folding into the rebase: SortProxyModel.lessThan hardcodes Qt.ItemDataRole.UserRole instead of reading self.sortRole(). Since this is now the shared proxy the remaining tab migrations will reuse, honouring setSortRole() costs one line and avoids someone subclassing it later. No-op today, as both models define SortRole = Qt.UserRole.

Still fine to defer, as agreed: the full model reset in source_remove() wiping other rows' "Calculating…" placeholders and the selection, pending your targeted beginRemoveRows/endRemoveRows follow-up.

[m3nu]

Content is good, but needs rebase.