refact(Android): allow for different fragment types inside ScreenContainer #1887

kkafar · 2023-09-13T15:27:33Z

Description

This PR is laying out ground work under not-so-distant Android modal & bottom sheet implementation.

Currently ScreenContainer<T: ScreenFragment>: ViewGroup (and thus ScreenStack: ScreenContainer<ScreenStackFragment>) operates on ScreenFragment: Fragment objects, making it impossible to use any other Fragment subclass with the stack (only single inheritance is possible on JVM).

This PR changes the situation by introducing ScreenFragmentWrapper interface which both:

holds the reference to underlaying Fragment
provides additional API previously directly attached to the ScreenFragment

Also introduced ScreenStackFragmentWrapper extends ScreenFragmentWrapper with API previously attached to ScreenStackFragment.

Thereafter by implementing such interface by the fragment subclass itself we can have both of worlds:

Customized fragment behaviour
Implementation of all methods required for fragment<->container communication.

Disclaimer: I have decided to split "Android modals PR" into few smaller ones to reduce its size and complexity. I won't use "stack PR" technique to spare myself whole bunch of rebasing & conflict resolving.

Changes

ScreenContainer is no longer parameterized with T: ScreenFragment. It uses ScreenFragmentWrapper as screen-primitve base type now.
Our fragment types ScreenFragment & ScreenStackFragment now implement ScreenFragmentWrapper & ScreenStackFragmentWrapper interfaces respectively.
ScreenContainer has access to the fragments it operates on (fragment API) via ScreenFragmentWrapper.fragment property.
Our custom methods previously implemented directly on Fragment subclass are exposed via the new interfaces so ScreenContainer still has access to them.

Test code and steps to reproduce

See that test examples build & CI passes.

I did some manual testing in our example apps & didn't notice any regression.

Checklist

Ensured that CI passes

…tWrapper

…ment

…d of being parameterized with T: ScreenFragment

…ent interface shape)

kkafar

Few remarks for the future & reviewers

kkafar · 2023-09-13T16:18:02Z

android/src/main/java/com/swmansion/rnscreens/FragmentHolder.kt

+package com.swmansion.rnscreens
+
+import androidx.fragment.app.Fragment
+
+interface FragmentHolder {
+    val fragment: Fragment
+}


Decided to split this property to separate interface as this is the key idea behind this PR.

