Support UiMediaScope (All Platforms) - #3216
Conversation
…which implements UiMediaScope and adds system theme and density for a unified management
…concerns, change value name to windowState and remove media resolution listener as it is implemented in WebMediaEnvironment
…ized if requested
…nd instead the LocalComposition provides a nullable HtmlElement instead of a Function provider. This way we defer creation of clip html target up until is needed and saves a few allocations on startup
Apolo (ApoloApps)
changed the title
Apolo (ApoloApps)
changed the title
|
Alexander Maryanovsky (@m-sasha) for Desktop changes |
| ComposeSceneKeyboardOffsetManager( | ||
| view = _overlayView, | ||
| keyboardOverlapHeightChanged = { height -> | ||
| mediaEnvironment.onKeyboardOverlapHeightChanged(height) |
There was a problem hiding this comment.
I would rather pair the mediaEnvironment with the KeyboardVisibilityListener directly - it tracks KB state form the very beginning and has keyboardFrame value to get correct height (if needed)
There was a problem hiding this comment.
keyboardFrame is not backed by mutableState, and so changes to it will not trigger recomposition on the place isImeShown is read (UimediaScope.KeyboardKind).
So i would have to pass mediaEnvironment to NativeKeyboardVisibilityListener to trigger
@OptIn(BetaInteropApi::class)
@ObjCAction
fun keyboardWillShow(arg: NSNotification) {
observers.forEach {
it.keyboardWillShow(arg.endFrame, arg.duration, arg.animationOptions)
}
keyboardFrame = arg.endFrame
mediaEnvironment.isImeShown = true// (substituting onKeyboardOverlapHeightChanged call)
}
@OptIn(BetaInteropApi::class)
@ObjCAction
fun keyboardWillHide(arg: NSNotification) {
observers.forEach {
it.keyboardWillHide(CGRectZero.readValue(), arg.duration, arg.animationOptions)
}
keyboardFrame = CGRectZero.readValue()
mediaEnvironment.isImeShown = false //(substituting onKeyboardOverlapHeightChanged call)
}There was a problem hiding this comment.
Not exactly. You can make MediaEnvironment implement the KeyboardVisibilityObserver and listen for keyboardWillShow/keyboardWillChangeSize/keyboardWillHide notifications there. As initial value, you can use the KeyboardVisibilityListener.keyboardFrame.height > 0.
P.S. Also, it seems like it's fine to convert keyboardFrame to state.
There was a problem hiding this comment.
Done. No need to convert KeyboardVisibilityListener.keyboardFrame to MutableState since it is only read to initialize isImeShown the first time. Subsequent updates to isImeShown state is done through MediaEnvironment as KeyboardVisibilityListener listening to keyboard changes
There was a problem hiding this comment.
Please rename file to the MediaEnvironment.ios.kt as well
There was a problem hiding this comment.
done (and fixed tests)
…ceneMediator iOS and a rebase error in ComposeWindowInternal Web which added density without the 'y'
… as listener to KeyboardVisibilityListener for changes. No need of MutableState in KeyboardVisibilityListener.keyboardFrame
…unction that returns the updated density) so that when drag is triggered is uses the latest system density
| internal object DefaultHapticFeedback : HapticFeedback { | ||
| override fun performHapticFeedback(hapticFeedbackType: HapticFeedbackType) { | ||
| } | ||
| @InternalComposeUiApi |
There was a problem hiding this comment.
I wonder if this interface can be internal? As I see it's used only in the ui:ui module.
There was a problem hiding this comment.
Nope, PlatformContext requires everything to be public (if not it leaks visibility) that's why it is marked with InternalComposeUiApi (and why we have Platform* interfaces in the first place, since common interfaces also have internal modifier thus cannot be used in PlatformContext)
| } | ||
| @InternalComposeUiApi | ||
| @OptIn(ExperimentalMediaQueryApi::class) | ||
| interface PlatformMediaEnvironment : UiMediaScope { |
There was a problem hiding this comment.
Why is this interface needed? Why do systemTheme and systemDensity need to be here?
There was a problem hiding this comment.
systemTheme and systemDensity are part of the MediaEnvironment of a device and I found it was a good way of centralizing similar logic into the same class. What I'm surprised is that it is not part of the common API on the first place. It is a good way of unifying scattered logic in a single, coherent place (encapsulating similar logic)
There was a problem hiding this comment.
It makes some sense, but I'm not entirely convinced.
Against it is the fact that UiMediaScope indeed doesn't have it, and we try to mostly adhere to the upstream decisions.
I also don't like that the code for managing system theme polling was moved from a specific to a generic file.
There was a problem hiding this comment.
I will be reverting as I do agree that maybe this is not the best place if UiMediaScope does not have it. I'll be removing PlatformMediaEnvironment in favour of directly using UiMediaScope. Also, reverting system theme polling changes in desktop to its own file
There was a problem hiding this comment.
reverted Desktop changes
| LocalPlatformScreenReader provides platformContext.screenReader, | ||
| LocalPlatformWindowInsets provides platformContext.windowInsets, | ||
| LocalPlatformPrefetchScheduler provides platformContext.prefetchScheduler, | ||
| LocalPlatformPrefetchScheduler providesComputed { platformContext.prefetchScheduler }, |
There was a problem hiding this comment.
why is providesComputed needed here?
There was a problem hiding this comment.
deferring platformContext.prefetchScheduler init
# Conflicts: # compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/ComposeWindowInternal.web.kt # compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/SystemThemeObserver.web.kt
…to previous state
5 participants
Add support for UiMediaScope in all platforms (some platforms are more complete than others or may have even simpler APIs to get the data from, any suggestion is welcomed)
Fixes https://youtrack.jetbrains.com/issue/CMP-10481/Add-Support-for-UiMediaScope
Release Notes
Features - Multiple Platforms