fix(picker): propagate setX/getX to the live lightweight popup wheels (#4897)#4902
Merged
Conversation
Setting Picker.setDate (and friends) while the lightweight popup was showing only updated the hidden committed value; the visible wheels stayed put. Custom popup buttons that read getDate() therefore worked off a stale base, so a "+7 days" button computed from the original date instead of the date the user had scrolled to (#4897). Track the live InternalPickerWidget on Picker, and have setDate / setSelectedString / setSelectedStringIndex / setTime / setDuration forward to it while the popup is on screen. Mirror the change on the getters so getDate() returns the current wheel value during edit; clear the field early in endEditing so external action listeners fired by Done see the just-committed value. The native picker has no in-progress wheel handle, so its semantics are unchanged - the Javadoc on each setter/getter spells out both behaviors. Drops the spinner.setValue(value); updateValue(); workaround from the custom-button handler. With propagating setters it is redundant, and worse, it actively clobbered user scrolling when an action did not touch value. The now-unused spinner parameter on createLightweightPopupButtonRow is removed. Adds PickerLiveValueScreenshotTest: opens a date picker on April 11, calls setDate(April 18) after the slide-up, and screenshots the wheels. Pre-fix the wheels stayed on April 11; post-fix they show April 18, so a regression here will fail the diff. Registered in Cn1ssDeviceRunner alongside the existing lightweight-picker tests and added to the JS chunk-stream skip list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
✅ JavaScript-port screenshot tests passed. |
Collaborator
Author
|
Compared 90 screenshots: 90 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
The anonymous ActionListener no longer references the enclosing Picker (only the captured LightweightPopupButton entry), so SpotBugs SIC_INNER_SHOULD_BE_STATIC_ANON correctly flags the implicit Picker.this back-reference as unnecessary. Extract it to a private static PopupButtonActionListener so the inner class stops retaining the enclosing instance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 90 screenshots: 90 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 90 screenshots: 90 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…ScreenshotTest Drops the dedicated PickerLiveValueScreenshotTest and instead piggybacks the #4897 propagation check on the existing custom-buttons test. Each variant now opens the popup with new Date() (so the wheels start on whatever day the test happens to run), then calls setDate(fixedDate) after the slide-up to spin them to April 11 2026 via the new live-propagation path. The committed baselines all show that fixed date, so a propagation regression would leave the wheels on the runtime "today" and fail the diff every day except by coincidence - without us having to maintain a separate screenshot per port. Adds a 400ms second-stage wait so the wheels settle at the new month/day/year before the PNG is captured. Total per-variant cost goes from ~6s to ~6.4s, still inside the 30s per-test deadline for four variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
getPickerValue(), the existingsetDate/setSelectedString/setSelectedStringIndex/setTime/setDuration(and matching getters) now operate on the live spinner while the lightweight popup is on screen, so a custom popup button can dosetDate(getDate() + 7d)and have the wheels track it.spinner.setValue(value); updateValue();) that clobbered user scrolling when the action didn't touchvalue. Cleans up the now-unusedspinnerparameter oncreateLightweightPopupButtonRow.PickerLiveValueScreenshotTest: opens a date picker on April 11 2026, callssetDate(April 18 2026)after the slide-up, screenshots the wheels. Wired intoCn1ssDeviceRunnerand the JS chunk-stream skip list (matches the other lightweight-picker tests).Test plan
mvn -pl core compile -DskipTests(JDK 8, framework target)mvn -pl common compile -DskipTestsforscripts/hellocodenameone(JDK 17)PickerLiveValueScreenshotTestbaseline gets generated and the existingLightweightPickerButtons*tests still match (the workaround removal does not change their visual output - those tests never click the buttons).picker.setDate(picker.getDate() + 7d)- wheels should now move 7 days forward from the visible position, not from the original base.🤖 Generated with Claude Code