fix: Refresh icon preferences - #6690
Merged
validcube merged 7 commits intoApr 25, 2026
Merged
Conversation
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
validcube
force-pushed
the
feature-16-dev_refresh-icon-on-update
branch
from
April 25, 2026 13:20
0678047 to
c633a6c
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLawnchairThemeManager now takes a Changes
Sequence DiagramsequenceDiagram
participant User
participant Prefs as PreferenceManager
participant ThemeMgr as LawnchairThemeManager
participant Recents as RecentsModel
participant Cache as IconCache
participant Model as LauncherModel
User->>Prefs: change icon/appearance preference
Prefs->>ThemeMgr: PreferenceChangeListener triggers (watched prefs)
ThemeMgr->>ThemeMgr: parseIconStateV2 (includes prefs1State + prefs2)
Prefs->>Recents: onThemeChanged()
Prefs->>Cache: clear in-memory icon cache (async)
Cache-->>Prefs: cleared
Prefs->>Model: model.reloadIfActive() (background executor)
Model-->>User: icons refreshed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
validcube
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (1)
46-63:⚠️ Potential issue | 🟡 MinorStore listener references and remove them on close.
The five
prefs1.<pref>.addListener { verifyIconState() }registrations (lines 54–58) create listeners that cannot be removed because the lambda references are never stored.removeListenerrequires the exact listener object, and sinceaddListenerreturns no token or unsubscribe function, these listeners will persist indefinitely.While this is benign for a
@LauncherAppSingletonthat lives for the app's lifetime, it diverges from the coroutine cleanup pattern shown below it. Store each listener and callremoveListenerin thelifecycle.addCloseable { ... }block to keep behavior consistent and correct should the singleton lifetime change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 46 - 63, The five prefs1.<pref>.addListener registrations (prefs1.wrapAdaptiveIcons.addListener, prefs1.transparentIconBackground.addListener, prefs1.shadowBGIcons.addListener, prefs1.coloredBackgroundLightness.addListener, prefs1.forceIconMonochrome.addListener) register lambdas that are never stored and thus cannot be removed; capture each listener in a val (e.g., wrapAdaptiveListener, transparentBgListener, etc.) when calling addListener, use those references in lifecycle.addCloseable to call prefs1.<pref>.removeListener(listener) on close, and keep the existing scope.cancel() behavior so verifyIconState() callbacks are cleaned up consistently.
🧹 Nitpick comments (2)
lawnchair/src/app/lawnchair/icons/ThemeManagerModule.kt (1)
27-28: Consider a clearer name thanprefs1.Pairing
prefs1withprefs2reads as ordinal numbering, but the two refer to different classes (PreferenceManagervsPreferenceManager2), andprefs1is the older one — not the "first". Something likelawnchairPrefsorlegacyPrefswould be less confusing for future readers. The same applies to the parameter name inLawnchairThemeManager's constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/ThemeManagerModule.kt` around lines 27 - 28, Rename the ambiguous parameter prefs1 to a clearer identifier (e.g., legacyPrefs or lawnchairPrefs) everywhere it's used: in the ThemeManagerModule function signature and where it's passed into LawnchairThemeManager's constructor, and update the corresponding constructor parameter name inside LawnchairThemeManager to match; ensure the type stays PreferenceManager (prefs2 remains PreferenceManager2) and update all references to the old name to avoid compile errors.lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (1)
88-93: Minor readability: extract the suffix builder.The single-line concatenation is hard to scan. A
joinToStringover a list of the five preference values reads better and makes future additions safer.♻️ Suggested change
- var appShapeKey = currentAppShape.getHashString() - var folderShapeKey = currentFolderShape.getHashString() - - val prefSuffix = "${prefs1.wrapAdaptiveIcons.get()},${prefs1.transparentIconBackground.get()},${prefs1.shadowBGIcons.get()},${prefs1.coloredBackgroundLightness.get()},${prefs1.forceIconMonochrome.get()}" - appShapeKey += prefSuffix - folderShapeKey += prefSuffix + val prefSuffix = listOf( + prefs1.wrapAdaptiveIcons.get(), + prefs1.transparentIconBackground.get(), + prefs1.shadowBGIcons.get(), + prefs1.coloredBackgroundLightness.get(), + prefs1.forceIconMonochrome.get(), + ).joinToString(",") + val appShapeKey = currentAppShape.getHashString() + prefSuffix + val folderShapeKey = currentFolderShape.getHashString() + prefSuffix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 88 - 93, The code builds prefSuffix with an inline string template which hurts readability; extract the suffix construction into a clearer builder using a list of the five preference values (prefs1.wrapAdaptiveIcons.get(), prefs1.transparentIconBackground.get(), prefs1.shadowBGIcons.get(), prefs1.coloredBackgroundLightness.get(), prefs1.forceIconMonochrome.get()) and call joinToString(",") to produce prefSuffix, then append that prefSuffix to appShapeKey and folderShapeKey as before (references: appShapeKey, folderShapeKey, prefSuffix, prefs1.* getters).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 24-25: Remove or replace the informal/profane developer note in
LawnchairThemeManager.kt and replace it with a professional tracked TODO or
remove it entirely; if this is a real KSP incremental staleness concern, create
an issue and add a concise TODO comment referencing that issue (e.g., "TODO:
Investigate KSP incremental update staleness — see ISSUE-1234") so future
maintainers can find the bug, otherwise delete the comment line entirely.
---
Outside diff comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 46-63: The five prefs1.<pref>.addListener registrations
(prefs1.wrapAdaptiveIcons.addListener,
prefs1.transparentIconBackground.addListener, prefs1.shadowBGIcons.addListener,
prefs1.coloredBackgroundLightness.addListener,
prefs1.forceIconMonochrome.addListener) register lambdas that are never stored
and thus cannot be removed; capture each listener in a val (e.g.,
wrapAdaptiveListener, transparentBgListener, etc.) when calling addListener, use
those references in lifecycle.addCloseable to call
prefs1.<pref>.removeListener(listener) on close, and keep the existing
scope.cancel() behavior so verifyIconState() callbacks are cleaned up
consistently.
---
Nitpick comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 88-93: The code builds prefSuffix with an inline string template
which hurts readability; extract the suffix construction into a clearer builder
using a list of the five preference values (prefs1.wrapAdaptiveIcons.get(),
prefs1.transparentIconBackground.get(), prefs1.shadowBGIcons.get(),
prefs1.coloredBackgroundLightness.get(), prefs1.forceIconMonochrome.get()) and
call joinToString(",") to produce prefSuffix, then append that prefSuffix to
appShapeKey and folderShapeKey as before (references: appShapeKey,
folderShapeKey, prefSuffix, prefs1.* getters).
In `@lawnchair/src/app/lawnchair/icons/ThemeManagerModule.kt`:
- Around line 27-28: Rename the ambiguous parameter prefs1 to a clearer
identifier (e.g., legacyPrefs or lawnchairPrefs) everywhere it's used: in the
ThemeManagerModule function signature and where it's passed into
LawnchairThemeManager's constructor, and update the corresponding constructor
parameter name inside LawnchairThemeManager to match; ensure the type stays
PreferenceManager (prefs2 remains PreferenceManager2) and update all references
to the old name to avoid compile errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84c5a95e-a86f-43e9-ad12-33dae8ca9241
📒 Files selected for processing (3)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.ktlawnchair/src/app/lawnchair/icons/ThemeManagerModule.ktlawnchair/src/app/lawnchair/preferences/PreferenceManager.kt
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (1)
45-59:⚠️ Potential issue | 🟠 MajorFix thread safety: post
verifyIconState()callback touiExecutor.The removal of
drawer_themed_iconslistener is correct—parseIconStateV2()doesn't read it, so it has no impact oniconStateoriconMaskcomputation. The preference still triggers icon reloads viareloadIcons, which is separate fromLawnchairThemeManager.However,
prefListenercallbacks fromPreferenceChangeListener.onPreferenceChange()are invoked synchronously on the thread that committed the SharedPreferences change (often not the UI thread). This meansverifyIconState()→listeners.forEach { it.onThemeChanged() }can execute off-UI-thread. Since the merge flow explicitly usesMainScope()to ensure UI-thread execution, wrap theprefListenercallback:private val prefListener = PreferenceChangeListener { uiExecutor.execute { verifyIconState() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 45 - 59, prefListener currently calls verifyIconState() directly which can run off the UI thread because PreferenceChangeListener callbacks are invoked on the committing thread; change prefListener (the PreferenceChangeListener instance) so it posts the work to the UI thread by calling uiExecutor.execute { verifyIconState() } (i.e., replace the direct verifyIconState() invocation with a uiExecutor.execute wrapper) so verifyIconState() and subsequent listeners.forEach { it.onThemeChanged() } always run on the UI thread.
🧹 Nitpick comments (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (1)
55-67: Reduce duplication for the five listener registrations.The five
addListener/removeListenercalls and the matching pref reads inprefSuffixreference the same set of prefs. Collecting them once avoids drift if a sixth pref is added (or one is removed) and one of the three sites is missed.♻️ Proposed refactor
- prefs1.wrapAdaptiveIcons.addListener(prefListener) - prefs1.transparentIconBackground.addListener(prefListener) - prefs1.shadowBGIcons.addListener(prefListener) - prefs1.coloredBackgroundLightness.addListener(prefListener) - prefs1.forceIconMonochrome.addListener(prefListener) + watchedIconPrefs.forEach { it.addListener(prefListener) } lifecycle.addCloseable { scope.cancel() - prefs1.wrapAdaptiveIcons.removeListener(prefListener) - prefs1.transparentIconBackground.removeListener(prefListener) - prefs1.shadowBGIcons.removeListener(prefListener) - prefs1.coloredBackgroundLightness.removeListener(prefListener) - prefs1.forceIconMonochrome.removeListener(prefListener) + watchedIconPrefs.forEach { it.removeListener(prefListener) } } }And in the class body:
private val watchedIconPrefs by lazy { listOf( prefs1.wrapAdaptiveIcons, prefs1.transparentIconBackground, prefs1.shadowBGIcons, prefs1.coloredBackgroundLightness, prefs1.forceIconMonochrome, ) }Then
prefSuffixbecomes:val prefSuffix = watchedIconPrefs.joinToString(",") { it.get().toString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 55 - 67, The repeated addListener/removeListener calls and the prefSuffix construction should use a single collection to avoid drift: create a private val watchedIconPrefs (e.g., listOf(prefs1.wrapAdaptiveIcons, prefs1.transparentIconBackground, prefs1.shadowBGIcons, prefs1.coloredBackgroundLightness, prefs1.forceIconMonochrome)) and replace the five addListener/removeListener calls to iterate watchedIconPrefs.forEach { it.addListener(prefListener) } and watchedIconPrefs.forEach { it.removeListener(prefListener) } inside lifecycle.addCloseable (keep scope.cancel as-is), and compute prefSuffix as watchedIconPrefs.joinToString(",") { it.get().toString() } so all three sites reference the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 45-59: prefListener currently calls verifyIconState() directly
which can run off the UI thread because PreferenceChangeListener callbacks are
invoked on the committing thread; change prefListener (the
PreferenceChangeListener instance) so it posts the work to the UI thread by
calling uiExecutor.execute { verifyIconState() } (i.e., replace the direct
verifyIconState() invocation with a uiExecutor.execute wrapper) so
verifyIconState() and subsequent listeners.forEach { it.onThemeChanged() }
always run on the UI thread.
---
Nitpick comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 55-67: The repeated addListener/removeListener calls and the
prefSuffix construction should use a single collection to avoid drift: create a
private val watchedIconPrefs (e.g., listOf(prefs1.wrapAdaptiveIcons,
prefs1.transparentIconBackground, prefs1.shadowBGIcons,
prefs1.coloredBackgroundLightness, prefs1.forceIconMonochrome)) and replace the
five addListener/removeListener calls to iterate watchedIconPrefs.forEach {
it.addListener(prefListener) } and watchedIconPrefs.forEach {
it.removeListener(prefListener) } inside lifecycle.addCloseable (keep
scope.cancel as-is), and compute prefSuffix as
watchedIconPrefs.joinToString(",") { it.get().toString() } so all three sites
reference the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2014bab5-b4b9-4f90-978e-09196e352188
📒 Files selected for processing (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (2)
100-105: Prefer immutableval+ single-expression construction.Switching
appShapeKey/folderShapeKeyfromvaltovarjust to append a suffix is unnecessary mutation; the suffix can be concatenated at initialization. Also consider extracting the suffix into a small helper so the format isn't duplicated if another path needs the same key in the future.♻️ Proposed cleanup
- var appShapeKey = currentAppShape.getHashString() - var folderShapeKey = currentFolderShape.getHashString() - - val prefSuffix = "${prefs1.wrapAdaptiveIcons.get()},${prefs1.transparentIconBackground.get()},${prefs1.shadowBGIcons.get()},${prefs1.coloredBackgroundLightness.get()},${prefs1.forceIconMonochrome.get()}" - appShapeKey += prefSuffix - folderShapeKey += prefSuffix + val prefSuffix = buildString { + append(prefs1.wrapAdaptiveIcons.get()).append(',') + append(prefs1.transparentIconBackground.get()).append(',') + append(prefs1.shadowBGIcons.get()).append(',') + append(prefs1.coloredBackgroundLightness.get()).append(',') + append(prefs1.forceIconMonochrome.get()) + } + val appShapeKey = currentAppShape.getHashString() + prefSuffix + val folderShapeKey = currentFolderShape.getHashString() + prefSuffix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 100 - 105, The current code mutates appShapeKey and folderShapeKey using var to append a duplicated suffix; change both to val and construct them in one expression by concatenating the hash and suffix at initialization (e.g., val appShapeKey = currentAppShape.getHashString() + prefSuffix), and extract the repeated suffix construction into a small helper function (e.g., buildPrefSuffix() that returns the string built from prefs1.wrapAdaptiveIcons.get(), prefs1.transparentIconBackground.get(), prefs1.shadowBGIcons.get(), prefs1.coloredBackgroundLightness.get(), prefs1.forceIconMonochrome.get()) so both appShapeKey and folderShapeKey can call it without duplicating logic.
39-39: Renameprefs1to something descriptive.The name
prefs1is confusing alongside the existingprefs(LauncherPrefs) andprefs2(PreferenceManager2) — the "1" suggests an ordering that doesn't exist. ConsiderpreferenceManager,lcPrefs, orlawnchairPrefsto match the surrounding conventions, and update the references on lines 61–65, 69–73, and 103.♻️ Suggested rename
- private val prefs1: PreferenceManager, + private val lawnchairPrefs: PreferenceManager,(then update all
prefs1.usages in the file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` at line 39, The field name prefs1 in LawnchairThemeManager is ambiguous next to prefs and prefs2; rename prefs1 to a descriptive identifier like preferenceManager or lawnchairPrefs across the class (update the declaration of prefs1 and all usages where prefs1. is referenced, e.g., in methods that read or write theme preferences and in any places referenced alongside prefs and prefs2) and adjust import/constructor parameter names accordingly so all references (existing prefs1. usages) compile and follow the project's naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 100-105: The current code mutates appShapeKey and folderShapeKey
using var to append a duplicated suffix; change both to val and construct them
in one expression by concatenating the hash and suffix at initialization (e.g.,
val appShapeKey = currentAppShape.getHashString() + prefSuffix), and extract the
repeated suffix construction into a small helper function (e.g.,
buildPrefSuffix() that returns the string built from
prefs1.wrapAdaptiveIcons.get(), prefs1.transparentIconBackground.get(),
prefs1.shadowBGIcons.get(), prefs1.coloredBackgroundLightness.get(),
prefs1.forceIconMonochrome.get()) so both appShapeKey and folderShapeKey can
call it without duplicating logic.
- Line 39: The field name prefs1 in LawnchairThemeManager is ambiguous next to
prefs and prefs2; rename prefs1 to a descriptive identifier like
preferenceManager or lawnchairPrefs across the class (update the declaration of
prefs1 and all usages where prefs1. is referenced, e.g., in methods that read or
write theme preferences and in any places referenced alongside prefs and prefs2)
and adjust import/constructor parameter names accordingly so all references
(existing prefs1. usages) compile and follow the project's naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6aaa6a0f-c7a0-4644-9d2e-4d92c13b61dc
📒 Files selected for processing (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (2)
85-105: Minor: cacheprefs1State()once perparseIconStateV2invocation.
prefs1State()makes 5.get()calls and is invoked twice on lines 104 and 105 for keys that always share the same prefs1 component. Caching it once keeps the two keys provably consistent (no risk of a change between calls) and avoids the duplicated.get()work.♻️ Proposed refactor
+ val prefs1Snapshot = prefs1State() - val appShapeKey = currentAppShape.getHashString() + prefs1State() - val folderShapeKey = currentFolderShape.getHashString() + prefs1State() + val appShapeKey = currentAppShape.getHashString() + prefs1Snapshot + val folderShapeKey = currentFolderShape.getHashString() + prefs1Snapshot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 85 - 105, In parseIconStateV2, call prefs1State() once and store its result in a local val (e.g., val prefs1StateVal) before computing appShapeKey and folderShapeKey, then use that cached prefs1StateVal when forming appShapeKey and folderShapeKey instead of calling prefs1State() twice; update references to prefs1State() in the appShapeKey and folderShapeKey assignments to use the new local variable.
49-75: Optional: consolidate the five add/remove listener calls.The list of monitored
prefs1entries is duplicated ininitandaddCloseable, which is easy to drift out of sync if a future pref is added or removed (e.g., adding it only toinit). Iterating once over a single list keeps both sides in lockstep.♻️ Proposed refactor
init { val scope = MainScope() + CoroutineName("LawnchairThemeManager") merge( prefs2.iconShape.get(), prefs2.customIconShape.get(), ).onEach { verifyIconState() } .launchIn(scope) - prefs1.wrapAdaptiveIcons.addListener(prefListener) - prefs1.transparentIconBackground.addListener(prefListener) - prefs1.shadowBGIcons.addListener(prefListener) - prefs1.coloredBackgroundLightness.addListener(prefListener) - prefs1.forceIconMonochrome.addListener(prefListener) + val watchedPrefs = listOf( + prefs1.wrapAdaptiveIcons, + prefs1.transparentIconBackground, + prefs1.shadowBGIcons, + prefs1.coloredBackgroundLightness, + prefs1.forceIconMonochrome, + ) + watchedPrefs.forEach { it.addListener(prefListener) } lifecycle.addCloseable { scope.cancel() - prefs1.wrapAdaptiveIcons.removeListener(prefListener) - prefs1.transparentIconBackground.removeListener(prefListener) - prefs1.shadowBGIcons.removeListener(prefListener) - prefs1.coloredBackgroundLightness.removeListener(prefListener) - prefs1.forceIconMonochrome.removeListener(prefListener) + watchedPrefs.forEach { it.removeListener(prefListener) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 49 - 75, Consolidate the repeated add/remove listener calls by collecting the prefs to monitor into a single list (e.g., val iconPrefs = listOf(prefs1.wrapAdaptiveIcons, prefs1.transparentIconBackground, prefs1.shadowBGIcons, prefs1.coloredBackgroundLightness, prefs1.forceIconMonochrome)) and then iterate over that list to call addListener(prefListener) in the init setup and iterate again inside lifecycle.addCloseable to call removeListener(prefListener); keep existing use of prefListener, verifyIconState(), scope cancellation, and the merge/onEach flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 85-105: In parseIconStateV2, call prefs1State() once and store its
result in a local val (e.g., val prefs1StateVal) before computing appShapeKey
and folderShapeKey, then use that cached prefs1StateVal when forming appShapeKey
and folderShapeKey instead of calling prefs1State() twice; update references to
prefs1State() in the appShapeKey and folderShapeKey assignments to use the new
local variable.
- Around line 49-75: Consolidate the repeated add/remove listener calls by
collecting the prefs to monitor into a single list (e.g., val iconPrefs =
listOf(prefs1.wrapAdaptiveIcons, prefs1.transparentIconBackground,
prefs1.shadowBGIcons, prefs1.coloredBackgroundLightness,
prefs1.forceIconMonochrome)) and then iterate over that list to call
addListener(prefListener) in the init setup and iterate again inside
lifecycle.addCloseable to call removeListener(prefListener); keep existing use
of prefListener, verifyIconState(), scope cancellation, and the merge/onEach
flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac000045-4eac-491a-b200-4ef907715cc5
📒 Files selected for processing (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt
SuperDragonXD
approved these changes
Apr 25, 2026
SuperDragonXD
left a comment
SuperDragonXD
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me. You can just apply the nitpick comments of Coderabbit, then merge afterwards.
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt (1)
59-66: Centralize the watched-prefs list to avoid drift withprefs1State().The exact same five prefs are enumerated here (registration) and again in
prefs1State()at lines 82-84 (state hash). If a future pref is added/removed in only one of the two places, the listener and the cache key go out of sync — either icons fail to refresh on change, or the state key won't differ when expected, suppressing a real refresh. Extract a single source of truth.♻️ Proposed refactor
+ private val statePrefs1 by lazy { + listOf( + prefs1.wrapAdaptiveIcons, + prefs1.transparentIconBackground, + prefs1.shadowBGIcons, + prefs1.coloredBackgroundLightness, + prefs1.forceIconMonochrome, + ) + } + init { val scope = MainScope() + CoroutineName("LawnchairThemeManager") merge( prefs2.iconShape.get(), prefs2.customIconShape.get(), ).onEach { verifyIconState() } .launchIn(scope) - val statePrefs1 = listOf( - prefs1.wrapAdaptiveIcons, - prefs1.transparentIconBackground, - prefs1.shadowBGIcons, - prefs1.coloredBackgroundLightness, - prefs1.forceIconMonochrome, - ) statePrefs1.forEach { it.addListener(prefListener) } @@ - private fun prefs1State(): String = "${prefs1.wrapAdaptiveIcons.get()},${prefs1.transparentIconBackground.get()}," + - "${prefs1.shadowBGIcons.get()},${prefs1.coloredBackgroundLightness.get()}," + - "${prefs1.forceIconMonochrome.get()}" + private fun prefs1State(): String = + statePrefs1.joinToString(",") { it.get().toString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt` around lines 59 - 66, The same five prefs are duplicated between the registration block (statePrefs1) and prefs1State(), causing potential drift; extract a single shared list (e.g., val watchedPrefs1 or ICON_STATE_PREFS) and replace both the statePrefs1 declaration and the manual listing in prefs1State() to reference that single list so the listener registration (statePrefs1.forEach { it.addListener(prefListener) }) and the state-hash computation in prefs1State() always stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 59-66: The same five prefs are duplicated between the registration
block (statePrefs1) and prefs1State(), causing potential drift; extract a single
shared list (e.g., val watchedPrefs1 or ICON_STATE_PREFS) and replace both the
statePrefs1 declaration and the manual listing in prefs1State() to reference
that single list so the listener registration (statePrefs1.forEach {
it.addListener(prefListener) }) and the state-hash computation in prefs1State()
always stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3311d96-e226-4377-a29b-a5af989d2fdb
📒 Files selected for processing (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt
Signed-off-by: Pun Butrach <pun.butrach@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt`:
- Around line 45-72: The five prefs used in LawnchairThemeManager
(prefs1.wrapAdaptiveIcons, prefs1.transparentIconBackground,
prefs1.shadowBGIcons, prefs1.coloredBackgroundLightness,
prefs1.forceIconMonochrome) are causing duplicate refreshes because
PreferenceManager currently registers reloadIcons as their primary listener;
remove reloadIcons as the primary listener for those five preferences in
PreferenceManager.kt so they no longer trigger the reloadIcons path, leaving
LawnchairThemeManager's prefListener/verifyIconState() path (which calls
listeners.forEach { it.onThemeChanged() }) to own cache clears and model
reloads; ensure BasePreferenceManager.onSharedPreferenceChange() no longer
invokes reloadIcons first for these specific prefs to avoid redundant
MODEL_EXECUTOR clears/reloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09f5f0f7-e335-40c4-8e7e-56afaa142bbd
📒 Files selected for processing (1)
lawnchair/src/app/lawnchair/icons/LawnchairThemeManager.kt
Signed-off-by: Pun Butrach <pun.butrach@gmail.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
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
Make icon update instant, you don't have to force restart the app (workaround of LC16), and wait lawnchair to recreate the workspace (like it is on LC15)!
KSP incremental build fail, i have no idea why?? (Maybe it's because of my cache?)
Fixes #6684
Fixes #6413
Reasoning
Make Lawnchair PreferenceManager (1) work with LawnchairThemeManager! Woo, no more weird workaround with using AOSP preference system!
And make toggling switch that affect icons feels so much smoother (before we recreate the launcher every time these changes, now we refresh it!)
Testing
Change any of the icon preferences.
Type of change
✅ Bug fix (A non-breaking change that fixes an issue)
Summary by CodeRabbit
Refactor
Performance
New Behavior