Repository navigation
Let inspector parameters be put back - #104
Conversation
- 🐛 #27 fixed the wheel-scroll bug that changed whatever field the pointer passed over; the damage it left could not be undone. Inspector parameters persist in QSettings, and nothing in the interface reset them — an fps of 11 and an alpha threshold of 0.4 that nobody chose followed the user into every later session and every later clip. - 🐛 Validation cannot catch this: `parameters_from_values` range-checks each persisted value, and every scrolled value is *inside* its range. They are legal values the user did not choose. - ✨ A "Reset to defaults" action in the inspector, beside the parameters it resets, describing its own scope — the creative parameters, not the output directory and not the machine settings that now live in Preferences. - 🔄 The reset travels through the reducer as a parameter event, so it invalidates a stale preview and persists like every other change instead of reaching into QSettings behind the store's back. Trigger: repro — scroll the inspector panel on a build before #27, then restart: the values stay. Closes #56
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| layout.addRow(self._form_label("Alpha threshold"), self.alpha_threshold_spinbox) | ||
| layout.addRow(self._form_label("Padding"), self.padding_spinbox) | ||
| layout.addRow(self._form_label("Horizontal stretch"), self.stretch_spinbox) | ||
| layout.addRow(self._form_label(""), self.reset_parameters_button) |
There was a problem hiding this comment.
💡 Quality: Reset button visual placement and tab order disagree
The reset button is added to _cleanup_controls (line 770), so it renders visually inside the "Crop & Cleanup" disclosure right after Horizontal stretch. But in tab_widgets it is appended dead last (line 858), after the entire Workspace section. A keyboard/screen-reader user tabbing through Crop & Cleanup jumps from the stretch spinbox straight to the Transform disclosure and only reaches "Reset to defaults" after Workspace, far from where it is shown. Given the inspector's strong tab-order/accessibility conventions, move the button to its visual position in the tab route (right after self.stretch_spinbox) or place it visually where the tab route expects it.
Was this helpful? React with 👍 / 👎
| scope = QCoreApplication.translate( | ||
| "Inspector", | ||
| "Compute acceleration, output directory, and output filename are not " | ||
| "affected.", | ||
| ) |
There was a problem hiding this comment.
💡 Quality: Reset scope text omits preserved crop/transform
The action's description states only "Compute acceleration, output directory, and output filename are not affected.", yet _reduce_parameters_reset also leaves transform (first_frame, crop) and top-level crop/crop_enabled untouched. A user reading the tooltip would reasonably infer crop and transform ARE reset, which contradicts the stated goal of an honest scope description. Consider mentioning that crop and transform framing are preserved (they have their own reset controls), or otherwise reconcile the text with what is actually reset.
Was this helpful? React with 👍 / 👎
| return _invalidate( | ||
| replace(state, timeline=timeline, model_available=model_available), | ||
| parameters, | ||
| PreviewInvalidationReason.CROP_CLEANUP, | ||
| ) |
There was a problem hiding this comment.
💡 Quality: Reset uses CROP_CLEANUP stale reason despite model/edge change
_reduce_parameters_reset always invalidates with PreviewInvalidationReason.CROP_CLEANUP, even when the reset changed model_id or edge_mode (which their own reducers classify as SEGMENTATION) or fps (SAMPLING). The category is purely presentational (it only drives the "preview again" status wording via presenter.py), so this is cosmetic rather than a caching bug, but the message shown after a reset that swapped the model will describe a crop/cleanup change. Consider choosing the most significant reason based on what actually changed.
Was this helpful? React with 👍 / 👎
- 🔄 Only the compiled `.qm` pair conflicted; they are generated, so they were rebuilt from the merged sources — 472 messages, none unfinished. - 🧪 Verified per CI job: 604, 182, 677 and 383 tests.
Code Review 👍 Approved with suggestions 0 resolved / 3 findingsAdds a Reset to defaults action in the inspector to recover from persisted parameters that were unintentionally set, addressing issue #56. The reset travels through the reducer like other parameter changes, invalidating stale previews and persisting consistently. Consider aligning the reset button's tab order with its visual placement in Crop & Cleanup, expanding the scope description to mention that crop and transform framing are preserved (they have their own reset controls), and choosing the preview invalidation reason based on what actually changed rather than always using CROP_CLEANUP. 💡 Quality: Reset button visual placement and tab order disagree📄 src/matteloop/ui/inspector.py:770 📄 src/matteloop/ui/inspector.py:846-858 The reset button is added to 💡 Quality: Reset scope text omits preserved crop/transform📄 src/matteloop/ui/inspector_reset.py:22-26 📄 src/matteloop/core/parameters.py:216-226 The action's description states only "Compute acceleration, output directory, and output filename are not affected.", yet 💡 Quality: Reset uses CROP_CLEANUP stale reason despite model/edge change📄 src/matteloop/core/parameters.py:238-242
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
1 participant
Closes #56.
#27 fixed the cause of the wheel-scroll bug — until then, scrolling the inspector changed whatever field the pointer passed over. The damage was never fixed. Inspector parameters persist in QSettings and there was no way to put them back: an fps of 11, an alpha threshold of 0.4, a horizontal stretch of 1.2 chosen by nobody, carried into every future session and every future clip.
Validation cannot rescue that.
parameters_from_valuesrange-checks every persisted value and falls back per key — but 11 fps and 0.4 % are legal values the user did not choose, which is precisely the case a range check cannot see. The only escape was deleting a registry key by hand.The change
A Reset to defaults action in the inspector, next to the parameters it resets, with a description that says exactly what it covers — the creative parameters, not the output directory or the machine settings that now live in Preferences.
The reset travels as a parameter event through the reducer like every other change, so it invalidates a stale preview and persists the same way rather than reaching into QSettings behind the store's back.