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

Introduce ComposePanel that can be disposed manually #620

Merged
merged 2 commits into from Jul 6, 2023

Conversation

Walingar
Copy link
Collaborator

@Walingar Walingar commented Jul 4, 2023

Proposed Changes

Current implementation of ComposePanel disposes Compose content on each detach from Swing hierarchy. But for some cases (like toolwindows in IntelliJ) it doesn't work well, since state getting lost. This state can be controlled only externally. For example, in IntelliJ Disposable is used for that.

This PR adds the ComposePanel.isDisposeOnRemove property. By default, it is true. If it is false, the Compose state will be preserved when the ComposePanel is removed from the UI hierarchy. In this case, the Compose state must be controlled by developers - they must call ComposePanel.dispose() manually.

Testing

Test: run introduced test

Issues Fixed

No issues related to this PR

@Walingar Walingar requested review from igordmn and m-sasha July 4, 2023 13:40
@m-sasha
Copy link

m-sasha commented Jul 4, 2023

Just as an alternative option, you could put a dispose method in ``ComposePanel and add a disposeWhenRemoved flag to the constructor (and/or a setter). This way you could avoid inheritance.

@Walingar
Copy link
Collaborator Author

Walingar commented Jul 4, 2023

We discussed this flag with @igordmn and decided that flag that controls behavior is much worse than inheritance

@igordmn
Copy link
Collaborator

igordmn commented Jul 4, 2023

We discussed this flag with @igordmn and decided that flag that controls behavior is much worse than inheritance

From another PR:

Robert C. Martin's Clean Code Tip 12: Eliminate Boolean Arguments
Boolean arguments loudly declare that the function does more than one thing. They are confusing and should be eliminated.

@m-sasha
Copy link

m-sasha commented Jul 4, 2023

I'm not a big believer in "experts" and their generic advice.
Anyway, it doesn't have to go into the constructor, it can just be a setter/getter. Declaring (and exposing!) an additional class to vary a single behavior seems too much.

But anyway, I'm just suggesting...

@Walingar
Copy link
Collaborator Author

Walingar commented Jul 4, 2023

My thought was to have smth like this, so this boolean param it not exposed. It is used internally and only inside removeNotify

class ComposePanel internal constructor(
    private val skiaLayerAnalytics: SkiaLayerAnalytics,
    private val autoDisposable: Boolean,
) : JLayeredPane() {
  @ExperimentalComposeUiApi
  constructor(
      panelScope: CoroutineScope,
      skiaLayerAnalytics: SkiaLayerAnalytics = SkiaLayerAnalytics.Empty
  ) : this(skiaLayerAnalytics, autoDisposable = false) {
      panelScope.launch {
          try {
              awaitCancellation()
          } finally {
              dispose()
          }
      }
  }

@m-sasha
Copy link

m-sasha commented Jul 4, 2023

Also, we have enabled flags in a bunch of composables... We don't do 'DisabledTextField`.

@Walingar
Copy link
Collaborator Author

Walingar commented Jul 4, 2023

Honestly, choosing from these two, I would choose a boolean flag here

@Walingar Walingar force-pushed the nr/compose-panel-disposable branch from d120ba2 to 658ccaf Compare July 5, 2023 12:54
So, ComposePanel can be disposed manually or automatically on `removeNotify`
@Walingar Walingar force-pushed the nr/compose-panel-disposable branch from 658ccaf to cabbcbf Compare July 5, 2023 13:21
@Walingar Walingar merged commit 929aa7a into jb-main Jul 6, 2023
2 checks passed
@Walingar Walingar deleted the nr/compose-panel-disposable branch July 6, 2023 09:19
Walingar added a commit to Walingar/androidx that referenced this pull request Jul 11, 2023
* Allow change ComposePanel dispose behavior by `isDisposeOnRemove`

So, ComposePanel can be disposed manually or automatically on `removeNotify`
mazunin-v-jb pushed a commit that referenced this pull request Jul 11, 2023
* Allow change ComposePanel dispose behavior by `isDisposeOnRemove`

So, ComposePanel can be disposed manually or automatically on `removeNotify`
igordmn pushed a commit that referenced this pull request Dec 20, 2023
This is an imported pull request from androidx#620.

Rewrite to using providers for getting HEAD SHA and changed files
information from either MANIFEST and CHANGE_INFO files or git
if these env variables are not set.

Resolves #620
Github-Pr-Head-Sha: 75a21ad
GitOrigin-RevId: 1baae30

Test: updated tests to match the new code structure
Bug: 303481898
Change-Id: I227ed2997280d4b41ba9d38c4ed5e8224843feef
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