Round-trip Settings through panel interfaces (#23) - #32
Open
mikeallisonJS wants to merge 2 commits into
Open
Conversation
Decouple Settings persistence from GUI widget introspection. Each main-window panel now exposes a small read_settings/apply_settings interface for its slice of Settings, and collect_settings_from_gui delegates to those panels instead of reaching into scattered gui.template_vars / gui.extension_vars / gui.exclude_unknown_vars attributes. - TemplatePanel / ExtensionFilterPanel / PreferencesPanel gain read_settings + apply_settings; the bind_to_app mirroring that copied widget refs onto the app shell is removed from every panel. - The main window reads templates via _get_template_settings() and preview rows via preview_panel.preview_files rather than mirrors. - Add a session-scoped Tk root fixture in conftest so GUI test modules share one interpreter (re-creating Tk per module fails on Windows). - Add tests/test_settings_panel_roundtrip.py: apply a Settings through the panels, collect it back, and assert it matches save/load file behavior; also assert the legacy widget-ref mirrors are gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request refactors GUI panel state management from direct app attribute binding to a Settings-object-based API. Four panel classes replace ChangesPanel Settings API Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Updated the PreferencesPanel to directly read and apply settings from its controls, removing reliance on the application's state. The `sync_controls` method was eliminated in favor of a more explicit `apply_settings` method that pushes settings into the panel. Additionally, the ArchimediusGUI now applies settings directly to the preferences panel, enhancing clarity and maintainability. - Removed the `_sync_inline_preferences_controls` method from ArchimediusGUI. - Updated `read_settings` and `apply_settings` methods in PreferencesPanel for better data handling. - Introduced a new `_sync_extension_texts` method to manage extension text boxes. This change improves the separation of concerns between the GUI and settings management, aligning with recent refactoring efforts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
archimedius_gui.py (1)
1049-1057:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset should reapply the
TemplatePaneldefaults too.
exclude_unknownnow lives inTemplatePanel.read_settings()/apply_settings(), but this reset path only resets template entries and then reappliesPreferencesPaneldefaults. The old exclude-unknown flags survive a reset and get written straight back out on the next save.Proposed fix
- self.template_panel.reset_to_defaults() self.extension_filter_panel.reset_all_selected() self.settings = default_settings() sync_gui_from_settings(self, self.settings) self.settings.supported_extensions = copy.deepcopy(defaults.DEFAULT_EXTENSIONS) + self.template_panel.apply_settings(self.settings) self._refresh_extension_filters() self.apply_theme(self.dark_mode) self.preferences_panel.apply_settings(self.settings)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@archimedius_gui.py` around lines 1049 - 1057, The template reset path currently calls self.template_panel.reset_to_defaults() but never updates self.settings from the TemplatePanel defaults, so template-specific flags like exclude_unknown persist; after reset_to_defaults() call TemplatePanel.read_settings(self.settings) (or otherwise copy the TemplatePanel defaults into self.settings) before calling sync_gui_from_settings/apply_settings so that self.settings reflects the TemplatePanel defaults and preferences_panel.apply_settings(self.settings) writes the correct values back to the GUI/state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/preferences_panel.py`:
- Around line 168-202: apply_settings currently only repaints extension text
widgets via _sync_extension_texts and does not update the panel's in-memory
extension model, so a subsequent read_settings will return stale
supported_extensions; update the panel's model by assigning a deep copy of the
incoming settings.supported_extensions into the panel/app model (e.g.
self.app.settings.supported_extensions =
copy.deepcopy(settings.supported_extensions) or updating the exact model object
the panel reads), then call _sync_extension_texts(settings.supported_extensions)
as now so the GUI and internal model are synchronized.
In `@tests/test_settings_panel_roundtrip.py`:
- Around line 50-51: The test currently hardcodes POSIX paths for source_dir and
output_dir; change these to platform-neutral constructions by using os.path.join
or pathlib.Path to build the paths (e.g., construct source_dir and output_dir
values via os.path.join("data", "in") and os.path.join("data", "out") or
Path("data") / "in") so the settings in tests/test_settings_panel_roundtrip.py
are portable across OSes; update any assertions or uses that expect string vs
Path accordingly (convert Path to str where necessary).
---
Outside diff comments:
In `@archimedius_gui.py`:
- Around line 1049-1057: The template reset path currently calls
self.template_panel.reset_to_defaults() but never updates self.settings from the
TemplatePanel defaults, so template-specific flags like exclude_unknown persist;
after reset_to_defaults() call TemplatePanel.read_settings(self.settings) (or
otherwise copy the TemplatePanel defaults into self.settings) before calling
sync_gui_from_settings/apply_settings so that self.settings reflects the
TemplatePanel defaults and preferences_panel.apply_settings(self.settings)
writes the correct values back to the GUI/state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa3d2185-2ee1-4f81-b885-55c65b84fa44
📒 Files selected for processing (9)
archimedius_gui.pygui/extension_filter_panel.pygui/preferences_panel.pygui/preview_panel.pygui/template_panel.pysettings.pytests/conftest.pytests/test_gui_filter_preview.pytests/test_settings_panel_roundtrip.py
💤 Files with no reviewable changes (1)
- gui/preview_panel.py
Comment on lines
+168
to
+202
| def read_settings(self, settings: Settings) -> None: | ||
| """Read the preferences slice into *settings* from the panel controls. | ||
|
|
||
| The general-preference controls (toggles, logging level, collision | ||
| policy) are the source of truth and are read directly from their | ||
| widgets. The supported extension lists come from the in-memory model | ||
| rather than the editable text boxes, because those are validated and | ||
| committed only on Save. | ||
| """ | ||
| settings.supported_extensions = copy.deepcopy(self.app.settings.supported_extensions) | ||
| settings.show_full_paths = self.pref_show_full_paths_var.get() | ||
| settings.auto_save_enabled = self.pref_auto_save_var.get() | ||
| settings.auto_preview_enabled = self.pref_auto_preview_var.get() | ||
| settings.logging_level = self.pref_logging_level_var.get() | ||
| settings.dark_mode = self.pref_dark_mode_var.get() | ||
| settings.collision_policy = self._collision_policy_from_label( | ||
| self.pref_collision_policy_var.get() | ||
| ) | ||
|
|
||
| def apply_settings(self, settings: Settings) -> None: | ||
| """Apply the preferences slice of *settings* to the panel controls.""" | ||
| self.pref_auto_preview_var.set(settings.auto_preview_enabled) | ||
| self.pref_auto_save_var.set(settings.auto_save_enabled) | ||
| self.pref_show_full_paths_var.set(settings.show_full_paths) | ||
| self.pref_dark_mode_var.set(settings.dark_mode) | ||
| self.pref_logging_level_var.set(settings.logging_level) | ||
| self.pref_collision_policy_var.set( | ||
| defaults.COLLISION_POLICY_LABELS.get( | ||
| settings.collision_policy, | ||
| defaults.COLLISION_POLICY_LABELS[ | ||
| defaults.DEFAULT_SETTINGS["collision_policy"] | ||
| ], | ||
| ) | ||
| ) | ||
| self._sync_extension_texts(settings.supported_extensions) |
There was a problem hiding this comment.
Synchronize supported_extensions when applying panel settings.
read_settings() serializes self.app.settings.supported_extensions, but apply_settings() only repaints the text widgets. A direct panel.apply_settings(settings) followed by panel.read_settings(...) will still emit the previous extension map, so this panel API does not round-trip its full slice yet. That also leaks stale data into collect_settings_from_gui() because it now delegates to this method.
Proposed fix
def apply_settings(self, settings: Settings) -> None:
"""Apply the preferences slice of *settings* to the panel controls."""
self.pref_auto_preview_var.set(settings.auto_preview_enabled)
self.pref_auto_save_var.set(settings.auto_save_enabled)
self.pref_show_full_paths_var.set(settings.show_full_paths)
self.pref_dark_mode_var.set(settings.dark_mode)
self.pref_logging_level_var.set(settings.logging_level)
self.pref_collision_policy_var.set(
defaults.COLLISION_POLICY_LABELS.get(
settings.collision_policy,
defaults.COLLISION_POLICY_LABELS[
defaults.DEFAULT_SETTINGS["collision_policy"]
],
)
)
+ self.app.settings.supported_extensions = copy.deepcopy(
+ settings.supported_extensions
+ )
self._sync_extension_texts(settings.supported_extensions)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/preferences_panel.py` around lines 168 - 202, apply_settings currently
only repaints extension text widgets via _sync_extension_texts and does not
update the panel's in-memory extension model, so a subsequent read_settings will
return stale supported_extensions; update the panel's model by assigning a deep
copy of the incoming settings.supported_extensions into the panel/app model
(e.g. self.app.settings.supported_extensions =
copy.deepcopy(settings.supported_extensions) or updating the exact model object
the panel reads), then call _sync_extension_texts(settings.supported_extensions)
as now so the GUI and internal model are synchronized.
Comment on lines
+50
to
+51
| source_dir="/data/in", | ||
| output_dir="/data/out", |
There was a problem hiding this comment.
Use platform-neutral path construction in test settings.
Line 50 and Line 51 hard-code POSIX absolute paths, which can make these round-trip assertions brittle on non-POSIX runners. Build them with os.path.join()/pathlib to keep behavior portable.
Suggested patch
- source_dir="/data/in",
- output_dir="/data/out",
+ source_dir=os.path.join("data", "in"),
+ output_dir=os.path.join("data", "out"),As per coding guidelines: **/*.py: Use os.path.join() or pathlib for constructing file paths to ensure cross-platform compatibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_settings_panel_roundtrip.py` around lines 50 - 51, The test
currently hardcodes POSIX paths for source_dir and output_dir; change these to
platform-neutral constructions by using os.path.join or pathlib.Path to build
the paths (e.g., construct source_dir and output_dir values via
os.path.join("data", "in") and os.path.join("data", "out") or Path("data") /
"in") so the settings in tests/test_settings_panel_roundtrip.py are portable
across OSes; update any assertions or uses that expect string vs Path
accordingly (convert Path to str where necessary).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Tests