refactor(misc): move settings filtering into store/ helper#2514
Conversation
|
@m3nu could you let me know if this structure looks okay? If so, I'll go ahead and refactor the bigger tabs the same way. |
There was a problem hiding this comment.
Thanks for picking this up — the Phase 5 direction is right, and getting a clean ViewModel pattern established here is exactly what will make the bigger tabs tractable. Since you're planning to use this as the template for the rest, I'd like to nail the shape before we replicate it. A few things to address:
1. Let the ViewModel own the data/policy logic — not just relay queries.
Right now get_settings_groups() returns the raw Peewee query and save_setting() is unchanged, so the only thing that really moved is the filter loop. Meanwhile the one genuinely data-driven decision — sys.platform != 'darwin' and group.group == 'Updates' — is still in the view. That's the kind of logic the VM should own. Ideally the view asks the VM "what should I render?" and gets back prepared, platform-correct groups + settings, with no DB queries or sys.platform checks left in populate(). That's what makes the abstraction earn its keep (and what makes it testable without a widget).
2. Let's settle what "ViewModel" means in Vorta before it spreads.
As written this is closer to a stateless repository/service than an MVVM ViewModel (no exposed state/commands). That's a legitimate choice — but whichever we pick becomes the convention for ArchiveTab & co., so let's be deliberate. Either lean into a real VM (owns presentation state, exposes the prepared data above), or name it for what it is. Happy to hear your preference.
3. Reconcile with the existing BaseTab persistence helpers.
BaseTab already provides save_profile_attr / bind_profile_attr etc. (from the earlier #2361 work). Adding a separate VM path for settings persistence gives us two mechanisms for "persist a user change." Before the larger tabs get migrated, let's make sure the VM layer composes with BaseTab rather than competing with it.
If you can fold the platform filtering into the VM and make the tests isolated, this becomes a solid template and the bigger refactors will go much more smoothly. Appreciate the work here.
|
Update: the "what does a ViewModel mean here" question from points 1–2 is resolved on #2361 — we're dropping |
2358b3d to
92ec456
Compare
|
Thanks , reshaped per your feedback One judgment call I'd like your read on: I kept |
f59b44f to
f28e39e
Compare
Move checkbox grouping, legacy-key filtering, and platform-specific Updates handling out of MiscTab.populate() into get_grouped_checkbox_settings() in store/settings.py, with isolated unit tests for the helper.
f28e39e to
76ce4b5
Compare
There was a problem hiding this comment.
🔍 Review — move misc settings filtering into a store/ helper
This is exactly the redirect from #2361 (drop the VM, move the platform Updates skip + legacy-key filter into a store/ helper), and it's a faithful extraction. I verified each behavior against the old populate():
- Legacy-key filter preserved, now O(1) set membership (
key not in valid_keys) instead of the linearsearch(...). Updates-off-darwin behaves identically —get_misc_settings()declarescheck_for_updates/updates_include_betaonly underif sys.platform == 'darwin'(settings.py:144), so off-darwin those keys aren't invalid_keys, the group empties out and is dropped. The platform gating is now data-driven inget_misc_settings()rather than a hardcoded group-name check in the view — cleaner, and the helper docstring documents it.- Group order (
order_by(group.asc())) preserved; empty/all-legacy groups are now dropped rather than rendered as bare headers (an improvement).
Tests cover alpha order, legacy filtering, Updates-off-darwin, and declared-keys-only. Approving — this supersedes my earlier (pre-reshape) change request.
Re your open question (save_setting)
"I kept
save_setting()as a 2-line direct ORM call in the view… happy to lift it assave_setting/bind_settingon BaseTab if you'd prefer."
Keep it in the view — don't lift to BaseTab. The get()/.value=/.save() pattern has no second caller: every other settings write in the views uses a different idiom (SettingsModel.update(...).where(...), e.g. main_window.py:351-354, source_tab.py:264-267). Lifting a single-use method to the base class would be symmetry for its own sake. If a unifying abstraction is ever wanted, the better home is a setter in store/settings.py that reconciles the get().save() vs update().where() split — but that's out of scope here. Non-blocking either way.
Description
This PR addresses the ViewModel extraction for the
MiscTaband introduces structural organization for future ViewModels.Related Issue
#2361 Phase 5 Partially
Motivation and Context
This is part of the ongoing architectural effort to decouple UI from data access logic. Moving the filtering and database logic into ViewModels makes the tabs easier to test without a full PyQt GUI context and reduces code duplication. Creating the
viewmodelsdirectory ensures the codebase remains organized as the remaining tabs are refactored during GSoC.How Has This Been Tested?
Added complete unit tests for
MiscTabViewModelintests/unit/test_misc_tab_viewmodel.pyto ensure it successfully retrieves groups, saves settings, and filters legacy keys correctly.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.