[web] Reuse same mediaQuery listener logic both for resize and theme listener - #3231
Conversation
…listener Introduce dark/light theming to the demo
There was a problem hiding this comment.
Pull request overview
Refactors web media-query handling to reuse a shared listener implementation for both system theme changes and resolution/density changes, and updates the demo UI to better reflect system theming.
Changes:
- Introduce a reusable
MediaQueryListenerabstraction formatchMediachange handling (with legacy fallback APIs). - Switch system theme observation and window density/resolution listening to use the shared listener.
- Update demo top bars to adapt colors based on system dark theme.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/SystemThemeObserver.web.kt | Replaces bespoke theme media-query wiring with shared MediaQueryListener. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/MediaQueryListener.kt | Adds shared media-query listener abstraction (add/remove listener + matches helper). |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/ComposeWindowInternal.web.kt | Reuses shared listener for resolution/density change detection and adds disposal. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/events/DisposableEventListener.kt | Removes unused imports / minor cleanup. |
| compose/mpp/demo/src/commonMain/kotlin/androidx/compose/mpp/demo/Screen.kt | Updates Material2 top bars to apply theme-aware background/content colors. |
| compose/mpp/demo/src/commonMain/kotlin/androidx/compose/mpp/demo/ApplicationLayouts.kt | Makes top bar accent color theme-aware in the demo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This clashes with #3216 😬 |
| when(mediaQueryListener.matches()) { | ||
| true -> SystemTheme.DARK | ||
| false -> SystemTheme.LIGHT | ||
| else -> SystemTheme.UNKNOWN |
There was a problem hiding this comment.
It's a bit confusing to see a third branch when first 2 branches already cover true and false.
Then I looked at the matches() method and noticed that it returns Boolean?.
What do you think about creating a value class for the returned value?
For example:
internal value class MediaQuery private constuctor(private val matchResult: Int) {
companion object {
val NOT_SUPPORTED = MediaQuery(-1)
val NO_MATCH = MediaQuery(0)
val MATCH = MediaQuery(1)
}
}then your when becomes:
when(mediaQueryListener.matches()) {
MATCH -> SystemTheme.DARK
NO_MATCH -> SystemTheme.LIGHT
else -> SystemTheme.UNKNOWN
}
There was a problem hiding this comment.
Or maybe it's not the responsibility of fun matches to notify about the unsupported state.
MediaQueryListener might have a static method for checking if queries are supported.
Then fun matches will return a Boolean without ?
There was a problem hiding this comment.
isMatchMediaSupported can be even evaluated once (by lazy) and reused in elsewhere.
There was a problem hiding this comment.
I absolutely don't mind having dedicated status if it's not overkill - Boolean? though is more or less an idiom for yes/no/we-don't-know but sure, performance-wise we won't lose anything and may be it will be clearer
|
Apolo (@ApoloApps) I see that your PR will be able to reuse |
DefaultWindowState is moved to a different file from ComposeWindowInternal and I remove anything related to MediaQueries from ComposeWindowState. |
|
Hi Apolo (@ApoloApps) ! The changes you are working at in the dedicated PR do absolutely make sense, so don't takt this action as a neglect or devaluation of what you are doing It's just that by definition the goal you are trying to achieve is more fundamental and thus will take more time compared to this small fix we'd better have so far. Rebasing after some changes is part of workflow after all ;) |
81a1eb1
into
jb-main
…listener (JetBrains#3231) The goal of this PR is to use the same code for listening media query events in both cases we are actually doing it: - in theme listener - in resize listener Apart form the fact that reusing is beneficial, code that were used in theme listener was actually working in a broader set of browsers. Also, this is preparation for having better approach for dispose. Demo was updated to support theming (see screenshot) <img width="2944" height="1840" alt="Screenshot_20260715-103828" src="https://github.com/user-attachments/assets/e1c155ae-2625-46a7-b4f4-a54e46a47191" /> <img width="2944" height="1840" alt="Screenshot_20260715-103859" src="https://github.com/user-attachments/assets/57c18bfa-756e-4355-a0da-c1be98fc53de" /> ## Testing manual ## Release Notes N/A
The goal of this PR is to use the same code for listening media query events in both cases we are actually doing it:
Apart form the fact that reusing is beneficial, code that were used in theme listener was actually working in a broader set of browsers.
Also, this is preparation for having better approach for dispose.
Demo was updated to support theming (see screenshot)
Testing
manual
Release Notes
N/A