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 decoration parameter to Window and DialogWindow to control the thickness of the undecorated window resizers. #1505

Merged

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Aug 21, 2024

The thickness of the undecorated window resizers is currently hardcoded to 8.dp. This PR adds a composition local, LocalUndecoratedWindowResizerBorderThickness to allow users to change it.

Fixes JetBrains/compose-multiplatform#4574

Testing

Tested manually with:

fun main() = application {
    var thickBorders by remember { mutableStateOf(false) }
    Window(
        onCloseRequest = ::exitApplication,
        decoration = when {
            thickBorders -> WindowDecoration.Undecorated(40.dp)
            else -> WindowDecoration.Undecorated()
        }
    ) {
        Row(
            verticalAlignment = Alignment.CenterVertically,
            modifier = Modifier
                .fillMaxSize()
                .padding(100.dp)
        ) {
            Checkbox(
                checked = thickBorders,
                onCheckedChange = { thickBorders = it },
            )
            Text("Thick resizers")
        }
    }
}

It's difficult to write an automated test for this because UndecoratedWindowResizer relies on MouseInfo.getPointerInfo() which can't be simulated from tests.

Release Notes

Features - Desktop

The thickness of border resizers in undecorated windows and dialogs can now be controlled by passing a new decoration argument.

@m-sasha m-sasha requested a review from igordmn August 21, 2024 10:20
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API Guidelines:

Avoid implicit inputs provided via CompositionLocal

@m-sasha
Copy link
Member Author

m-sasha commented Aug 22, 2024

API Guidelines:

Avoid implicit inputs provided via CompositionLocal

We don't currently expose UndecoratedWindowResizer, so there is no place to pass an explicit argument.
We could expose it, or provide an additional parameter to Window and DialogWindow, but each of those comes with its own considerations and downsides.

@m-sasha m-sasha force-pushed the m-sasha/user-settable-undecorated-window-resizer-thickness branch from 7243da5 to 643529a Compare September 2, 2024 10:55
@m-sasha m-sasha changed the title Add LocalUndecoratedWindowResizerBorderThickness to control the thickness of the undecorated window resizers. Add decoration parameter to Window and DialogWindow to control the thickness of the undecorated window resizers. Sep 2, 2024
/**
* Defines the options for window decoration.
*/
interface WindowDecoration {
Copy link
Collaborator

@igordmn igordmn Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interface WindowDecoration {
sealed class WindowDecoration {

AOSP uses sealed class in a case when it isn't possible to have custom implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we didn't want to use sealed or enum classes because it can make it hard to provide backwards compatibility later when we want to add additional subtypes. For example, someone can write

when (decoration) {
    is System -> ...
    is Undecorated -> ...
}

and when we add another type, this will be broken.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right 🤔, let's keep interface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @igordmn - it should be sealed, but not class, sealed interface should work here. I guess it's covered by some policy - need to look for formal rule there, but the idea is to avoid implementation outside of our code. Explicit requirement in case of adding is pro, not cons

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @igordmn - it should be sealed, but not class, sealed interface should work here. I guess it's covered by some policy - need to look for formal rule there, but the idea is to avoid implementation outside of our code. Explicit requirement in case of adding is pro, not cons

If we design this interface similar to TextFieldDecoration and move the implementation of the Undecorated Resizer to this part, it can be provided to users to achieve an immersive title bar application experience on Windows. This requires exposing the Window to set window properties and exposing the HitTest related interfaces for hit testing of controls within the title bar. You can refer to this existing implementation for specifics. Additionally, in the current Resizer implementation, if the user calls a Dialog or Popup, the Resizer response will be blocked and won’t respond . If we implement the Resizer through this new interface and always place the WindowDecoration field above the ContentLayer, can this solve the problem? Would this design make the Decoration API more user-friendly and extensible?

@igordmn igordmn self-requested a review September 2, 2024 11:30
Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍. Let's wait for the second approval.

/**
* Defines the options for window decoration.
*/
interface WindowDecoration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @igordmn - it should be sealed, but not class, sealed interface should work here. I guess it's covered by some policy - need to look for formal rule there, but the idea is to avoid implementation outside of our code. Explicit requirement in case of adding is pro, not cons

/**
* The default thickness of the resizers in an undecorated window.
*/
val DefaultWindowResizerThickness: Dp = 8.dp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move it from global scope to companion object

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @igordmn - it should be sealed, but not class, sealed interface should work here. I guess it's covered by some policy - need to look for formal rule there, but the idea is to avoid implementation outside of our code. Explicit requirement in case of adding is pro, not cons

If we want a sealed interface here, we need to hide the actual subtypes from the user, to make the when scenario I mentioned impossible.

Copy link
Collaborator

@igordmn igordmn Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move it from global scope to companion object

We can reuse the AOSP pattern:

object WindowDecorationDefaults {
    val ResizerThickness: Dp = 8.dp
}

It is more preferable over companion object, because it will be consistent with AOSP

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, it is only for Undecorated.

I delegate the decision to @m-sasha

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to hide the actual subtypes from the user, to make the when scenario I mentioned impossible.

Why do we need to hide it? Why do we want to make such switch/when impossible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit requirement in case of adding is pro, not cons

It's OK to require extra case handling in this case. It won't break binary compatibility that we have to maintain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not dive into the exact semantics of what binary compatibility means.

De-facto, valid code that was written and compiled before the addition of another subtype will crash at runtime afterwards. That's breaking existing code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure. But the policy for breaking source code compatibility is MUCH softer than for binary compatibility. The reason behind this is affecting 3rd party libraries that users cannot control.
It doesn't sound like a reason for avoid exposing subtypes at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not source compatibility that will be broken. Existing compiled code will throw an exception at runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of it is about possibilities and potential changes. Not a blocker at the moment I guess, we can discuss it when we'll really need this

@m-sasha m-sasha force-pushed the m-sasha/user-settable-undecorated-window-resizer-thickness branch from 2fa61e5 to a86f773 Compare September 3, 2024 21:50
@m-sasha m-sasha merged commit 5ce381c into jb-main Sep 4, 2024
6 checks passed
@m-sasha m-sasha deleted the m-sasha/user-settable-undecorated-window-resizer-thickness branch September 4, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make resize area larger or configurable for undecorated windows
4 participants