mirror of
https://github.com/tuskyapp/Tusky
synced 2025-02-04 10:47:49 +01:00
83403ebb58
This pull request has main 2 goals related to improving the handling of
View lifecycles in Fragments:
- **Use viewLifecycleOwner when applicable**: every coroutine touching
Views in Fragments must be launched in the `coroutinescope` of
`viewLifecycleOwner` to avoid the following issues:
1. The code will crash if it references a View binding that is no more
available and keeps running after the Fragment view hierarchy has been
destroyed.
2. The code will leak Views if it references Views from its parent scope
after the Fragment view hierarchy has been destroyed.
3. Multiple instances of the same coroutine will run at the same time,
if the coroutine is launched in `onViewCreated()` from the wrong scope
and the view hierarchy is destroyed then re-created in the same Fragment
instance.
- **Clear View-related references in Fragments**: it is an error to keep
a reference to Views or any other class containing View references in
Fragments after `onDestroyView()`. It creates a memory leak when the
Fragment is added to the back stack or is temporarily detached. A
typical object that leaks Views is a RecyclerView's Adapter: when the
adapter is set on the RecyclerView, the RecyclerView registers itself as
a listener to the Adapter and the Adapter now contains a reference to
the RecyclerView that is not automatically cleared. It is thus
recommended to clear all these view references one way or another, even
if the Fragment is currently not part of a scenario where it is detached
or added to a back stack.
In general, having a `lateinit var` not related to Dagger dependency
injection in a Fragment is a code smell and should be avoided:
- If that `lateinit var` is related to storing something View-related,
it must be removed if possible or made nullable and set to `null` in
`onDestroyView()`.
- If that `lateinit var` is meant to store fragment arguments, it can be
turned into a `val by lazy{}`.
- If that `lateinit var` is related to fetching some information from
the Activity, it can be turned into a `val` with no backing field that
will simply call the activity when accessed. There is no need to store
the value in the Fragment.
When possible, View-related values must not be stored as Fragment
fields: all views should be accessed only in `onViewCreated()` and
passed as arguments to various listeners down the chain.
However, it's still required to use nullable fields **when the Fragment
exposes public methods that are called directly by an external entity**,
and these methods use the View-related value. Since the Fragment has no
control over when the external entity will call these public methods,
the field must never assumed to be non-null and null checks must be
added for every call. Note that exposing public methods on a Fragment to
be called directly is an antipattern, but switching to a different
architecture is out of scope of this pull request.
- Use `viewLifecycleOwner` in Fragments where applicable.
- Remove view-related fields and instead declare them in
`onViewCreated()` when possible.
- When not possible, declare view-related fields as nullable and set
them to `null` in `onDestroyView()`.
- Pass non-null View-related fields as arguments when possible, to not
rely on the nullable Fragment field.
- Replace `lateinit var` containing an Activity-related value with `val`
accessing the Activity directly on demand.
- Remove some unused fragment fields.
- Replace `onCreateView()` with passing the layout id as Fragment
constructor argument when possible.
- Replace `isAdded` checks with `view != null`. A Fragment being added
to an Activity doesn't mean it has a View hierarchy (it may be detached
and invisible).
- Remove `mediaPlayerListener` field in `ViewVideoFragment` and turn it
into a local variable. It is then passed into a
`DefaultLifecycleObserver` that replaces the `onStart()`/`onStop()`
overrides and is unregistered automatically when the view hierarchy is
destroyed.