Declare and implement a new v2.ScrollbarAdapter #368

m-sasha · 2023-01-11T09:37:52Z

... in order to distinguish between scrollbar (track) size and scrollable viewport size.

Proposed Changes

Add a new ScrollbarAdapter interface in androidx.compose.foundation.v2
The new interface will provide contentSize and viewportSize. maxScrollOffset will be implemented as an extension function.
Add ScrollState.viewportSize in order to support the new ScrollbarAdapter implementation for a regular scrollable container.
Implement the new ScrollbarAdapter for ScrollState and for LazyListState.
~~Copy all public functions in androidx.compose.foundation.Scrollbar to the new androidx.compose.foundation.v2.Scrollbar and mark all the old ones as deprecated.~~
Provide full binary backwards compatibility using @JvmName and partial source backwards compatibility by naming the new functions the same as the old ones. The only exception is the scrollbar adapter interface itself, for which the users will need to change the import from androidx.compose.foundation to androidx.compose.foundation.v2

Testing

Test: Manually and with new unit tests.

Issues Fixed

Fixes: JetBrains/compose-multiplatform#2605

m-sasha · 2023-01-11T14:46:28Z

I will probably also add tests (or a way to test) whether implementations of the old ScrollbarAdapter interface still work correctly.

igordmn · 2023-01-11T21:59:47Z

...oundation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/Scrollbar.desktop.kt

@@ -128,9 +127,10 @@ fun defaultScrollbarStyle() = ScrollbarStyle(
 * @param interactionSource [MutableInteractionSource] that will be used to dispatch
 * [DragInteraction.Start] when this Scrollbar is being dragged.
 */
+@Deprecated("Use androidx.compose.foundation.v2.VerticalScrollbar instead")


It is better to add replaceWith, so IDEA can replace automatically.

I tried, but it doesn't work well. If I use

replaceWith = ReplaceWith( expression = "androidx.compose.foundation.v2.VerticalScrollbar" )

or

replaceWith = ReplaceWith( expression = "androidx.compose.foundation.v2.VerticalScrollbar(adapter, modifier, reverseLayout, style, interactionSource)" )

it breaks the code completely.

If I use

replaceWith = ReplaceWith( expression = "VerticalScrollbar(adapter, modifier, reverseLayout, style, interactionSource)", "androidx.compose.foundation.v2.VerticalScrollbar" )

It updates the code "correctly" (only re-arranging the order of arguments), but it doesn't even add (much less replace) the import.

I have an idea. Nevertheless what we decide to do with JvmName approach - we don't create androidx.compose.foundation.v2.VerticalScrollbar, we just add overload androidx.compose.foundation.VerticalScrollbar(androidx.compose.foundation.v2.ScrollbarAdapter)

In this case, users can just replace rememberScrollbarAdapter

Also, I we don't add JvmName's, I would describe the proper migration guide in rememberScrollbarAdapter message: Edit -> Find -> Replace in file -> replace "androidx.compose.foundation.rememberScrollbarAdapter" to "androidx.compose.foundation.v2.rememberScrollbarAdapter"

Check the latest code.

compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Scroll.kt

igordmn · 2023-01-11T22:05:18Z

...oundation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/Scrollbar.desktop.kt

@@ -174,9 +174,10 @@ fun VerticalScrollbar(
 * @param interactionSource [MutableInteractionSource] that will be used to dispatch
 * [DragInteraction.Start] when this Scrollbar is being dragged.
 */
+@Deprecated("Use androidx.compose.foundation.v2.HorizontalScrollbar instead")


Please, replace deprecated API in our internal example (example1/Main.jvm.kt)

No need, after the changes to VerticalScrollbar and rememberScrollbarAdapter.

igordmn · 2023-01-11T22:32:32Z

...dation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/v2/Scrollbar.desktop.kt

+ * limitations under the License.
+ */
+
+package androidx.compose.foundation.v2


I have an idea. We can use JvmName to not force users to change their code.

We can keep the old names and package, and still preserve the backward compatibility on binary level:

package androidx.compose.foundation @JvmName("ScrollbarAdapter") @Deprecated interface ScrollbarAdapter1 @JvmName("ScrollbarAdapter2") interface ScrollbarAdapter

We lose source compatibility though, but only in few cases, when users use custom implementation of ScrollbarAdapter. After they update version of Compose, they have to change interface to ScrollbarAdapter1, or provide viewportSize somehow. But it is better than forcing the majority of users who don't use custom adapters to replace deprecated calls.

Good idea but unfortunately JvmName doesn't currently work for interfaces: https://youtrack.jetbrains.com/issue/KT-31420/Support-JvmName-on-interface-or-provide-other-interface-evolution-mechanism

Can we use it for just rememberScrollbarAdapter/VerticalScrollbar, and create only v2.ScrollbarAdapter?

igordmn · 2023-01-11T22:56:32Z

...se/foundation/foundation/src/desktopTest/kotlin/androidx/compose/foundation/ScrollbarTest.kt

@@ -650,11 +695,12 @@ class ScrollbarTest {
                }
            }

+            @Suppress("DEPRECATION")


I will probably also add tests (or a way to test) whether implementations of the old ScrollbarAdapter interface still work correctly.

It seems we still test the old functions here? How does thumb size on scrollbar smaller than viewport pass?

It's actually using the new implementation behind the scenes.

ScrollbarAdapter(state) returns NewAdapterAsOld(NewScrollbarAdapter(scrollState)) and then when you pass it to Scrollbar, it just unwraps the new adapter:

private fun ScrollbarAdapter.asNewAdapter(trackSize: Int) = if (this is NewAdapterAsOld) this.newAdapter // Just unwrap else OldAdapterAsNew(this, trackSize)

So existing code will automatically use the new implementation, and will enjoy the fix (unless it actually implements the old ScrollbarAdapter itself).

It also means that I need to come up with a good way to test whether the old adapter really works when it's wrapped into OldAdapterAsNew.

Nice trick :).

