Fix Dark/Light Mode toggle: preserve app theme, stop growing canvas, refresh l10n#4934
Merged
Merged
Conversation
Collaborator
Author
|
Compared 7 screenshots: 7 matched. |
Collaborator
Author
|
Compared 16 screenshots: 16 matched. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 106 screenshots: 106 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 106 screenshots: 106 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 106 screenshots: 106 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
…anvas Two regressions in the simulator's Dark/Light Mode menu (and the adjacent Larger Text menu), plus a related stale-compile bug behind "my localization edits don't take effect until I run mvn clean": 1. JavaSEPort: dark-mode / larger-text toggles now route through a new refreshThemeOnly() helper instead of refreshSkin. refreshSkin called Display.installNativeTheme(), which is UIManager.setThemeProps( nativeProps) and **replaces** the installed theme wholesale -- so a CSS-compiled app theme (custom UIID fonts, custom margins, overridden colors) was wiped to the bare native theme after one click. Visually the user saw "fonts are completely wrong" until the simulator was relaunched. refreshSkin also recomputed zoomLevel as canvas.width * retinaScale / skin.width, which equals retinaScale on a HiDPI display whose skin already fits the screen. The canvas then grew to skin * retinaScale while subsequent drawing still ran at zoomLevel == 1, so the simulator appeared to "activate zoom mode" for no reason after a Dark/Light click. refreshThemeOnly() uses UIManager.refreshTheme() (re-applies the currently-installed themeProps, clears the styles cache so shouldUseDarkStyle resolves $Dark<UIID> correctly) and leaves the canvas alone. 2. CompileCSSMojo: edits to a .properties localization bundle now invalidate the up-to-date check. The shared AbstractCN1Mojo.getCSSSourcesModificationTime only walks src/main/css, but the CSS compiler also reads the l10n directory (passed via -l) and bakes the bundle into the same theme.res. With the old check, l10n edits looked like "no change", the mojo skipped the compiler, and the stale theme.res hid the .properties update until a mvn clean. The new getLocalizationModificationTime() rolls the l10n directory's recursive modtime into the comparison. Tests: - CompileCSSMojoTest gains three cases: recompilesWhenLocalizationFileNewerThanThemeRes (the actual reproducer -- failed before the fix, passes after); skipsCompilationWhenAllInputsOlderThanThemeRes (guards against an over-eager fix that just disables the up-to-date check); recompilesWhenCssFileNewerThanThemeRes (locks down the CSS path so it can't regress alongside the l10n fix). A new setupMultiModuleCommonProject helper builds the canonical parent/common/ layout the production check is written against; the pre-existing single-module helper put CSS at project/src/main/css which left getCSSSourcesModificationTime returning zero and silently masked the staleness bug. - UIManagerDarkModeStyleTest gains two cases: testRefreshThemeAppliesDarkStyleAfterCachedLightLookup (the cached light style must not win after a dark-mode flip + refreshTheme); testRefreshThemePreservesAppThemeCustomizationsAcrossDarkModeFlip (app-only UIIDs and overridden colors must survive the refresh -- this is the contract refreshThemeOnly() relies on). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…every run
Follow-up to the l10n staleness fix in this PR. Reviewer caught a
feedback loop: when the user opts in to "Auto Update Default Bundle",
AutoLocalizationBundle persists to disk on every missing-key lookup
(see `get` -> `storeEntry` -> `persistIfNeeded(true)` -> `persist`).
Each persist advanced the bundle file's mtime, so
CompileCSSMojo.getLocalizationModificationTime -- which now factors l10n
mtime into the staleness comparison -- would treat the runtime cache
flush as a fresh user edit and force a ~30s CSS recompile on every
subsequent `cn1:run`.
Reproducer: opt in to auto-bundle, add `getString("hello")` to the app,
run twice. The second `cn1:run` recompiles CSS even though the user
changed nothing.
Fix: snapshot the bundle file's mtime before each persist and restore
it after the write. If an external editor advanced the mtime since our
last restoration (i.e. current mtime != preservedMtime), adopt the
user's mtime as the new preserved target so genuine user edits still
propagate through the staleness check. `setLastModified` failure is
treated as best-effort -- worst case is one extra CSS recompile, which
is the pre-fix behavior, not a regression.
Why neither the original code review nor the test plan caught this:
- I scoped my review to CompileCSSMojo in isolation and reasoned "the
compiler reads `-l <l10nDir>`, so l10n edits must invalidate the
cache." That logic is correct for user-driven edits. I never asked
who else writes to `src/main/l10n/*.properties` at runtime.
- The auto-bundle lives in Ports/JavaSE while the staleness check
lives in maven/codenameone-maven-plugin -- different modules, so the
cross-cutting writer didn't surface naturally during the mojo-level
review.
- The new CompileCSSMojoTest cases are pure mtime arithmetic
(Files.write + setLastModified). The auto-bundle never executes in
those tests, so its runtime write pattern can't show up. There was
no integration test of "compile -> simulate -> compile" that would
have asserted the second compile is a no-op.
Tests:
- AutoLocalizationBundleMtimeTest is new and covers four scenarios:
runtimePersistKeepsOriginalFileMtime (the actual reproducer);
multipleRuntimePersistsKeepOriginalFileMtime (no drift across many
persists); externalUserEditPropagatesToMtimeEvenAfterAutoBundleWrite
(a user edit between auto-bundle writes still propagates); and
freshFilePersistEstablishesPreservedMtime (no baseline yet -> first
write becomes the baseline). All four fail before this commit and
pass after.
The bundle is a private inner class of JavaSEPort so the new test uses
reflection, mirroring the established pattern in
tests/core/test/com/codename1/impl/javase/AutoLocalizationBundleTest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix-darkmode-toggle-and-l10n-staleness
branch
from
0cf991d to
a229d8c
Compare
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
1 participant
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
Three connected fixes after testing the recently added Dark/Light Mode menu:
JavaSEPort.refreshThemeOnly()replacesrefreshSkinfor the Dark/Light/Unsupported and Larger Text menu items.refreshSkinwas callingDisplay.installNativeTheme()-- which isUIManager.setThemeProps(nativeProps)and wipes the installed theme -- so a CSS-compiled app theme (custom UIID fonts, margins, colors) silently reverted to the bare native theme after one click.refreshSkinalso recomputed the canvas asskin * retinaScaleon HiDPI, growing it to ~2× while drawing stayed atzoomLevel == 1, which read as "the simulator activated zoom mode for no reason". The new helper usesUIManager.refreshTheme()(preserves themeProps, clears the styles cache soshouldUseDarkStyleflips correctly) and leaves the canvas alone.CompileCSSMojo.getLocalizationModificationTime()rolls the l10n directory's recursive mtime into the up-to-date check. The sharedgetCSSSourcesModificationTimeonly walkedsrc/main/css, but the CSS compiler also readssrc/main/l10n(passed via-l) and bakes the bundle into the sametheme.res. Without this, editingMessages.propertieslooked like "no change", the mojo skipped the compile, and stale strings stayed intheme.resuntilmvn clean.Three new
CompileCSSMojoTestcases (one is the actual reproducer that fails before the fix) and two newUIManagerDarkModeStyleTestcases that pin the contractrefreshThemeOnly()relies on: cached light-mode styles must give way to dark variants afterrefreshTheme, and app-only UIIDs / overridden colors must survive a dark-mode flip.Test plan
mvn test -Dtest=CompileCSSMojoTest(7/7 pass; the newrecompilesWhenLocalizationFileNewerThanThemeResfails on master and passes here)mvn test -Dtest=UIManagerDarkModeStyleTest(6/6 pass)codenameone-javasetest suite greentheme.cssstill apply and the simulator window doesn't grow.propertieslocalization bundle, restart the simulator, confirm the change appears withoutmvn clean🤖 Generated with Claude Code