Skip to content

Clarify usage of compose.swing.render.on.graphics flag #2071

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

Open
wants to merge 4 commits into
base: jb-main
Choose a base branch
from

Conversation

MatkovIvan
Copy link
Member

More clear KDoc + warning based on misusage and questions from community

Release Notes

N/A

@MatkovIvan MatkovIvan requested review from m-sasha and igordmn May 5, 2025 10:05
@m-sasha m-sasha changed the title Clarify usage of ompose.swing.render.on.graphics flag Clarify usage of compose.swing.render.on.graphics flag May 5, 2025
Comment on lines +105 to +108
if (ComposeFeatureFlags.useSwingGraphics) {
println("Usage of \"compose.swing.render.on.graphics\" affect only behaviour of ComposePanel." +
"Usage together with ComposeWindow or ComposeDialog won't have any effect.")
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about printing this. The flag is for the entire process, but you can have both ComposePanel and ComposeWindow in a single app...

Copy link
Member Author

Choose a reason for hiding this comment

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

True. The intention here is to notify already wrong configured applications. Ideas are welcome

Copy link
Member

Choose a reason for hiding this comment

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

Why is this a global flag, though? Can we not make it an argument to ComposePanel instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Already thought about it, so yes we can. But I don't really won't to remove global flag because despite "experimental" status it's widely adopted

Copy link
Member Author

@MatkovIvan MatkovIvan May 5, 2025

Choose a reason for hiding this comment

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

Why is this a global flag, though?

It was introduced in #601, so for now it's "historically" I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Exposed as another optional/experimental parameter in ComposePanel's constructor

Copy link
Member

@m-sasha m-sasha May 5, 2025

Choose a reason for hiding this comment

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

Great!

As for this print, I think I personally would prefer not to pollute the output, but I'd leave the decision to @igordmn .
If we do add it, probably better to print to System.err, though (we're in desktopMain here).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is valid to open a ComposeWindow inside IntelliJ that can use a global useSwingGraphics for ComposePanel's. Because it is valid, we shouldn't print anything. A better name, something like composePanelUseSwingGraphics, wouldn't cause the issue.

As for a flag, I prefer an argument over it. For historical reasons we can either keep it, or deprecate it.

* @param renderSettings Configuration class for rendering settings.
*/
class ComposePanel @ExperimentalComposeUiApi constructor(
private val skiaLayerAnalytics: SkiaLayerAnalytics,
@property:ExperimentalComposeUiApi
private val useSwingGraphics: Boolean = ComposeFeatureFlags.useSwingGraphics,
Copy link
Member

@m-sasha m-sasha May 5, 2025

Choose a reason for hiding this comment

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

Maybe it should go into RenderSettings below?
Regardless, the documentation of RenderSettings needs updating.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably no.

  1. It will be a parameter not only for ComposePanel that I'm trying to avoid in this PR
  2. I see a place where it's used only if useSwingGraphics == false

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we look at the API from user perspective, it might be better to have these parameters under 1 thing, just from ComposePanel user perspective.

The points are valid, but we can fix them by creating something like:

sealed class ComposePanelRenderSettings {
  class LightweightLayer: ComposePanelRenderSettings()
  class HeavyweightLayer(val renderSettings: RenderSettings): ComposePanelRenderSettings 
}

Lightweight, heavyweight - Swing terms.

I think it is better to figure out the shape now rather than trying to avoid big changes. Because we will stuck with it forever, and it looks not consistent to have 2 params that affect rendering separately.

Comment on lines +105 to +108
if (ComposeFeatureFlags.useSwingGraphics) {
println("Usage of \"compose.swing.render.on.graphics\" affect only behaviour of ComposePanel." +
"Usage together with ComposeWindow or ComposeDialog won't have any effect.")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is valid to open a ComposeWindow inside IntelliJ that can use a global useSwingGraphics for ComposePanel's. Because it is valid, we shouldn't print anything. A better name, something like composePanelUseSwingGraphics, wouldn't cause the issue.

As for a flag, I prefer an argument over it. For historical reasons we can either keep it, or deprecate it.

* @param renderSettings Configuration class for rendering settings.
*/
class ComposePanel @ExperimentalComposeUiApi constructor(
private val skiaLayerAnalytics: SkiaLayerAnalytics,
@property:ExperimentalComposeUiApi
private val useSwingGraphics: Boolean = ComposeFeatureFlags.useSwingGraphics,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we look at the API from user perspective, it might be better to have these parameters under 1 thing, just from ComposePanel user perspective.

The points are valid, but we can fix them by creating something like:

sealed class ComposePanelRenderSettings {
  class LightweightLayer: ComposePanelRenderSettings()
  class HeavyweightLayer(val renderSettings: RenderSettings): ComposePanelRenderSettings 
}

Lightweight, heavyweight - Swing terms.

I think it is better to figure out the shape now rather than trying to avoid big changes. Because we will stuck with it forever, and it looks not consistent to have 2 params that affect rendering separately.

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.

3 participants