-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send correct window size on resize - this fixes popup crashes that happened #1166
Conversation
}) | ||
|
||
channel.trySend(getParentContainerBox()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have this before. Is it necessary to fix the issue? Or is it here for other reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We trigger exactly the same event the very first time as we trigger on resize - thus the logic of resolving/assigning dimensions is uniform and less prone for bugs when this logic is slightly different the very first time and on resize (this is exactly what happened before actually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need channel
here? It is asynchronous, and we can have 1-frame lag/blink because of that.
Can we just write:
var composeWindow: ComposeWindow? = null
window.addEventListener("resize", { _ ->
composeWindow?.resize(newSize)
})
composeWindow = ComposeWindow(...
composeWindow?.resize(newSize)
(we need to rewrite the public API requestResize
as well, so it is a suggestion for the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We intentionally do not react on every resize event anyway (there's a lot of them when we manually resize the browser window), because it's quite expensive. So we throttle the events anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we receive 1 resize event per frame, or it can be 10 events per frame? If it is the first, it shouldn't cause lags in theory.
We resize synchronously on desktop on every event. It is done intentionally, as the main relayout shouldn't be expensive - it just relayouts the top layout, and doesn't relayout the children. We strive to make resize as fast as the window resize, so users don't see black/white bars. We also have multiple reports that our resize still isn't ideal (because of AWT implementation we still have white bars).
As for resize on init - is there any reason to not make it synchronous? It isn't visible to user comparing to desktop (because the source code is loaded asynchronously), but I would still do that to avoid passing zero size on the first recomposition/layout - it can cause races with other init code.
…ith density in attachTo
val height = boxSize.height * density | ||
|
||
// TODO: What we actually should do is to pass this new dimensions to attachTo without setting them directly on canvas. | ||
// We should be HTMLCanvas-agnostic in that sense |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should aim to be HTMLCanvas-agnostic in CavnasBasedWindow implementations.
It's easier to implement the details here rather than in skiko, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by agnostic I meant that ideally there should be only one place where we are setting canvas dimensions physically. Now we do it in three different places and this can get out of hand. If this place will be somewhere in compose this is indeed an issue of design. But I'd rather definitely width/height setting to one place.
}) | ||
|
||
channel.trySend(getParentContainerBox()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need channel
here? It is asynchronous, and we can have 1-frame lag/blink because of that.
Can we just write:
var composeWindow: ComposeWindow? = null
window.addEventListener("resize", { _ ->
composeWindow?.resize(newSize)
})
composeWindow = ComposeWindow(...
composeWindow?.resize(newSize)
(we need to rewrite the public API requestResize
as well, so it is a suggestion for the future)
}) | ||
|
||
channel.trySend(getParentContainerBox()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate thing that is also important to mention (but not so important to fix) - if we still want async resize, better to subscribe on it inside a separate coroutine inside ComposeWindow
instead of LaunchedEffect
. When we subscribe inside LaunchedEffect
:
- a child (ComposeScene) manipulates its parent (Canvas), it can complicate the code in the future (it is simple right now).
- the Coroutine dispatcher for effects is partially frame-bound. It is complicated to explain in 2 words, but the main rule - if a coroutine doesn't affect the Composable content and doesn't depend on it - don't place it inside LauchedEffect.
}) | ||
|
||
channel.trySend(getParentContainerBox()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we receive 1 resize event per frame, or it can be 10 events per frame? If it is the first, it shouldn't cause lags in theory.
We resize synchronously on desktop on every event. It is done intentionally, as the main relayout shouldn't be expensive - it just relayouts the top layout, and doesn't relayout the children. We strive to make resize as fast as the window resize, so users don't see black/white bars. We also have multiple reports that our resize still isn't ideal (because of AWT implementation we still have white bars).
As for resize on init - is there any reason to not make it synchronous? It isn't visible to user comparing to desktop (because the source code is loaded asynchronously), but I would still do that to avoid passing zero size on the first recomposition/layout - it can cause races with other init code.
This is the minimal sufficient changes that doesn't supposed to introduce changes in skiko
This is fix for https://youtrack.jetbrains.com/issue/COMPOSE-969