[Config-driven linear onboarding dialogs] Extract render engine and controllers - #9365
Open
LukasPaczos wants to merge 28 commits into
Open
[Config-driven linear onboarding dialogs] Extract render engine and controllers#9365LukasPaczos wants to merge 28 commits into
LukasPaczos wants to merge 28 commits into
Conversation
afterFade is typed as an Animator factory, so entrance work that is not an Animator has to be wrapped in a zero-duration ValueAnimator purely to receive a start callback. That wrapper is only correct by way of a platform detail: end() on a never-started ValueAnimator fires onAnimationStart while cancel() does not, so a skip triggers the work and teardown suppresses it. Right semantics, but inherited rather than written, and unpinnable by a unit test that cannot inflate views. onContentReady states them as engine behaviour instead. It runs at the same point as afterFade, exactly once per render, on the animated, snapped and skipped paths alike, and never once the handle is unbound. The two slots are two ownership models at one moment: afterFade is bounded and engine-owned, onContentReady is unbounded and stopped by the binder in unbind. afterFade's KDoc also now records that the card stops intercepting touches as it starts, so a screen revealing interactive content there has to gate its own clickability until the animator ends. FakeBackgroundController gains the release() override its interface has required since that member was added. The test class did not compile without it.
The fit corrector re-anchored the card itself on its veto path, hardcoding verticalBias to 0f. CardAnchorController applies 0.5f on tablet for the same unanchored case, so the two disagreed and whichever ran last won. The corrector now reports through onDecorationHidden and leaves the card's constraints alone. Config-driven re-runs cardAnchor.apply(null), which also drops the arrow depth the callback used to set on its own. Legacy keeps the old write verbatim in its callback, so its behaviour is unchanged.
Embellishment.None had no placement data of its own, so it fell through to a hardcoded bias in CardAnchorController that also served the fit-veto case and was wrong for it: legacy pins the card high when a decoration does not fit, but centres it on a tablet when the screen has none. None now maps to an undrawn Space that reserves the room a decoration would have taken, floored at the card's bottom inset since the card anchors above the band and so never reserves that inset itself. A decoration-less card therefore lands near a decorated one without consulting whichever screen ran before it, which is what the dock step reached for by keeping the outgoing wing INVISIBLE. That trick made placement depend on step order, so the bottom wing's exit goes back to GONE. Placement moves into an exhaustive table and bias selection into a pure resolver, both now unit-testable. The table also means a new embellishment cannot compile until its placement is stated, and the arrow depth reads the band's drawsArtwork rather than inferring artwork from a non-null decoration.
Contributor
Author
|
bugbot run |
LukasPaczos
commented
Jul 31, 2026
Comment on lines
-119
to
-123
| dialog.updateLayoutParams<ConstraintLayout.LayoutParams> { | ||
| verticalBias = 0f | ||
| bottomToTop = ConstraintLayout.LayoutParams.UNSET | ||
| bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This is the only change to the "flag off" code path. This code is moved directly to BrandDesignUpdateWelcomePage, so no behavior change.
LukasPaczos
commented
Jul 31, 2026
Comment on lines
+549
to
+556
| onDecorationHidden = { | ||
| binding.daxDialogCta.cardView.setArrowDepthFraction(0f) | ||
| binding.daxDialogCta.root.updateLayoutParams<ConstraintLayout.LayoutParams> { | ||
| verticalBias = 0f | ||
| bottomToTop = ConstraintLayout.LayoutParams.UNSET | ||
| bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID | ||
| } | ||
| }, |
Contributor
Author
There was a problem hiding this comment.
Refs https://github.com/duckduckgo/Android/pull/9365/changes#r3690849481, this is the only change to production codebase, just a moved functional call.
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 722e873. Configure here.
LukasPaczos
marked this pull request as ready for review
July 31, 2026 14:10
Contributor
|
@LukasPaczos Overall looking good! Added some comments by me and from a Claude review (indicated by 🤖 prefix)
Screen_recording_20260803_120440.mp4
Screen_recording_20260803_121110.mp4
|
mikescamell
reviewed
Aug 3, 2026
Contributor
Author
Screen_recording_20260803_184729.mp4Is that something you can reliably trigger?
This is ready for another round! |
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.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1208671518894266/task/1216978525560082?focus=true
Tech Design URL (if applicable): https://app.asana.com/1/137249556945/project/1208671518894266/task/1216854264994244?focus=true
API Proposals URL(s) (if applicable):
Description
Step 2 of the config-driven onboarding dialogs work.
DialogConfig.ViewModelresolves the orchestrator's current step into the config, andDialogRenderEnginediffs itagainst the previous config to drive seven collaborators — one per visual axis, plus the shared card
choreography.
COMPARISON_CHART(stateless, with a bespoke content entrance) andADDRESS_BAR_POSITION(stateful) areported end to end. The whole arm sits behind
OnboardingBrandDesignUpdateToggles#configDrivenDialogs. Thelegacy renderer and its
ViewModelare untouched.Divergences from the tech design
ContentHandlegainsonContentReadyalongsideafterFadeafterFadeis anAnimatorthe engine owns end to end. Some entrance work can't be modelled that way (an unbounded loop, or animation driven outside the animator framework, for exampleINPUT_SCREEN's toggle animation), so it needs a side-effect hook that still runs exactly once per render on the animated, snapped and skipped paths.CardArrowControllerDialogConfig.cardArrow, so this is the place for that logic.afterFade/onContentReadymust be gated by the binderisAnimating→interceptChildTouchescovered the whole entrance. It doesn't: interception is released asafterFadestarts (so that the already visible CTA buttons are clickable). Anything interactive revealed inafterFadeis tappable while invisible (for example,INPUT_SCREEN_PREVIEWstep's suggestion buttons), so binders need to gate that directly. Documented onContentHandleand will impact only theINPUT_SCREEN_PREVIEWstep later on.Behaviour changes vs the legacy renderer (ported parts)
CardAnchorResolvernow aligns the bias across all steps. To let the resolver be the only owner, the sharedOnboardingDecorationFitCorrectorno longer writes the anchor itself. That reset moved intoBrandDesignUpdateWelcomePage#onDecorationHidden, setting the same three properties on the same view, so the legacy renderer behaves as before.ADD_TO_DOCKheld on to the outgoing bottom wing's anchor so the card stayed put, even though the wing itself was hidden, andINPUT_SCREEN_PREVIEWpinned it to the parent bottom. Both assumed a fixed step order.Embellishment.Nonenow declares its own independent placement, backed by aSpace, so the card keeps a comparable height on every undecorated screen. No anchoring rule reads which screen rendered before it, which matters because step order is meant to be reshuffled freely.Example of undecorated card's position on a tablet:
ADD_TO_DOCKandINPUT_SCREEN_PREVIEWhad to work around.This can be tested now by applying this diff:
but will be easier to exercise when rest of the steps are migrated.
Steps to test this PR
flag off
transitions and skips
fit calculation
arrow slide
Note
Medium Risk
Large new onboarding UI stack on a feature flag; first-run UX and animation/fit behavior differ from legacy in several documented ways until all steps are migrated.
Overview
Adds a config-driven onboarding path gated by
configDrivenDialogs()(default off). When brand design update and that toggle are on, onboarding usesConfigDrivenWelcomePageFragmentandbuildConfigDrivenPageBlueprints()instead of the legacy brand-design welcome page.Each step is described as value-comparable
DialogConfig;ConfigDrivenOnboardingPageViewModelmaps orchestrator steps viaDialogConfigResolver, andDialogRenderEnginediffs configs and drives collaborators (background, embellishments, card anchor/arrow/stage, step indicator, content binders). Comparison chart and address bar position are ported end-to-end; other dialogs auto-advance with existing side effects until follow-ups land.Layout/fit:
OnboardingDecorationFitCorrectorno longer repins the card when a decoration hides—hosts handle that (BrandDesignUpdateWelcomePage, config-driven embellishment path). AnundecoratedBandSpaceplusEmbellishment.Noneplacement keeps undecorated screens at a stable card height.CardAnchorResolvercentralizes card anchoring and arrow depth.Reviewed by Cursor Bugbot for commit b4d0f59. Bugbot is set up for automated code reviews on this repo. Configure here.