Conversation
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneContext.skiko.kt
Outdated
Show resolved
Hide resolved
|
Besides the name - LGTM |
MatkovIvan
left a comment
MatkovIvan
left a comment
There was a problem hiding this comment.
I'm ok with converting Empty object to class, but API shape/naming requires some polishing
compose/ui/ui/api/desktop/ui.api
Outdated
|
|
||
| public final class androidx/compose/ui/platform/PlatformContext$Companion { | ||
| public final fun getEmpty ()Landroidx/compose/ui/platform/PlatformContext; | ||
| public class androidx/compose/ui/platform/PlatformContext$Base : androidx/compose/ui/platform/PlatformContext { |
There was a problem hiding this comment.
It shouldn't be in API dump - please make sure that proper opt-in is applied
There was a problem hiding this comment.
What proper opt-in? Do you mean @InternalComposeUiApi annotation?
PlatformContext.Empty was in the API dump. Now PlatformContext.Base is in the API dump. Several other types scoped to PlatformContext are also in the API dump (RootForTestListener). If you think they shouldn't be, we can fix all of them in a separate PR.
There was a problem hiding this comment.
Please do it at least for new types in this PR.
Others might be done separately
|
|
||
| // This object must be stateless because it is used as a delegate in other ViewConfiguration | ||
| // implementations | ||
| object EmptyViewConfiguration : ViewConfiguration { |
There was a problem hiding this comment.
Not sure that exposing it like this makes sense from API point of view. Previously it was a getter of specific implementation, but now it's an API itself.
Why it's an object inside PlatformContext interface?
Also, as API it requres KDoc now
There was a problem hiding this comment.
It's all under @InternalComposeUiApi interface PlatformContext, so presumably it's internal compose API.
PlatformContext.Empty had no KDoc, many properties in PlatformContext have no KDoc.
There was a problem hiding this comment.
Yeah, it's semi-public but still it's used for cross modules and cross platforms, so it's better to add clarifications for teammates.
It's not a requirement, but good practice
There was a problem hiding this comment.
KDocs are needed for APIs that are intended to be used by the general public. For internal APIs it's not important whether they're used across modules. It's only important if there's something unusual that needs to be explained, and here there isn't - it's pattern used in many of other places, including 5 more times in this very file.
| } | ||
|
|
||
| override val inputModeManager = DefaultInputModeManager() | ||
| open class Base : PlatformContext { |
There was a problem hiding this comment.
Why do we need to declare it inside interface?
Also, missing KDoc
There was a problem hiding this comment.
We don't need to, but I like that it's scoped to PlatformContext. In my opinion PlatformContext.Base is better than BasePlatformContext. It's more discoverable, and it doesn't pollute the global scope.
m-sasha
force-pushed
the
m-sasha/fix-platformcontext-empty
branch
from
3eca6f2 to
f2c6f34
Compare
...ose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/PlatformLayersComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
3 participants
Replace PlatformContext.Empty with PlatformContext.Base() and make it a regular superclass of most implementations.
Fixes https://youtrack.jetbrains.com/issue/CMP-9195
Testing
Added a unit test to make sure the input mode is reset between tests.
Release Notes
Fixes - Multiple Platforms
LocalInputModeManager.current.inputModenot being reset between tests.