Skip to content
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

Add isMaximized and setMaximized methods to PlatformOperations #916

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MohamedRejeb
Copy link

@MohamedRejeb MohamedRejeb commented Apr 21, 2024

Related to: JetBrains/compose-multiplatform-core#1148

Hi,
I added setMaximized methods to PlatformOperations which is using a native implementation for macos.
I added maximized property in HardwareLayer and SkiaLayer.awt.
This will fix a problem that we have on Compose Desktop when we try to do something like this: floating -> fullscreen -> maximized, the window will stay fullscreen, more details in the PR above.

The swing implementation on Windows and Linux is not tested yet.

The current implementation is not final, but it's working, so let me know what you think.

@MohamedRejeb MohamedRejeb changed the title Add setMaximized method to PlatformOperations Add isMaximized and setMaximized methods to PlatformOperations Apr 21, 2024
@MatkovIvan MatkovIvan requested a review from igordmn April 25, 2024 13:08
@igordmn
Copy link
Collaborator

igordmn commented May 8, 2024

Thanks for the PR!

Sorry for the delay, I plan to look at it more closely later. There are potential race conditions with the current code, I am still not sure if they are relevant or not, I need to look at it more carefully.

@MohamedRejeb
Copy link
Author

Any updates on this?

@igordmn
Copy link
Collaborator

igordmn commented Aug 30, 2024

Sorry, no update

@igordmn
Copy link
Collaborator

igordmn commented Aug 30, 2024

A preliminary review:

  1. The most obvious race condition is in performSelectorOnMainThread. Between calling setMaximized and performing the scheduled task, other operations can appear (checking isMaximized, or changing isFullscreen)

  2. There is existing race condition inside osxSetFullscreenNative (because of the same performSelectorOnMainThread). It worked not well so far. If we introduce an additional logic, we'll have more bugs.

  3. I am not sure about toggling instead of setting. Multiple scheduled toggle's will result in an undesired state.

  4. We have 2 different operation doing the same, but slightly differently (the new toggleFullScreenAndZoom and the old makeFullscreen). It can lead to an inconsistent state.

  5. We should have this contract:

layer.isFullscreen = true
println(layer.isFullscreen) // should be true immediately
println(layer.isMaximized ) // should be false immediately

layer.isMaximized = true
println(layer.isMaximized ) // should be true immediately
println(layer.isFullscreen) // should be false immediately

But the current logic may not guarantee that. Also I am not sure if the native functions also can guarantee it. If not, we should write workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants