Skip to content

fix(session): let a roll-wide reset undo an unwanted setting (#1047) - #1118

Merged
marcinz606 merged 3 commits into
marcinz606:mainfrom
thetalkingdrum:fix/reset-roll-to-defaults
Sep 19, 2026
Merged

marcinz606 merged 3 commits into
marcinz606:mainfrom
thetalkingdrum:fix/reset-roll-to-defaults

Conversation

@thetalkingdrum

@thetalkingdrum thetalkingdrum commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1047: once a boolean setting (flip H/V, Normalize, Use Luma/Color Average) got
applied across a roll, there was no way to push the unchecked state back out. A misclick
meant fixing every frame by hand.

Root cause: GranularSettingsDialog (behind Apply/Paste/Sticky) pre-selects and
surfaces rows by comparing each field to WorkspaceConfig() defaults. A field a user set
and then reverted to its default is indistinguishable from "never touched," so it's hidden
and unticked by default — there's no live frame left to use as a "pristine" source once
every frame in the roll already holds the unwanted value.

Fix:

  • Adds Reset Roll to Defaults… to the frame-list context menu (next to Apply Settings),
    which resets every visible frame to its own asset defaults in one step — reusing
    _asset_defaults, the same per-asset logic Reset Settings already uses, so a
    composite's inherited film process, a merge's seeded shadow lift, and stitch/triplet
    wiring survive the reset rather than being wiped by a blind WorkspaceConfig() overlay.
    Each frame's reset is still an ordinary undo step, and the active frame re-renders
    immediately.
  • With several frames selected, Reset Settings itself becomes Reset N Frames and
    resets the whole selection instead of only the active frame — the same confirm-gated
    reset_roll_settings, just scoped to state.selected_indices instead of the whole
    visible roll. Single-frame Reset Settings is unchanged (still unconfirmed, one click).
  • Closes a separate catalog gap: Normalize (e6_normalize), Use Luma Average, and
    Use Color Average had no SettingRow at all, so they were invisible to Apply/Paste/
    Sticky in either direction, independent of the reset gap above. e6_normalize is also
    a metering input — it decides whether an E-6 frame's highlight point comes from sampling
    the image or a fixed offset from the shadow — so it's registered in
    _BOUNDS_INPUT_FIELDS alongside its two siblings, or applying it alone would leave a
    target frame's cached bounds stale.

What this doesn't fix

The default-diff heuristic behind GranularSettingsDialog/_row_edited is unchanged: Apply
Settings still can't tell "this field is at its default because the user put it there" apart
from "never touched," so a source frame holding an unwanted-but-default value still won't
pre-select or propagate through the normal Apply Settings picker. This PR is a blunt escape
hatch for that case, not a fix to the picker itself. The root fix would track which fields a
user has actually interacted with, independent of the resulting value — no such tracking
exists anywhere in WorkspaceConfig/session today — which is a bigger redesign than this
issue's reported symptom calls for. Flagging it in case it's worth its own issue.

Known limitation (pre-existing, not introduced here)

A reset frame that isn't the active one keeps a stale film-strip thumbnail until you
select it — selecting it renders it correctly right away, so the underlying setting is
correct, only the thumbnail lags. This is not new: Apply Settings and Apply Preset
have the same gap today, since thumbnails only regenerate as a side effect of actually
rendering a frame, and both write straight to the DB for every frame but the active one.
Worth its own follow-up covering all three actions rather than a partial fix here.

Heads up on overlapping open PRs

Testing

make all (lint + type + full pytest) passes, with the one known pre-existing failure
(test_load_tooltip_follows_a_rebinding, a macOS key-symbol rendering difference — fails
the same way on main with this branch stashed, and CI runs Ubuntu).

Also verified end-to-end against the real FileBrowser widget and a real SQLite-backed
StorageRepository, and manually in the running app: the menu item appears correctly
positioned, the confirm dialog reports the right frame count, the active frame's canvas
updates immediately, and every frame's setting reverts to default with no crash. The
multi-frame-selection path got the same treatment, plus a driver run against real scans
from a live roll confirming a selected-but-inactive frame resets and an unselected frame is
left untouched.

…606#1047)

Once a boolean like flip H/V or Normalize was applied across a roll, there
was no way to push the unchecked/default state back out: Apply Settings'
picker only pre-selects rows that differ from default, so a field already
at its default on every source frame could never be chosen. Adds a
roll-scoped "Reset Roll to Defaults..." action that resets every visible
frame to its own asset defaults (reusing _asset_defaults, so composites and
merges keep what they are), plus closes a catalog gap where Normalize,
Use Luma Average and Use Color Average had no SettingRow at all and so
were invisible to Apply/Paste/Sticky in either direction.
reset_roll_settings mirrors apply_preset_fields's render=False convention
for the active frame, which requires the caller to trigger the render —
_reset_roll skipped that, so the canvas kept showing the pre-reset image
until the active frame was reselected.
@thetalkingdrum
thetalkingdrum force-pushed the fix/reset-roll-to-defaults branch from 726fa60 to 37bed39 Compare September 16, 2026 20:49
@thetalkingdrum
thetalkingdrum marked this pull request as draft September 17, 2026 16:55
With several frames selected in the file list, the context menu's
Reset Settings action now reads Reset N Frames and resets the whole
selection instead of only the active frame, reusing the existing
reset_roll_settings(scope="selection") and gated behind a confirm
dialog like Reset Roll to Defaults. Single-frame Reset Settings is
unchanged.

Also fixes two findings from reviewing this change: confirm_reset_frames
now uses count_of() for correct pluralization instead of a hardcoded
"frames", and e6_normalize (Normalize) is added to _BOUNDS_INPUT_FIELDS
so applying or pasting it alone invalidates a target E6 frame's cached
bounds, matching its sibling rows (Use Luma/Color Average) added in the
same PR.
@thetalkingdrum
thetalkingdrum marked this pull request as ready for review September 17, 2026 17:28
@marcinz606
marcinz606 merged commit 2a17b6e into marcinz606:main Sep 19, 2026
1 check passed
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.

[Bug]: Unchecked setting can not be transferred to other images of roll

2 participants