Also in the future I plan to make ScreenFragmentWrapper interfaces much more granular (split those into number of fine-grained interfaces.

kkafar · 2023-09-13T16:19:45Z

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.kt

@@ -16,9 +16,9 @@ import com.facebook.react.modules.core.ChoreographerCompat
 import com.facebook.react.modules.core.ReactChoreographer
 import com.swmansion.rnscreens.Screen.ActivityState

-open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(context) {
+open class ScreenContainer(context: Context?) : ViewGroup(context) {


ScreenContainer type is no longer parameterized with ScreenFragment type. Instead whole fragment type variance is handled by ScreenFragmentWrapper interface.

kkafar · 2023-09-13T16:23:33Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.kt

+open class ScreenFragment : Fragment, ScreenFragmentWrapper {
    enum class ScreenLifecycleEvent {
        Appear, WillAppear, Disappear, WillDisappear
    }

+    override val fragment: Fragment
+        get() = this
+


Fragment subclass itself implements the new interface.

Thanks to fragment property:

override val fragment: Fragment get() = this

we can refer to this specific Fragment instance inside ScreenContainer.

Please note also that we are not creating any strong-reference cycle here as this property is computed on-the-fly.

WoLewicki

I added some comments, other than them, LGTM 🚀

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.kt

android/src/main/java/com/swmansion/rnscreens/ext/ViewExt.kt

This reverts commit 2f8cbc8.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-native-screens](https://togithub.com/software-mansion/react-native-screens) | [`^3.25.0` -> `^3.26.0`](https://renovatebot.com/diffs/npm/react-native-screens/3.25.0/3.26.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-native-screens/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-native-screens/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-native-screens/3.25.0/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-native-screens/3.25.0/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>software-mansion/react-native-screens (react-native-screens)</summary> ### [`v3.26.0`](https://togithub.com/software-mansion/react-native-screens/releases/tag/3.26.0) [Compare Source](https://togithub.com/software-mansion/react-native-screens/compare/3.25.0...3.26.0) Minor release adding new useAnimatedHeaderHeight and useReanimatedHeaderHeight hooks, providing fixes for search bar and introducing internal changes. *Please note that new hooks introduced in this release are fully functional on Paper, on Fabric there are few edge cases we are still working on*. #### What's Changed #### 🐛 Bug fixes - Change implementation of `headerConfig` prop on Android by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1883 - Change elements visibility on search bar open by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1903 - Fix positioning of large header and search bar by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1895 - Change implementation of calculating status bar, refactor methods used on header height change by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1917 - Fix calculating header height when changing status/action bar visibility by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1922 - Allow Reanimated Screen to check large header by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1915 - Fix issue when emptying nav stack on Windows by [@chrisglein](https://togithub.com/chrisglein) in [software-mansion/react-native-screens#1890 - Update podspec to use install_modules_dependencies by [@cipolleschi](https://togithub.com/cipolleschi) in [software-mansion/react-native-screens#1920 - Remove MaxPerm args from JVM invocation by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1888 #### 👍 Improvements - Calculate values of useHeaderHeight natively by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1802 - Allow for different fragment types inside ScreenContainer by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1887 - Add focused states on page transitions by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1894 #### 🔢 Miscellaneous - **Create FUNDING.yml by [@aleqsio](https://togithub.com/aleqsio) in [software-mansion/react-native-screens#1886 - Migrate from deprecated `RCTEventEmitter` by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1867 - Use `require` syntax for resolution of all native components by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1909 - Run Android e2e with JDK 17 by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1892 - Put timelimit on execution of each workflow by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1893 - Trigger e2e tests on JS-only changes by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1910 - Update deprecated expo install instructions to `npx expo install` by [@GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira) in [software-mansion/react-native-screens#1899 - Bump activesupport from 6.1.7.3 to 7.0.7.2 in /TestsExample by [@dependabot](https://togithub.com/dependabot) in [software-mansion/react-native-screens#1877 - Update deps & RN in example apps after release by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1878 - Migrate `Example` app & e2e tests to RN 0.72.4 by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1880 - Bump library deps to recent versions (including RN) by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1881 - Bump library native Android deps & config by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1891 #### New Contributors - [@chrisglein](https://togithub.com/chrisglein) made their first contribution in [software-mansion/react-native-screens#1890 - [@GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira) made their first contribution in [software-mansion/react-native-screens#1899 - [@cipolleschi](https://togithub.com/cipolleschi) made their first contribution in [software-mansion/react-native-screens#1920 **Full Changelog**: software-mansion/react-native-screens@3.25.0...3.26.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: valora-bot <valorabot@valoraapp.com> Co-authored-by: Silas Boyd-Wickizer <silasbw@gmail.com> Co-authored-by: Silas Boyd-Wickizer <silas@valora.xyz>

kkafar added 25 commits September 13, 2023 17:46

Add FragmentHolder interface

55170ce

Add ScreenFragmentWrapper interface

1f31afb

Add onContainerUpdate to ScreenFragmentWrapper interface

103ce7e

Add activity & context resolving methods to ScreenFragmentWrapper

920a1a0

Add childScreenContainers to ScreenFragmentWrapper interface

6e1ba67

Add ScreenLifecycleEventDispatcher interface

50ccfaa

Rename ScreenLifecycleEventDispatcher -> ScreenEventDispatcher

5c7f455

ScreenFragmentWrapper extends ScreenEventDispatcher

9692479

Add more communication-with-continer-related methods to ScreenFragmen…

108bdfa

…tWrapper

Add View.recycle extension method

2f8cbc8

Fix method signatures in ScreenEventDispatcherInterface

36ca192

Partially migrate ScreenFragment

2562241

Finish initial implementation of ScreenFragmentWrapper for ScreenFrag…

6eb753f

…ment

Test migration of ScreenContainer to use ScreenFragmentWrapper instea…

33f4e11

…d of being parameterized with T: ScreenFragment

Fix type specs for ScreenContainer<*> -> just ScreenContainer

433441d

Fix type specs in ScreenStackFragment

aa1fa27

Pass fragment wrapper to a Screen

519180b

More adaptation in ScreenContainer

73e7d88

Start separating ScreenStackFragmentWrapper interface

732a0ec

Implement ScreenStackFragmentWrapper for ScreenStackFragment (in curr…

745a647

…ent interface shape)

Initial changes in ScreenStack

fafc033

Add canNavigateBack method to wrapper interface

b05fdbd

Add dismiss method to wrapper interface

b6f02bd

Finish initial migration of ScreenStack

3e4373d

Make it compile

277fbd9

kkafar force-pushed the @kkafar/partial-android-refactor branch from 956bcbb to 277fbd9 Compare September 13, 2023 15:47

kkafar commented Sep 13, 2023

View reviewed changes

kkafar requested review from WoLewicki and tboba September 13, 2023 16:35

WoLewicki reviewed Sep 14, 2023

View reviewed changes

kkafar added 4 commits September 15, 2023 08:35

Remove cherry-pick leftover

de2dadc

Unify naming between properties and methods

e947d9a

Revert "Add View.recycle extension method"

ee40686

This reverts commit 2f8cbc8.

Merge branch 'main' into @kkafar/partial-android-refactor

cc8b2df

WoLewicki approved these changes Sep 15, 2023

View reviewed changes

Fix nullability handling while casting newTop

eac2adc

kkafar merged commit 84ed791 into main Sep 15, 2023
4 checks passed

kkafar deleted the @kkafar/partial-android-refactor branch September 15, 2023 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact(Android): allow for different fragment types inside ScreenContainer #1887

refact(Android): allow for different fragment types inside ScreenContainer #1887

kkafar commented Sep 13, 2023 •

edited

kkafar left a comment

kkafar Sep 13, 2023

kkafar Sep 13, 2023

kkafar Sep 13, 2023

WoLewicki left a comment

refact(Android): allow for different fragment types inside ScreenContainer #1887

refact(Android): allow for different fragment types inside ScreenContainer #1887

Conversation

kkafar commented Sep 13, 2023 • edited

Description

Changes

Test code and steps to reproduce

Checklist

kkafar left a comment

Choose a reason for hiding this comment

kkafar Sep 13, 2023

Choose a reason for hiding this comment

kkafar Sep 13, 2023

Choose a reason for hiding this comment

kkafar Sep 13, 2023

Choose a reason for hiding this comment

WoLewicki left a comment

Choose a reason for hiding this comment

kkafar commented Sep 13, 2023 •

edited