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 constructor with RenderSettings to ComposePanel #1377

Merged
merged 15 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion compose/ui/ui/api/desktop/ui.api
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public final class androidx/compose/ui/awt/ComposeDialog : javax/swing/JDialog {
public final class androidx/compose/ui/awt/ComposePanel : javax/swing/JLayeredPane {
public static final field $stable I
public fun <init> ()V
public fun <init> (Lorg/jetbrains/skiko/SkiaLayerAnalytics;)V
public fun <init> (Lorg/jetbrains/skiko/SkiaLayerAnalytics;Landroidx/compose/ui/awt/RenderingSettings;)V
public fun add (Ljava/awt/Component;)Ljava/awt/Component;
public fun addFocusListener (Ljava/awt/event/FocusListener;)V
public fun addNotify ()V
Expand Down Expand Up @@ -528,6 +528,17 @@ public final class androidx/compose/ui/awt/ComposeWindow : javax/swing/JFrame {
public fun setUndecorated (Z)V
}

public final class androidx/compose/ui/awt/RenderingSettings {
public static final field $stable I
public static final field Companion Landroidx/compose/ui/awt/RenderingSettings$Companion;
public fun <init> (Ljava/lang/Boolean;)V
public final fun isVsyncEnabled ()Ljava/lang/Boolean;
}

public final class androidx/compose/ui/awt/RenderingSettings$Companion {
public final fun getDefault ()Landroidx/compose/ui/awt/RenderingSettings;
}

public final class androidx/compose/ui/awt/SwingPanel_desktopKt {
public static final fun SwingPanel-euL9pac (JLkotlin/jvm/functions/Function0;Landroidx/compose/ui/Modifier;Lkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)V
public static final fun getNoOpUpdate ()Lkotlin/jvm/functions/Function1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@ import org.jetbrains.skiko.SkiaLayerAnalytics
* @param skiaLayerAnalytics Analytics that helps to know more about SkiaLayer behaviour.
* SkiaLayer is underlying class used internally to draw Compose content.
* Implementation usually uses third-party solution to send info to some centralized analytics gatherer.
* @param renderingSettings Configuration class for rendering settings.
*/
class ComposePanel @ExperimentalComposeUiApi constructor(
private val skiaLayerAnalytics: SkiaLayerAnalytics,
private val renderingSettings: RenderingSettings
elijah-semyonov marked this conversation as resolved.
Show resolved Hide resolved
) : JLayeredPane() {
constructor() : this(SkiaLayerAnalytics.Empty)
constructor() : this(SkiaLayerAnalytics.Empty, RenderingSettings.Default)

init {
check(isEventDispatchThread()) {
Expand Down Expand Up @@ -196,7 +198,8 @@ class ComposePanel @ExperimentalComposeUiApi constructor(
return ComposeContainer(
container = this,
skiaLayerAnalytics = skiaLayerAnalytics,
windowContainer = windowContainer
windowContainer = windowContainer,
renderingSettings = renderingSettings,
).apply {
focusManager.releaseFocus()
setBounds(0, 0, width, height)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ internal class ComposeWindowPanel(
// but it's always disabled here. Using fallback instead of [check] to support
// opening separate windows from [ComposePanel] with such layer type.
if (it == LayerType.OnComponent) LayerType.OnSameCanvas else it
}
},
// TODO: Add RenderingSettings to ComposeWindowPanel constructor
renderingSettings = RenderingSettings.Default
)
private val composeContainer
get() = requireNotNull(_composeContainer) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2024 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.compose.ui.awt

import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.ComposeFeatureFlags
import org.jetbrains.skiko.SkikoProperties

/**
* Configuration class for rendering settings.
*
* @property isVsyncEnabled Indicates whether presentation vsync is enabled or not
* When true, the internal implementation will attempt to synchronize drawable presentations
* with vsync. It guarantees that the frame is presented without visual artifacts like tearing in
* exchange for a latency increase.
* When false, the internal implementation will not attempt to synchronize drawable presentations
* it can reduce latency but may introduce visual artifacts like screen tearing.
* When null, the internal implementation will use the global configuration from [SkikoProperties.vsyncEnabled]
*
* This flag has no effect if [ComposeFeatureFlags.useSwingGraphics] is true. In this case Compose
* will render the content to Swing provided offscreen buffer and the presentation will be controlled
* by Swing.
*/
@ExperimentalComposeUiApi
class RenderingSettings(
Copy link
Member

Choose a reason for hiding this comment

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

Should we add equals/hashCode to it?

Copy link
Author

Choose a reason for hiding this comment

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

Nop, we don't do any comparisons/caching/using it as a key, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe RenderSettings instead? A bit shorter and doesn't lose any meaning.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, sounds fine

val isVsyncEnabled: Boolean?
Copy link
Member

Choose a reason for hiding this comment

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

It's better/cleaner to put this in a (fake) enum, e.g. VSyncMode.Enabled, VSyncMode.Disabled, and VsyncMode.Default. Although maybe it's overkill here; not sure.

Copy link
Author

Choose a reason for hiding this comment

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

I'd leave it as it is now (ExperimentalAPI after all), I haven't yet performed an investigation about different presentation modes on non-Metal rendering backends.

) {
companion object {
/**
* Default rendering settings
*/
val Default = RenderingSettings(
isVsyncEnabled = null
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import androidx.compose.ui.LayerType
import androidx.compose.ui.awt.AwtEventFilter
import androidx.compose.ui.awt.AwtEventListener
import androidx.compose.ui.awt.AwtEventListeners
import androidx.compose.ui.awt.RenderingSettings
import androidx.compose.ui.input.key.KeyEvent
import androidx.compose.ui.platform.LocalLifecycleOwner
import androidx.compose.ui.platform.LocalInternalViewModelStoreOwner
Expand Down Expand Up @@ -78,6 +79,7 @@ import org.jetbrains.skiko.SkiaLayerAnalytics
* for window coordinate space.
* @property useSwingGraphics Flag indicating if offscreen rendering to Swing graphics is used.
* @property layerType The type of layer used for Popup/Dialog.
* @property renderingSettings The settings for rendering.
Copy link
Member

Choose a reason for hiding this comment

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

Add a note that it will work only with useSwingGraphics == true

Copy link
Author

@elijah-semyonov elijah-semyonov Jun 12, 2024

Choose a reason for hiding this comment

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

It's in the doc of RenderSettings.

Copy link
Author

Choose a reason for hiding this comment

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

In general it's not true, because we can add settings, that will work with swing

*/
internal class ComposeContainer(
val container: JLayeredPane,
Expand All @@ -88,6 +90,7 @@ internal class ComposeContainer(

private val useSwingGraphics: Boolean = ComposeFeatureFlags.useSwingGraphics,
private val layerType: LayerType = ComposeFeatureFlags.layerType,
private val renderingSettings: RenderingSettings,
) : WindowFocusListener, WindowListener, LifecycleOwner, ViewModelStoreOwner {
val windowContext = PlatformWindowContext()
var window: Window? = null
Expand Down Expand Up @@ -338,7 +341,13 @@ internal class ComposeContainer(
return if (useSwingGraphics) {
SwingSkiaLayerComponent(mediator, renderDelegate, skiaLayerAnalytics)
} else {
WindowSkiaLayerComponent(mediator, windowContext, renderDelegate, skiaLayerAnalytics)
WindowSkiaLayerComponent(
mediator,
Copy link
Member

Choose a reason for hiding this comment

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

Add explicit parameter names if it's not one-liner

Copy link
Author

Choose a reason for hiding this comment

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

Do we add them when variables match parameter names or are obvious?

Copy link
Member

Choose a reason for hiding this comment

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

It's better to add it to be on the safe side during possible refactoring

windowContext,
renderDelegate,
skiaLayerAnalytics,
renderingSettings
)
}
}

Expand Down Expand Up @@ -381,7 +390,8 @@ internal class ComposeContainer(
density = density,
layoutDirection = layoutDirection,
focusable = focusable,
compositionContext = compositionContext
compositionContext = compositionContext,
renderingSettings = renderingSettings
)
LayerType.OnComponent -> SwingComposeSceneLayer(
composeContainer = this,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package androidx.compose.ui.scene

import org.jetbrains.skia.Rect as SkRect
import androidx.compose.runtime.CompositionContext
import androidx.compose.ui.awt.RenderingSettings
import androidx.compose.ui.awt.getTransparentWindowBackground
import androidx.compose.ui.awt.setTransparent
import androidx.compose.ui.awt.toAwtRectangle
Expand Down Expand Up @@ -52,7 +53,8 @@ internal class WindowComposeSceneLayer(
density: Density,
layoutDirection: LayoutDirection,
focusable: Boolean,
compositionContext: CompositionContext
compositionContext: CompositionContext,
private val renderingSettings: RenderingSettings
) : DesktopComposeSceneLayer(composeContainer, density, layoutDirection) {
private val window get() = requireNotNull(composeContainer.window)
private val windowContext = PlatformWindowContext().also {
Expand Down Expand Up @@ -193,7 +195,8 @@ internal class WindowComposeSceneLayer(
mediator = mediator,
windowContext = windowContext,
renderDelegate = renderDelegate,
skiaLayerAnalytics = skiaLayerAnalytics
skiaLayerAnalytics = skiaLayerAnalytics,
renderingSettings = renderingSettings,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package androidx.compose.ui.scene.skia

import androidx.compose.ui.awt.RenderingSettings
import androidx.compose.ui.platform.PlatformWindowContext
import androidx.compose.ui.scene.ComposeSceneMediator
import java.awt.Dimension
Expand All @@ -24,6 +25,7 @@ import javax.accessibility.Accessible
import org.jetbrains.skiko.GraphicsApi
import org.jetbrains.skiko.SkiaLayer
import org.jetbrains.skiko.SkiaLayerAnalytics
import org.jetbrains.skiko.SkiaLayerProperties
import org.jetbrains.skiko.SkikoRenderDelegate

/**
Expand All @@ -36,7 +38,8 @@ internal class WindowSkiaLayerComponent(
private val mediator: ComposeSceneMediator,
private val windowContext: PlatformWindowContext,
renderDelegate: SkikoRenderDelegate,
skiaLayerAnalytics: SkiaLayerAnalytics
skiaLayerAnalytics: SkiaLayerAnalytics,
private val renderingSettings: RenderingSettings,
) : SkiaLayerComponent {
/**
* See also backend layer for swing interop in [SwingSkiaLayerComponent]
Expand All @@ -47,6 +50,13 @@ internal class WindowSkiaLayerComponent(
// apply `checkNotNull` for "non-null" field.
checkNotNull(mediator.accessible)
},
properties = run {
val defaultProperties = SkiaLayerProperties()

SkiaLayerProperties(
isVsyncEnabled = renderingSettings.isVsyncEnabled ?: defaultProperties.isVsyncEnabled,
)
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
properties = run {
val defaultProperties = SkiaLayerProperties()
SkiaLayerProperties(
isVsyncEnabled = renderingSettings.isVsyncEnabled ?: defaultProperties.isVsyncEnabled,
)
},
properties = SkiaLayerProperties(
isVsyncEnabled = renderingSettings.isVsyncEnabled ?: SkiaLayerProperties.Default.isVsyncEnabled,
),

Copy link
Author

Choose a reason for hiding this comment

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

There can be multiple properties in future, this approach will reconstruct SkiaLayerProperties every single time. There is no Default in SkiaLayerProperties.Companion

analytics = skiaLayerAnalytics
) {
override fun paint(g: Graphics) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package androidx.compose.ui.window

import androidx.compose.ui.assertThat
import androidx.compose.ui.awt.RenderingSettings
import androidx.compose.ui.isEqualTo
import androidx.compose.ui.scene.ComposeContainer
import androidx.lifecycle.Lifecycle
Expand Down Expand Up @@ -142,7 +143,8 @@ class ComposeContainerLifecycleOwnerTest {
container = ComposeContainer(
container = pane,
skiaLayerAnalytics = SkiaLayerAnalytics.Empty,
window = window
window = window,
renderingSettings = RenderingSettings.Default
).also {
it.lifecycle.addObserver(allEvents)
}
Expand Down Expand Up @@ -172,7 +174,8 @@ class ComposeContainerLifecycleOwnerTest {
container = ComposeContainer(
container = pane,
skiaLayerAnalytics = SkiaLayerAnalytics.Empty,
window = window
window = window,
renderingSettings = RenderingSettings.Default
).also {
it.lifecycle.addObserver(allEvents)
}
Expand Down Expand Up @@ -202,7 +205,8 @@ class ComposeContainerLifecycleOwnerTest {
container = ComposeContainer(
container = pane,
skiaLayerAnalytics = SkiaLayerAnalytics.Empty,
window = window
window = window,
renderingSettings = RenderingSettings.Default
).also {
it.lifecycle.addObserver(allEvents)
}
Expand Down Expand Up @@ -250,7 +254,8 @@ class ComposeContainerLifecycleOwnerTest {
container = ComposeContainer(
container = this,
skiaLayerAnalytics = SkiaLayerAnalytics.Empty,
window = window
window = window,
renderingSettings = RenderingSettings.Default
)
container.lifecycle.addObserver(observer)
window.add(this)
Expand Down
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ moshi = "1.13.0"
protobuf = "3.21.8"
paparazzi = "1.0.0"
paparazziNative = "2022.1.1-canary-f5f9f71"
skiko = "0.8.4"
skiko = "0.8.7"
sqldelight = "1.3.0"
retrofit = "2.7.2"
wire = "4.5.1"
Expand Down
Loading