Let's change code to the new API though, it is always better to test what users will use directly.

Now testing both new and old code.

…ish between scrollbar (track) size and scrollable viewport size.

… than Float, to allow for very large content.

…alues do not necessarily represent pixels.

…rollbarAdapter and ScrollbarAdapter creator functions.

… adapters.

…ed to AOSP.

m-sasha · 2023-01-16T14:01:08Z

Added testing of "old" ScrollbarAdapter implementations with the new code.

The only remaining untested code is NewScrollbarAdapterAsOld (not unwrapped), but

It's very unlikely to be used anyway. Basically only by someone implementing their own scrollbar via our adapter.
To test it we'd need to copy the entire old scrollbar implementation to the test.

…wportSize after it's merged upstream.

igordmn · 2023-01-18T12:06:52Z

compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Scroll.kt

+    /**
+     * Size of the viewport on the scrollable axis, or -1 if still unknown.
+     */
+    internal var viewportSize: Int by mutableStateOf(-1, structuralEqualityPolicy())


0 is more appropriate here, it won't mess with calculations that we can perform before the layout stage. We don't perform such calculations though (and shouldn't), so it is just a minor issue. If we would make it public, then it definitely should be not -1, it always hard to reason about magic constants.

I started with 0 at first, but then I thought that -1 would be better as it lets the developer know that it's not a known value yet. 0 is theoretically a legal value for the viewport size, so he wouldn't be able to tell between "unknown" and "known and is 0".

Actually, I noticed that I wasn't checking it correctly for -1, so I should.

Changed to 0, as the Googler(s) also preferred it.

…barAdapter (#368) Declare and implement a new, v2.ScrollbarAdapter in order to distinguish between scrollbar (track) size and scrollable viewport size.

m-sasha requested a review from igordmn January 11, 2023 09:38

m-sasha changed the title ~~Declare and implement a new v2.ScrollbarAdapter (variant 1)~~ Declare and implement a new v2.ScrollbarAdapter Jan 11, 2023

igordmn reviewed Jan 11, 2023

View reviewed changes

eymar force-pushed the jb-main branch from b519427 to 38063eb Compare January 13, 2023 11:55

m-sasha added 4 commits January 14, 2023 12:34

Declare and implement a new, v2.ScrollbarAdapter in order to distingu…

200a084

…ish between scrollbar (track) size and scrollable viewport size.

Change the properties of ScrollbarAdapter to be of type Double rather…

12c58ee

… than Float, to allow for very large content.

Made ScrollState.viewportSize internal until it is merged upstream.

856462f

Fixed documentation of the new ScrollbarAdapter to explain that the v…

19783c2

…alues do not necessarily represent pixels.

m-sasha force-pushed the bugfix/new-scrollbar-adapter-v1 branch from 571728b to 19783c2 Compare January 14, 2023 10:35

m-sasha and others added 3 commits January 16, 2023 15:32

Use @JvmName to provide binary backwards compatibility for rememberSc…

eda1176

…rollbarAdapter and ScrollbarAdapter creator functions.

Test the new scrollbar implementation with both old and new scrollbar…

ae6c044

… adapters.

Align the addition of ScrollState.viewportSize with the change propos…

04f00f3

…ed to AOSP.

Improve precision of scrollbar thumb position and size.

4bae67b

m-sasha requested a review from igordmn January 17, 2023 10:51

m-sasha added 2 commits January 17, 2023 21:50

Minor documentation fixes.

7aa39ac

Removed comment about changes that need to be made to ScrollState.vie…

2b23eac

…wportSize after it's merged upstream.

igordmn approved these changes Jan 18, 2023

View reviewed changes

Changed default ScrollState.viewportSize value to 0.

14f6b67

m-sasha merged commit 8c9299b into jb-main Jan 18, 2023

m-sasha deleted the bugfix/new-scrollbar-adapter-v1 branch January 18, 2023 14:29

m-sasha mentioned this pull request Jan 18, 2023

Height of vertical scrollbar is not accurately calculated when attached to LazyColumn JetBrains/compose-multiplatform#1211

Closed

eygraber mentioned this pull request Jan 18, 2023

Kotlin 1.8 Canvas JS Error - Layout state is not idle before measure starts JetBrains/compose-multiplatform#2646

Closed

m-sasha mentioned this pull request Apr 12, 2023

Add 1.4.0 to changelog JetBrains/compose-multiplatform#3036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare and implement a new v2.ScrollbarAdapter #368

Declare and implement a new v2.ScrollbarAdapter #368

m-sasha commented Jan 11, 2023 •

edited

Loading

m-sasha commented Jan 11, 2023

igordmn Jan 11, 2023

m-sasha Jan 12, 2023

igordmn Jan 13, 2023

igordmn Jan 13, 2023

m-sasha Jan 16, 2023

igordmn Jan 11, 2023

m-sasha Jan 12, 2023

m-sasha Jan 16, 2023

igordmn Jan 11, 2023

m-sasha Jan 12, 2023

igordmn Jan 12, 2023

m-sasha Jan 16, 2023

igordmn Jan 11, 2023

m-sasha Jan 12, 2023

m-sasha Jan 12, 2023

igordmn Jan 12, 2023

m-sasha Jan 16, 2023

m-sasha commented Jan 16, 2023

igordmn Jan 18, 2023

m-sasha Jan 18, 2023

m-sasha Jan 18, 2023

m-sasha Jan 18, 2023

Declare and implement a new v2.ScrollbarAdapter #368

Declare and implement a new v2.ScrollbarAdapter #368

Conversation

m-sasha commented Jan 11, 2023 • edited Loading

Proposed Changes

Testing

Issues Fixed

m-sasha commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-sasha commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-sasha commented Jan 11, 2023 •

edited

Loading