-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Migrate ReactClippingViewGroup to Kotlin
#49413
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| package com.facebook.react.uimanager | ||
|
|
||
| import android.graphics.Rect | ||
| import android.view.View | ||
|
|
||
| /** | ||
| * Interface that should be implemented by [View] subclasses that support `removeClippedSubviews` property. When this property is set for the [ViewGroup] subclass | ||
| * it's responsible for detaching it's child views that are clipped by the view boundaries. Those | ||
| * view boundaries should be determined based on it's parent clipping area and current view's offset | ||
| * in parent and doesn't necessarily reflect the view visible area (in a sense of a value that | ||
| * [View.getGlobalVisibleRect] may return). In order to determine the clipping rect for | ||
| * current view helper method [ReactClippingViewGroupHelper.calculateClippingRect] can be used | ||
| * that takes into account parent view settings. | ||
| */ | ||
| public interface ReactClippingViewGroup { | ||
| /** | ||
| * Notify view that clipping area may have changed and it should recalculate the list of children | ||
| * that should be attached/detached. This method should be called only when property `removeClippedSubviews` is set to `true` on a view. | ||
| * | ||
| * CAUTION: Views are responsible for calling [updateClippingRect] on it's children. | ||
| * This should happen if child implement [ReactClippingViewGroup], return true from [getRemoveClippedSubviews] and clipping rect change of the current view may affect clipping | ||
| * rect of this child. | ||
| */ | ||
| public fun updateClippingRect() | ||
|
|
||
| /** | ||
| * Get rectangular bounds to which view is currently clipped to. Called only on views that has set | ||
| * `removeCLippedSubviews` property value to `true`. | ||
| * | ||
| * @param outClippingRect output clipping rect should be written to this object. | ||
| */ | ||
| public fun getClippingRect(outClippingRect: Rect) | ||
|
|
||
| /** | ||
| * Sets property `removeClippedSubviews` as a result of property update in JS. Should be | ||
| * called only from [ViewManager.updateView] method. | ||
| * | ||
| * Helper method [ReactClippingViewGroupHelper.applyRemoveClippedSubviewsProperty] may be | ||
| * used by [ViewManager] subclass to apply this property based on property update map [ReactStylesDiffMap]. | ||
| */ | ||
| public var removeClippedSubviews: Boolean | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,17 +19,17 @@ internal class ReactHorizontalScrollContainerLegacyView(context: Context) : | |
| ReactViewGroup(context) { | ||
| private val isRTL: Boolean = I18nUtil.instance.isRTL(context) | ||
|
|
||
| override fun setRemoveClippedSubviews(removeClippedSubviews: Boolean) { | ||
| // removeClippedSubviews logic may read metrics before the offsetting we do in onLayout() and is | ||
| // such unsafe | ||
| if (isRTL) { | ||
| super.setRemoveClippedSubviews(false) | ||
| return | ||
| override var removeClippedSubviews: Boolean = false | ||
| set(value) { | ||
| // removeClippedSubviews logic may read metrics before the offsetting we do in onLayout() and | ||
| // is such unsafe | ||
| if (isRTL) { | ||
| field = false | ||
| } else { | ||
| field = value | ||
| } | ||
| } | ||
|
|
||
| super.setRemoveClippedSubviews(removeClippedSubviews) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[update] see comments in PR #49607
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah this is bad :| |
||
| } | ||
|
|
||
| protected override fun onLayout(changed: Boolean, left: Int, top: Int, right: Int, bottom: Int) { | ||
| if (isRTL) { | ||
| // When the layout direction is RTL, we expect Yoga to give us a layout | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably an indicator of a breaking change. We should check how often
ReactClippingViewGroupis implemented in OSS. If the number is relevant this will have to be marked as [BREAKING] rather than internal.If there are no relevant usages on the other hand, we can make this class [INTERNAL]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some usages in OSS (ref), so it would be a breaking change indeed.
Now, my memory started getting back to why I added the backwards compatibility annotations: seems like we can prevent this from being a breaking change by adding the following (tested by reverting the changes in ReactHorizontalScrollContainerLegacyView):
But in the other hand, I don't see any other cases where we have done this in the codebase. Do we prefer to have it as a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to look only at Kotlin usages here
and here
I believe the latter as just forks, so we can consider this actually [INTERNAL]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, understood. Seems good then, thank you for double checking!