Conversation
5f80465 to
44d9b6c
Compare
| internal fun Dimension.toDpSize() = DpSize(width.dp, height.dp) | ||
| internal fun Point.toDpOffset() = DpOffset(x.dp, y.dp) | ||
| internal fun Rectangle.toDpRect() = DpRect( |
There was a problem hiding this comment.
Any reason to change it? Currently, it's aligned with the similar API across other platforms
There was a problem hiding this comment.
To me, Object.asSomething has the connotation of casting, or wrapping an object, like in Arrays.asList().
Object.toSomething(), on the other hand, has the connotation of converting, and creating a completely independent new object. Like in Array.toList()
| */ | ||
| @ExperimentalComposeUiApi | ||
| @Immutable | ||
| class DpInsets( |
There was a problem hiding this comment.
We must not introduce such API one more time as desktop only. Please reuse the existing one
cc @svastven
There was a problem hiding this comment.
The one in ios is internal; this one is public (but experimental).
We could change ios to use this one, but I wouldn't say it's a "must". There are plenty of places where we have internal copies of code because we don't want to create a dependency.
Up to the ios team...
There was a problem hiding this comment.
It's public and in skikoMain
There was a problem hiding this comment.
@InternalComposeUiApi is about using only inside our library. If you need really user-faced API - it should be existing foundation/commonMain one
There was a problem hiding this comment.
PlatformInsets is not the same as DpInsets.
What I thought you meant was this:
There was a problem hiding this comment.
DpInsets is currently internal to iosMain as convenience so that we don’t introduce an API that isn’t used anywhere else unless it’s actually needed.
Personally, I’m fine with having DpInsets as a dp-based convenience type alongside PlatformInsets, which is px-based and also internal API. I don’t see this as duplication, as long as DpInsets remains internal. If moved to skikoMain, then we can remove the one in iosMain.
If this new API is being used for window/platform insets, then I think we should use the existing WindowInsets / PlatformInsets APIs.
If we decide to introduce a public dp-based insets API, then I think it should live in common, at the same level as something like DpRect. But I am not sure this is the case.
As for PlatformInsets it is a px-based API. Its purpose is to represent the platform backing to WindowInsets, which are also px-based. The developer can then choose the PlatformInsets behavior they need for their particular case, for example dynamic getters, packed values, introduce new ones, etc.
There was a problem hiding this comment.
I moved DpInsets to skikoMain. Not sure what else should be done here.
There was a problem hiding this comment.
WindowInsets is related to DpInsets mostly just by name, and WindowRulers is even less related.
These are types for a concrete purpose, not generic geometry types like, for example, DpRect.
So I see no reason to forcibly use WindowInsets instead of a simple holder of 4 named Dp values. Also, WindowInsets is in foundation, while this new API is in ui.
There was a problem hiding this comment.
If we have an issue with DpInsets, there is an option to remove it and keep only availableBounds public - insets can be derived from it.
I am also fine with keeping the current DpInsets public.
Another option is to rename them to DpScreenInsets
There was a problem hiding this comment.
I don't see anything wrong with DpInsets.
7abe766 to
d0f6910
Compare
48f8bc8 to
fb106d0
Compare
| * | ||
| * Note that the actual placement is set asynchronously. | ||
| */ | ||
| fun requestPlacement(placement: WindowPlacement) { |
There was a problem hiding this comment.
How does it work with Wayland?
There was a problem hiding this comment.
In the end, the implementation goes through the same API as the old Window API, so it works the same way...
| /** | ||
| * The list of screens on which the window can be placed. | ||
| */ | ||
| val screens: List<Screen> = devices.map { Screen(it) } |
There was a problem hiding this comment.
Shouldn't it be state-backed? How to be notified about a new screen appearing?
There was a problem hiding this comment.
This is part of WindowScreenProviderScope, which is a very short-lived object. It's created for, and only exists, during the call to WindowScreenProvider.getScreen().
Also, there's no AWT API to monitor screens...
92a59f1 to
f23f001
Compare
f23f001 to
146a6be
Compare
| * | ||
| * @see androidx.compose.ui.window.v2.Window | ||
| */ | ||
| @ExperimentalComposeUiApi |
There was a problem hiding this comment.
Please add a KDoc note to each function that it is planned to be moved to androidx.compose.ui.awt after stabilization.
There was a problem hiding this comment.
You mean to every v2 function/class?
There was a problem hiding this comment.
Actually, we probably have to keep classes under "v2".
What we discussed earlier was about Composable functions
There was a problem hiding this comment.
Discussed that keeping it forever in "v2" is also an option.
igordmn
left a comment
There was a problem hiding this comment.
Approve for the current public API.
I also looked at the implementation, but not deeply.
49a2341 to
5502d13
Compare
… and `SwingWindow`
…metryProviderScope
…e and position separately
…owBoundsProvider`.
…ate to avoid accidentally using it. Add size and position properties to WindowState and DialogState.
5502d13 to
7619a90
Compare
This PR introduces a new WindowState API, and corresponding composable functions.
The main improvements relative to the current API:
DpSize.Unspecified).DpSize.Unspecifiedwith afillMaxSize()top-level modifier would cause the window to fill the screen. Requested in:WindowStatesaving/restoring (see CMP-1535 Revisit WindowState/DialogState API).Quick-start
The new APIs reside in
androidx.compose.ui.window.v2.rememberWindowState(initialScreenProvider=...)orWindowState.requestScreento specify the screen on which the window may be placed. The actual screen (which may change over time) is observable viaWindowState.bounds.rememberWindowState(initialBoundsProvider=...)orWindowState.requestBoundsto change the bounds of the window. The actual bounds (which may be different, and which will change over time) are observable viaWindowState.bounds.DialogWindowhas a similar API, also inandroidx.compose.ui.window.v2.Plan
The plan is to introduce this API as experimental, gather feedback from users and then deprecate the old one, replacing it with the new one.
Note that currently the PR only includes the window state API, but a similar one will be added for dialogs, once the window one has been generally approved by reviewers.
Fixes https://youtrack.jetbrains.com/issue/CMP-1535
Fixes https://youtrack.jetbrains.com/issue/CMP-9260
Fixes https://youtrack.jetbrains.com/issue/CMP-2923
Fixes https://youtrack.jetbrains.com/issue/CMP-7377
Fixes https://youtrack.jetbrains.com/issue/CMP-1873
Testing
The new API is tested via all the tests of the previous one, plus some additional tests.
This should be tested by QA
Release Notes
Features - Desktop
WindowStateAPI.