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

Rename desktop's Dialog to DialogWindow #661

Merged
merged 2 commits into from Jul 19, 2023

Conversation

MatkovIvan
Copy link
Member

@MatkovIvan MatkovIvan commented Jul 13, 2023

Proposed Changes

API change

- androidx.compose.ui.window.Dialog(onCloseRequest, state, visible, title, icon, undecorated, transparent, resizable, enabled, focusable, onPreviewKeyEvent, onKeyEvent, content)
+ androidx.compose.ui.window.DialogWindow(onCloseRequest, state, visible, title, icon, undecorated, transparent, resizable, enabled, focusable, onPreviewKeyEvent, onKeyEvent, content)

Testing

Test: N/A

@MatkovIvan MatkovIvan requested a review from igordmn July 13, 2023 10:41
@MatkovIvan MatkovIvan mentioned this pull request Jul 13, 2023
3 tasks
@igordmn
Copy link
Collaborator

igordmn commented Jul 13, 2023

We need to discuss that online with @m-sasha

@igordmn igordmn requested a review from m-sasha July 13, 2023 10:44
@m-sasha
Copy link

m-sasha commented Jul 13, 2023

Maybe call this PlatformDialog? We could even make it a policy to call "native" widgets wrapped in a Composable PlatformWidget.

@igordmn
Copy link
Collaborator

igordmn commented Jul 13, 2023

Naming it to PlatformDialog also looks good at the first glance. But at the second glance, we have to think significantly more:

  • about renaming Window -> PlatformWindow
  • about renaming MenuBar -> PlatformMenuBar
  • about renaming DialogWindowScope -> PlatformDialogScope
  • about creating PlatformPopup
  • does it make sense to create PlatformTextField instead of UIKItTextField on iOS
  • does it make sense to create PlatformTextField instead of SwingTextField. What will be used when we run tests, integrate into Fleet, or replace Swing by LWJGL?
  • Should we make LocalPlatform.platformDialogFactory, which affects PlatformDialog or not? What if we decide in the future that common Dialog should be also affected by LocalDialogFactory?
  • does this code look good:
fun main() = application {
   PlatformWindow {
      PlatformMenuBar {

      }
      if (dialogVisible) {
         PlatformDialog {

         }
      }
   }
}

If we just rename to DialogWindow we don't have to think about all of that now.

@m-sasha
Copy link

m-sasha commented Jul 13, 2023

The problem with DialogWindow is that it's too generic/abstract. The name doesn't hint at its uniqueness/how it's different from Dialog.
But, you're right, PlatformDialog is still too generic because it's not entirely clear what platform it refers to.
But, if we're doing UIKitTextField and SwingTextField then the dialog here should be SwingDialog. That seems to say exactly what it is.

@igordmn
Copy link
Collaborator

igordmn commented Jul 13, 2023

then the dialog here should be SwingDialog

It violates the current strategy of the window API - hide Swing as implementation details under the API and provide one small interop scope.window, which can be deprecated or renamed to an extension swingWindowOrNull similar to event.awtEventOrNull

@igordmn
Copy link
Collaborator

igordmn commented Jul 13, 2023

A lot of questions :)

The problem with DialogWindow is that it's too generic/abstract.

The same as with Window, application. It is on purpose.

@igordmn
Copy link
Collaborator

igordmn commented Jul 13, 2023

It is on purpose.

There are 2 reasons:

  • provide a simple DSL for creating a generic application
  • hide Swing implementation details, so we can replace it in the future (as said above)

May be it was a mistake to allow defining the platform Dialog anywhere in the composition, not only in ApplicationScope/WindowScope. But I can't say for sure yet.

@MatkovIvan MatkovIvan marked this pull request as ready for review July 18, 2023 10:23
@igordmn
Copy link
Collaborator

igordmn commented Jul 18, 2023

@m-sasha, will it be okay in the current state?

@m-sasha
Copy link

m-sasha commented Jul 18, 2023

If the goal is to hide the implementation then I guess I don't have a better suggestion for a name.

@MatkovIvan
Copy link
Member Author

If the goal is to hide the implementation

Not to hide but to distinguish in-window and on-canvas implementations

@igordmn
Copy link
Collaborator

igordmn commented Jul 18, 2023

Not to hide but to distinguish in-window and on-canvas implementations

The goal behind naming Window/DialogWindow instead of SwingWindow/SwingDialog is to hide details. See the messages above

(but distinguishing is also the goal, yes)

@MatkovIvan MatkovIvan merged commit dff9485 into jb-main Jul 19, 2023
2 checks passed
@MatkovIvan MatkovIvan deleted the ivan.matkov/rename-desktop-dialog branch July 19, 2023 07:18
MatkovIvan added a commit to JetBrains/compose-multiplatform that referenced this pull request Oct 10, 2023
eymar pushed a commit that referenced this pull request Feb 14, 2024
If a receiver is meant to listen only to system broadcasts and/or self broadcasts, the receiver doesn't need to be exported.

## Proposed Changes

  - Update documentation on `registerReceiver` methods

## Testing

Test: N/A

## Issues Fixed

Fixes: N/A

This is an imported pull request from androidx#661.

Resolves #661
Github-Pr-Head-Sha: e7c8f3b
GitOrigin-RevId: 9959398
Change-Id: I06dd7f5205893dec2069f40d64eacfca51c8e91c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants