Reland: Migrate ReactClippingViewGroup to Kotlin (#49413)

[mateoguzmana]

Summary:

Reland of #49413 which was reverted due to an internal crash. I've attempted to do a solution to keep backwards compatibility but doesn't seem to work – keeping the original solution for now, perhaps something else can be cleaned up to avoid the breakage.

Changelog:

[INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin

Test Plan:

yarn test-android
yarn android

[alanleedev]

@mateoguzmana I don't think we should convert ReactClippingViewGroup to Kotlin with it's children like ReactViewGroup left as Java. It will result in the same issue as before.
We need to look at if converting its child classes (e.g. ReactViewGroup) together to Kotlin would be possible solution.

Looking at the code more closely, I think I found the problem.

[alanleedev]

I think there are issue with this code.

1. override var removeClippedSubviews: Boolean = false creates a new backing field for ReactHorizontalScrollContainerLegacyView.kt instead of using parent class.
   So remove the = false and add get() as it will give error without it.

2. You need to use super.removeClippedSubviews to access parent field but if you use field it is setting the value on the backing field for the current class and not the parent.

Suggested change

	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
	}
	override var removeClippedSubviews: Boolean
	get() = super.removeCLippedSubviews
	set(value) {
	// removeClippedSubviews logic may read metrics before the offsetting we do in onLayout() and
	// is such unsafe
	if (isRTL) {
	super. removeClippedSubviews = false
	} else {
	super. removeClippedSubviews = value
	}

I'll test this out and report result later.

[alanleedev]

@mateoguzmana Above change is verified to work and is not causing a crash.

I first though there might be some interop magic that was happening but Kotlin properties are just automatically handling setter/getters under the hood and there is nothing special about it.

1. So property in Kotlin interface just creates Java setXXX()/getXXX() methods.

2. If Java class has setter/getter, chlld Kotlin class can reference them using a Kotlin property but it is just calling the setXXX()/getXXX()

3. Just like in Java, if Kotlin child want to set the property of the parent, it needs to use super.XXX so it will call the setter/getter in the parent.

Things are different if the inheritance hierarchy is all Kotlin. Child can just access the property without specifying super.

(cc @cortinico )

[mateoguzmana]

Thank you so much for the thorough explanation and for checking this in-depth; it makes a lot of sense now. Sorry for the back-and-forth on this one :)

I've applied these changes in 6fd5421.

[alanleedev]

@mateoguzmana Could you also make sure Java -> Kt conversion retains the file commit history and not treated as deleting and adding a new file?

[mateoguzmana]

Could you also make sure Java -> Kt conversion retains the file commit history and not treated as deleting and adding a new file?

This is a very good point, I think we should take this into account for other PRs as well as I've noticed many of them not keeping the history once migrated. It seems to happen most of the time when using Android Studio to do an initial conversion as it stages the file automatically as a deletion.

I've done it now for this PR by renaming first using git mv and then adding the changes after moving it with git – but GitHub somehow still shows the file as deleted and added, not so sure if it's an issue on the UI only, but in my local editor I still see the history. See:

[facebook-github-bot]

@alanleedev has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[alanleedev]

I've done it now for this PR by renaming first using git mv and then adding the changes after moving it with git – but GitHub somehow still shows the file as deleted and added, not so sure if it's an issue on the UI only, but in my local editor I still see the history. See:

@mateoguzmana Thanks for checking. I think it could be an issue with GitHub so importing the PR as is.
I think next time you can try to do the rename as a single commit and add additional changes as a separate commit and it may show up properly in GitHub.

[facebook-github-bot]

@alanleedev merged this pull request in ac57ec4.

[react-native-bot]

This pull request was successfully merged by @mateoguzmana in ac57ec4

^{When will my fix make it into a release? | How to file a pick request?}

[alanleedev]

@mateoguzmana BTW, I had to amend the PR before merging to keep the history.

[mateoguzmana]

@mateoguzmana BTW, I had to amend the PR before merging to keep the history.

@alanleedev did you do anything in particular? (just for my knowledge)

When I pull the latest from the main branch and check the history of the file (with either code editor or using git commands) I still see the file as a full addition and the renaming is not in the history – I'm afraid it didn't fully work.

––

When writing this comment I got curious and dug deeper into how this works with git. Seems like there is a "similarity score" and that's how git detects whether to consider a file as an addition/deletion or to keep it as "renamed with changes". There are some interesting posts about this online, quite interesting:

• Detect renames: https://git-scm.com/docs/git-diff/2.12.5#Documentation/git-diff.txt--Mltngt
• How does git detect similar files for its rename detection?: https://stackoverflow.com/a/7941544/5415299

[alanleedev]

@alanleedev did you do anything in particular? (just for my knowledge)

@mateoguzmana I had to run some Sapling command to make sure the history was kept in internal Meta codebase using Sapling (fork of Mercurial). I guess things may look different in GitHub.

@cortinico Do you know the best practice here for OSS?