Skip to content

fix(desktop): rescale cx/cy in rebaseForCurrentOutputs on output size change - #4272

Draft
kvnloo wants to merge 1 commit into
noctalia-dev:mainfrom
kvnloo:fix/noctalia-4123-rebase-cxcy
Draft

fix(desktop): rescale cx/cy in rebaseForCurrentOutputs on output size change#4272
kvnloo wants to merge 1 commit into
noctalia-dev:mainfrom
kvnloo:fix/noctalia-4123-rebase-cxcy

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Make rebaseForCurrentOutputs rescale widget cx/cy when placement size differs from the current output size, then write the new placement size — same arithmetic as remapForOutputChange.

Motivation

Origin issue #4123: after exitEdit, a widget centered on a 1366-wide output could be stored behind placement_width=1920, locking the old center forever because rebase only backfilled size without rescaling coordinates.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Build / packaging

Related Issue

Related to #4123

Testing

  • Replica of the function body (rebase_placement_test.cpp): baseline assignment-only fails cx must rescale 1366 -> 1920 (exit 1); candidate rescale then write size passes (exit 0).
  • Matching output size leaves cx/cy; legacy placementWidth==0 records size without inventing a rescale.
  • Diff limited to src/shell/desktop/desktop_widget_layout.h (rebaseForCurrentOutputs only). Does not touch settings.toml shadowing (open feat(config): warn when settings.toml shadows hand-authored widget placement keys #4124).

Checklist

  • This PR is ready for review, or it is marked as Draft.
  • I read and followed the relevant guidance in CONTRIBUTING.md.
  • I ran just format with clang-format v22+ installed, or this PR has no code changes.
  • I ran the relevant build or test commands, or explained why they were not run.
  • I self-reviewed the changes.
  • I checked for new warnings or errors.
  • I updated user-facing documentation in docs/user/ when this PR changes documented behavior or configuration, or this PR does not require documentation changes.
  • I added or updated assets/translations/en.json, or this PR adds no new user-facing strings.
  • I did not edit non-English translation files unless this PR is explicitly for translation tooling, an import/export sync, or a maintainer-requested locale change.
  • I used the existing canonical names for config keys, IPC names, paths, and identifiers.

Additional Notes

Prior PR #4236 was closed for an incomplete template body. Same candidate SHA a14172fcb227716ce7ca308669f5e8ff95c1df2e.

… change

rebaseForCurrentOutputs only backfilled placementWidth/Height, so exitEdit
locked 1366-centered widgets behind a 1920 placement size and skipped
remapForOutputChange. Mirror remapForOutputChange's rescale before writing
the current output size.

Refs noctalia-dev#4123
@ItsLemmy

ItsLemmy commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator
  1. It is a no-op for the reported bug

Both reproductions in #4123 have placement_* == current output size:

  • reporter: cx = 1280, placement_width = 3440 on a 3440-wide output
  • commenter: cx = 683, placement_width = 1920 on a 1920-wide output

The new guard is widget.placementWidth != width || widget.placementHeight != height. With placement already equal to the live size, the rescale never fires. The symptom is unchanged.

The actual corruption path is the backfill branch of remapForOutputChange (desktop_widget_layout.h:91-96): coordinates authored under an unknown older geometry get stamped with the current size and no rescale is possible, because the old basis is not recorded anywhere. The reporter says as much ("no deterministic retroactive fix"). rebaseForCurrentOutputs is not on that path: it runs only from exitEdit (desktop_widgets_controller.cpp:347, lockscreen_widgets_controller.cpp:238).

Load and hotplug already go through remapForOutputChange (desktop_widgets_controller.cpp:104,262-264;
lockscreen_widgets_controller.cpp:311-314,148-150), which does rescale correctly.

  1. It breaks the contract rebaseForCurrentOutputs exists for

cx/cy are absolute logical coordinates; placementWidth/Height records the basis they were authored in
(config_types.h:676-678). At editor exit, cx/cy come straight out of the editor, where they are the on-screen position on
the live output (desktop_widgets_editor.cpp:1472,1514, coordinates clamped against the current output). So the only stale thing is the recorded basis: stamping it is right, rescaling the coordinates the user just dropped is wrong. This is also the documented recovery mechanism for legacy coordinates (#3971: "needed a reapply").

Regression A: cross-output drag (multi-monitor, differing logical sizes)

updateDrag reassigns state->outputName and rewrites cx/cy into the target output's local space
(desktop_widgets_editor.cpp:2136-2139,2178-2183, group case 2210-2215), and never touches placementWidth/Height. So at exitEdit the recorded basis is the source output's size while the coordinates belong to the target output.

Drag a widget from 3840x2160 to 1920x1080 and drop it centered (cx = 960, basis still 3840):

            1920

cx = 960 × ────── = 480
new 3840

The widget teleports to the quarter line on exit, and saveSnapshotToConfig() (desktop_widgets_controller.cpp:350) persists it. Deterministic on any multi-output setup where logical sizes differ.

Regression B: geometry change while the editor is open

DesktopWidgetsEditor::onOutputChange() (desktop_widgets_editor.cpp:2626-2632) only calls syncSurfaces() + requestLayout(); it does not remap its own snapshot. The controller's remapForOutputChange at :262-264 operates on m_snapshot, which exitEdit then throws away (m_snapshot = m_editor->snapshot(), :345). So the editor's copy holds live coordinates with a stale basis: scale 1→2 on a 3440-wide output, exit edit, every widget's cx is halved relative to where it is drawn.

  1. Verification claim doesn't support the change

"Replica of the function body" tests the patch's own arithmetic assumption, not the behavior of the shell; the file isn't
in the diff. Layout/placement changes want functional verification in the running shell, which would have
surfaced both regressions above. Also note the patch is verbatim the suggestion in the #4123 comment; both share the same misreading of rebase vs remap.

Constructive alternative

The real invariant to restore is "recorded basis always matches the coordinates":

  1. Keep rebaseForCurrentOutputs as a pure basis stamp.
  2. In updateDrag, when the drag reassigns outputName (desktop_widgets_editor.cpp:2180-2183 and 2212-2215), also set placementWidth/Height to the target output's logical size. Basis changes, coordinates don't.
  3. For the editor-open geometry change, run remapForOutputChange on the editor's own snapshot at the top of
    DesktopWidgetsEditor::onOutputChange() before syncSurfaces(), so coordinates and basis move together and the user sees the remap.
  4. [BUG] Desktop widget placement: stale coordinates get locked in by placement backfill; config.toml placement keys are silently shadowed by settings.toml #4123's real symptom (pre-placement-tracking configs) is a loud hint plus docs, plus feat(config): warn when settings.toml shadows hand-authored widget placement keys #4124 for the settings.toml
    shadowing half. Note also that if two functions are meant to have identical arithmetic, one should be deleted rather
    than duplicated.

Also worth flagging, the design detail that made this patch look correct (cx/cy are live post-editor, only
the basis is stale) is not stated anywhere in desktop_widget_layout.h. A one-line comment on rebaseForCurrentOutputs would prevent the next PR of this shape.

@ItsLemmy
ItsLemmy marked this pull request as draft September 5, 2026 16:01
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.

2 participants