Repository navigation
fix(tune-editor): replace stacked globals on re-apply and host materi… - #864
Merged
hm21 merged 6 commits intoSep 16, 2026
Merged
Conversation
hm21
force-pushed
the
fix/tune-reapply-and-material-ui-host
branch
from
September 16, 2026 06:42
f3dde8d to
5ba379b
Compare
1 of 7 tasks
…al_ui dialogs Re-applying Tune appended session matrices and crushed the image. Collapse only this session's untimed globals, keep timed and custom entries, and seed sliders from the latest global per id. Hosts that use Flutter MaterialApp need a material_ui MaterialLocalizations ancestor for dialogs after the v14 UI migration. Co-authored-by: Cursor <cursoragent@cursor.com>
PIE CI fails PRs that are not formatted with `dart format`. Co-authored-by: Cursor <cursoragent@cursor.com>
… instead of an in-package shim MaterialUiLocalizationsScope wrapped the main editor, the tune editor and two dialogs. Sub-editors are pushed onto the host navigator, bottom sheets and the loading overlay are built there as well, so in a host that still uses `package:flutter/material.dart` the paint, text, filter and blur editors kept failing with "No MaterialLocalizations found" while only the tune editor worked. Every entry point would need the wrapper, and any new one would be a regression. The host owns the fix: adding material_ui's `GlobalMaterialLocalizations.delegates` to the SDK `MaterialApp` makes the `material_ui` and `cupertino_ui` localizations visible to every route and overlay. The README setup section now says so; the widget and its tests are gone.
- Seed the sliders from one map pass over the applied adjustments instead of scanning the list once per slider through a helper; the map's last-write-wins already yields the latest untimed entry per id. - Rename mergeTuneEditorResult to mergeTuneAdjustments, matching the file and the mergeFilterStates sibling, and name its parameters after what they hold (applied, sliders). - Drop the extra copy in openTuneEditor; addHistory copies every entry it records. - Accept any Decoration for bottomBarActiveItemDecoration, not only BoxDecoration. - Fold the state lookup into the cascade in the main editor tests, which cascade_invocations flagged and CI treats as fatal.
The preview rendered the merged list, i.e. the applied adjustments plus the sliders. Two problems with that: - The tune editor's FilteredWidget has no play position, so timed entries in the applied list were applied unconditionally, whatever their window. - A host that keeps its own adjustment list and seeds the sliders from the entry being edited (divine-mobile does this per tune "set") saw that entry twice: once from the applied list and once from the slider. Only done() merges; the preview shows what the sliders edit, as on stable. A test pins the contract.
hm21
force-pushed
the
fix/tune-reapply-and-material-ui-host
branch
from
September 16, 2026 07:16
d152e88 to
a311589
Compare
Owner
|
Thanks a lot for this PR, @saif-ellafi — great catch on the stacking bug, and the tests you brought along made the review much easier. The Here is what changed on top of your commits before merging:
Also verified against a large downstream app that drives the tune editor on a video timeline (~20k tests, plus a manual run on device). Thanks again! |
Contributor
Author
|
Amazing, I'm glad it helped. It's hard to take credit anymore unlike in my first PRs back then :) but well, at least the trigger and the testing! Cheers for improving the library |
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
2 participants
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.
Description
Hello, I've discovered the tune editor was stacking changes instead of restoring the state. This PR was AI-assisted and includes a bundle of related issues discovered during the testing process.
Tune no longer stacks a full extra set of global matrices on every Done. Re-apply used to append the session on top of history, which could crush the image (often to black).
mergeTuneAdjustmentskeeps timed and custom entries, replaces only this session's untimed globals, and the sliders are seeded from the latest global per id.A custom
TuneEditorWidgets.slidernow receives the value it reported throughonChangedinstead of the value from the last drag end.Adds
TuneEditorStyle.bottomBarActiveItemDecorationso hosts can mark the selected Tune attribute without forking the bottom bar.Maintainer edit (rebased onto
stable, review commits on top): theMaterialUiLocalizationsScopeshim was dropped. It only wrapped the main editor, the tune editor and two dialogs, but sub-editors, bottom sheets and the loading overlay are built under the host navigator, so in a host that still usespackage:flutter/material.dartthe paint, text, filter and blur editors kept failing withNo MaterialLocalizations found. The host owns that fix: adding material_ui'sGlobalMaterialLocalizations.delegatesto the SDKMaterialAppmakes the localizations visible to every route and overlay, verified against all sub-editors, the close-warning dialog and the loading dialog. The README setup section now documents it.Related Issue: Closes #
Type of Change