This pull request fixes the following issues:
- `FiltersActivity` launches a new coroutine to collect the ViewModel
state every time the Activity is resumed, without cancelling the
previous coroutine.
- `FiltersActivity` reloads the filters in `onResume()`, even if loading
is already in progress (without cancelling the current loading). This
can lead to inconsistent state.
List of improvements:
- Implement `launchAndRepeatOnLifecycle()` to combine
`coroutineScope.launch()` with `repeatOnLifecycle()` for the same
`Lifecycle`. Use it in `FiltersActivity` to update the view only when
the Activity is visible.
- Optimize the filters loading: load them when `FiltersViewModel` is
created and when returning from `EditFilterActivity` (when receiving the
Activity result). Cancel the load already in progress, if any.
- use `MutableStateFlow.update()` to update the state in a thread-safe
way.
- Turn `FiltersViewModel.deleteFilter()` into a suspending function in
order to perform the update in the coroutinescope of the Activity
lifecycle, so the View passed as argument doesn't leak.
- Wait for an ongoing load operation to complete before performing a
delete filter operation, so the state stays consistent.
- Add `Intent.withSlideInAnimation()` as a simpler and more flexible
alternative to `Activity.startActivityWithSlideInAnimation(Intent)`.
Two things changed here:
The check for `positionStart`only in `onItemRangeInserted` is not always
correct - we only want to jump up when something is inserted at the top,
if we already are at the top.
`enablePlaceholders = false` has unintended side effects - the
recyclerview adapter sometimes receives an "onItemRangeRemoved" followed
by an "onItemRangeInserted", instead of just "onItemRangeChanged".
Together they should make sure the timelines stay were they are.
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.
The pull request to integrate the SplashScreen library (#4413) required
overriding the theme before setting the layout in
`MainActivity.onCreate()`, in order to switch from `SplashTheme` to
`TuskyTheme`. Since the parent `BaseActivity` already contained code to
override the theme in case the user selects the "black" theme, that
logic was added at the same spot in `BaseActivity`.
However, since other Activities inherit from `BaseActivity` and
sometimes declare a different default theme than `TuskyTheme` in the
Manifest, the wrong theme was set for those Activities when not in Black
theme mode.
This pull request ensures that the theme will only be overridden to
`TuskyTheme` in `MainActivity`, the only Activity that uses a splash
screen.
The current code loads emojis using Glide into basic custom `Target`s
and doesn't keep a hard reference to the Target. This creates a few
problems:
- Unlike images loaded into `ImageViewTarget`s, Emoji animations are not
paused when the Activity/Fragment becomes invisible. GIF decoding use
resources in the background.
- When `TextView`s get recycled in a RecyclerView, the loading of emojis
for the previous bind are not canceled when binding the new text and
starting the load of the new emojis. This is also handled automatically
when using `ImageViewTarget` but not for custom targets. Also, when the
obsolete emojis complete loading, the `TextView` will be unnecessarily
invalidated and redrawn.
- Since Glide's `RequestManager` doesn't keep hard references to Targets
after they are loaded and the emoji Target is currently not stored in
any View, emojis don't get an opportunity to clean up (at least stop
their animation) when the Activity/Fragment is destroyed. Depending on
the Drawable implementation, animations may run forever in the
background and cause memory leaks.
This pull request aims to properly track the lifecycle of emoji Targets,
cancel their loading an stop animations when appropriate. It also
reimplements `emojify()` to be more efficient.
- Add extension functions `View.setEmojiTargets()` and
`View.clearEmojiTargets()` to store and clear lists of emoji targets in
View tags, keeping a hard reference to them as long as the View is used.
When clearing emoji targets, pending requests will be canceled and
animations will be stopped to free memory. This is similar to what
`ImageViewTarget` does, except here multiple Targets are stored for a
single View instead of one.
- Add helper extension function `View.updateEmojiTargets()` to
automatically clear the View emoji targets, then allowing to call
`emojify()` one or more times in the `EmojiTargetScope` of that View,
and finally store all the pending targets of the `EmojiTargetScope` in
the View.
- Reimplement `CharSequence.emojify()` using
`View.updateEmojiTargets()`. This is used in RecyclerViews as well and
will automatically cancel previous emoji loadings for the same View and
stop animations.
- The main logic of `emojify()` has been moved to `EmojiTargetScope`.
Replace usage of slow regex `Matcher` with faster
`CharSequence.indexOf()`. Use `SpannableString` (with `toSpannable()`)
instead of `SpannableStringBuilder` to store the `EmojiSpan`s.
- Rename `EmojiSpan.getTarget()` to `EmojiSpan.createGlideTarget()` and
improve the target to stop/resume the animation according to the parent
component lifecycle, and stop the animation when clearing the target.
Use a hard reference to the view instead of a weak reference, since the
lifecycle of the Target now matches the one of the View and the Target
will be cleared at the latest when the View is destroyed.
- Use `View.updateEmojiTargets()` in `ReportNotificationViewHolder` in
order to store the targets of 2 separate sets of emojis into the same
TextView.
- Fix: reimplement the code to merge the 2 emoji sets into a single
`CharSequence` in `ReportNotificationViewHolder` using
`TextUtils.expandTemplate()`. The current code uses `String.format()`
which returns a String instead of a Spannable so the computed emojis are
lost.
- Store the emoji targets in `AnnouncementAdapter` in the parent view
after clearing the previous ones. It is a better location than storing
one emoji target in each child `Tooltip` view because tooltips are not
recycled when refreshing the data and the previous targets would not be
canceled properly.
- Bonus: update `ViewVideoFragment` to use `CustomViewTarget` instead of
`CustomTarget` to load the default artwork into `PlayerView`. The
loading will also automatically be canceled when the fragment view is
detached.
Using `Either<Throwable, T>` is basically the same as `Result<T>` with a
less friendly API. Futhermore, `Either` is currently only used in a
single component.
- Replace `Either` with `Result` in `AccountsInListFragment` and
`AccountsInListViewModel`.
- Add a method to convert a `NetworkResult` to a `Result` in
`AccountsInListViewModel`. Alternatively, `NetworkResult` could be used
everywhere in the code but other classes are already using `Result`.
- Replace `updateState()` method with `MutableStateFlow.update()` in
`AccountsInListViewModel`.
- Store the current search query in a `MutableStateFlow` collected by a
coroutine. This allows automatically cancelling the previous search when
a new query arrives, instead of launching a new coroutine for each query
which may conflict with the previous ones.
- Optimize `ListUtils`.
This does 4 things:
- Alt text is now translated when opening media of translated posts.
Previously only the long-press alt text was translated.
- The translate button is now hidden on non-public posts. The Mastodon
api returns 403 there.
- Translated posts will only be collapsible when the original was
collapsible as well. It is just weird when an "show more" button
suddenly appears because the post got longer by translating it.
- The translation status and the untranslate button are now shown below
each other instead of next to each other. Looks way better on smaller
display or long texts.
Before / After
<img
src="https://github.com/tuskyapp/Tusky/assets/10157047/2cadd15b-2e28-4989-9bd3-d3bdd4c75329"
width="320"/> <img
src="https://github.com/tuskyapp/Tusky/assets/10157047/0ecab094-6c96-49a5-bc99-aa56b7fe2ec2"
width="320"/>
Found a post with this weird media focus in the wild on chaos.social:
```
"focus": {
"x": 0.0,
"y": null
}
```
```
com.squareup.moshi.JsonDataException: Expected a double but was NULL at path $[0].media_attachments[0].meta.focus.y
at com.squareup.moshi.JsonUtf8Reader.nextDouble(JsonUtf8Reader.java:787)
at com.squareup.moshi.StandardJsonAdapters$6.fromJson(StandardJsonAdapters.java:167)
at com.squareup.moshi.StandardJsonAdapters$6.fromJson(StandardJsonAdapters.java:164)
at com.keylesspalace.tusky.entity.Attachment_FocusJsonAdapter.fromJson(Attachment_FocusJsonAdapter.kt:37)
at com.keylesspalace.tusky.entity.Attachment_FocusJsonAdapter.fromJson(Attachment_FocusJsonAdapter.kt:20)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at com.keylesspalace.tusky.entity.Attachment_MetaDataJsonAdapter.fromJson(Attachment_MetaDataJsonAdapter.kt:54)
at com.keylesspalace.tusky.entity.Attachment_MetaDataJsonAdapter.fromJson(Attachment_MetaDataJsonAdapter.kt:23)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at com.keylesspalace.tusky.entity.AttachmentJsonAdapter.fromJson(AttachmentJsonAdapter.kt:66)
at com.keylesspalace.tusky.entity.AttachmentJsonAdapter.fromJson(AttachmentJsonAdapter.kt:22)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at com.squareup.moshi.CollectionJsonAdapter.fromJson(CollectionJsonAdapter.java:81)
at com.squareup.moshi.CollectionJsonAdapter$2.fromJson(CollectionJsonAdapter.java:55)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at com.keylesspalace.tusky.entity.StatusJsonAdapter.fromJson(StatusJsonAdapter.kt:195)
at com.keylesspalace.tusky.entity.StatusJsonAdapter.fromJson(StatusJsonAdapter.kt:26)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at com.squareup.moshi.CollectionJsonAdapter.fromJson(CollectionJsonAdapter.java:81)
at com.squareup.moshi.CollectionJsonAdapter$2.fromJson(CollectionJsonAdapter.java:55)
at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:41)
at retrofit2.converter.moshi.MoshiResponseBodyConverter.convert(MoshiResponseBodyConverter.java:46)
at retrofit2.converter.moshi.MoshiResponseBodyConverter.convert(MoshiResponseBodyConverter.java:27)
at retrofit2.OkHttpCall.parseResponse(OkHttpCall.java:246)
at retrofit2.OkHttpCall$1.onResponse(OkHttpCall.java:156)
at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:519)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at java.lang.Thread.run(Thread.java:919)
```
(this one is for @charlag)
Calling `PreferenceManager.getDefaultSharedPreferences()` will read the
preference file from disk every time. This PR makes `SharedPreferences`
a singleton so they will only be created once at appstart (with a few
exceptions where it is hard to inject, e.g. in the `openLink` helper)
which should help getting our ANRs down.
```
StrictMode policy violation; ~duration=285 ms: android.os.strictmode.DiskReadViolation
at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1666)
at libcore.io.BlockGuardOs.access(BlockGuardOs.java:74)
at libcore.io.ForwardingOs.access(ForwardingOs.java:128)
at android.app.ActivityThread$AndroidOs.access(ActivityThread.java:8054)
at java.io.UnixFileSystem.checkAccess(UnixFileSystem.java:313)
at java.io.File.exists(File.java:813)
at android.app.ContextImpl.ensurePrivateDirExists(ContextImpl.java:790)
at android.app.ContextImpl.ensurePrivateDirExists(ContextImpl.java:781)
at android.app.ContextImpl.getPreferencesDir(ContextImpl.java:737)
at android.app.ContextImpl.getSharedPreferencesPath(ContextImpl.java:962)
at android.app.ContextImpl.getSharedPreferences(ContextImpl.java:583)
at android.content.ContextWrapper.getSharedPreferences(ContextWrapper.java:221)
at android.content.ContextWrapper.getSharedPreferences(ContextWrapper.java:221)
at androidx.preference.PreferenceManager.getDefaultSharedPreferences(PreferenceManager.java:119)
at com.keylesspalace.tusky.BaseActivity.onCreate(BaseActivity.java:96)
...
```
This does four things
- set `enablePlaceholders = false` on `PagingConfig`s to avoid Paging
Data that contains null placeholders, we don't want them (everywhere,
not just in notifications)
- make sure NotificationsPagingAdapter does not crash when it encounters
a null placeholder
- makes sure the notifications refresh correctly when the filters change
- the filters are now also respected when loading a gap
closes#4433
Currently translated at 100.0% (643 of 643 strings)
Translated using Weblate (Italian)
Currently translated at 99.3% (639 of 643 strings)
Co-authored-by: Manuel <mannivuwiki@gmail.com>
Translate-URL: https://weblate.tusky.app/projects/tusky/tusky/it/
Translation: Tusky/Tusky
Currently translated at 100.0% (643 of 643 strings)
Translated using Weblate (Welsh)
Currently translated at 100.0% (643 of 643 strings)
Co-authored-by: fin-w <fin-w@tutanota.com>
Translate-URL: https://weblate.tusky.app/projects/tusky/tusky/cy/
Translation: Tusky/Tusky
API 31+ devices don't need to install the compat SplashScreen because
they have their own native splash screen implementation. Furthermore, we
don't need to customize the splash screen so there is no need to call
the library.
On API 31+, the SplashScreen compat library registers some kind of
listener which causes display bugs with the Tusky theme on some Android
versions. Disabling the library on API 31+ solves this issue.
Fixes#4446.
Loading the image as a `Drawable` allows using the Drawable API to
abstract the drawing.
This way, any kind of `Drawable` (including the fallback vector
drawable) can be drawn in one pass to the Bitmap, without having to be
converted to a `Bitmap` first.
Also, `BitmapDrawable` will automatically use
[`Paint.FILTER_BITMAP_FLAG`](https://developer.android.com/reference/android/graphics/Paint#FILTER_BITMAP_FLAG)
when drawing, ensuring the resized image is high-quality even when using
a Bitmap-backed Canvas.
Currently some vector drawables are loaded using the AppCompat library
and others are loaded using the framework.
This pull request uniformizes this to use AppCompat to load them all.
Other changes:
- Set all compound drawables using relative positioning, since all XML
layouts are also using relative positioning.
- Remove unnecessary layer list drawable used to center
`R.drawable.ic_play_indicator` icon and use
`ImageView.setForegroundGravity()` instead.
- Merge layers in toolbar icons `ic_arrow_back_with_background` and
`ic_more_with_background` into a single vector drawable. Note that the
AppCompat implementation of vector drawables is unable to load vector
drawables inside layer-list drawables, so this change also makes these
images compatible with older Android versions.
**Note**: technically, AppCompat will always delegate to the framework
to load vector drawables on API 24+ which is the current minSDK version
of the app. But at least this gives the option to lower the minSDK
version in the future.
This means a popup will appear if you have that option enabled in the
preferences which will have a popup similar to the unfollow dialog
asking you if you want to follow the user.
The Androidx SplashScreen library is added as a dependency to the
project but isn't properly enabled in the current code. This pull
request configures the splash screen properly.
- Remove `SplashScreenActivity` which is not needed and use
`MainActivity` as main entry point to the application. `MainActivity`
inherits from `BaseActivity` which already detects if no account is
configured and redirects to `LoginActivity` if needed, just like
`SplashScreenActivity`.
- Initialize the SplashScreen library in `MainActivity.onCreate()`.
- Instead of letting the SplashScreen library set the final theme from
the `postSplashScreenTheme` attribute in SplashTheme, let `BaseActivity`
set it according to the user settings.
- When no account is available in `MainActivity.onCreate()`, keep the
splash screen shown until `LoginActivity` appears.
- Disable the slide-in animation when launching `LoginActivity` when no
account is available because the detection happens in `onCreate()` and
an Activity that finishes itself in `onCreate()` will not be drawn, so
the slide-in animation will not be visible either and only
`LoginActivity` will appear.
- Upgrade `core-splashscreen` to 1.2.0-alpha01 which contains a fix for
corrupted app theme on API 31+.
`RequestListener.onResourceReady()` may also be called to provide the
placeholder image when the request is starting or when it is cleared.
Check if the current request is complete (its status set to COMPLETE) to
determine if the Glide resource is for a thumbnail/placeholder or the
final image, and resume the coroutine only for the final image (or in
case of error).
This logic is [borrowed from the Glide Flow
API](a7351b0ecf/integration/ktx/src/main/java/com/bumptech/glide/integration/ktx/Flows.kt (L378)).
The main benefit of upgrading to version 1.2.0 of `DrawerLayout` is ~~to
properly support **predictive back animations**: when initiating a back
gesture on API 33+, the DrawerLayout will animate automatically~~ to
handle back navigation automatically. The predictive back animation of
the menu however depends on `NavigationView` which is not used in the
project.
In addition to the upgrade, simplify DrawerLayout integration:
- Forward key events to the DrawerLayout so it can intercept them and
close itself when needed.
- Don't handle the DrawerLayout closing manually using the
`OnBackPressedCallback` anymore. This is not necessary since the back
event will now be intercepted by the DrawerLayout when needed before
reaching the `OnBackPressedDispatcher`.
- Remove legacy fix for DrawerLayout staying open after Activity
recreation.
Hilt is an annotation processor built on top of Dagger which allows to
remove all the Android dependency injection boilerplate code (currently
around 900 lines) by writing it for us.
Hilt can use KSP instead of Kapt so Kapt can be completely removed from
the project. Kapt is slow, deprecated and has a few compatibility
issues. Removing Kapt will improve build times since no Java stubs have
to be generated for Kotlin classes anymore (Note that KSP also processes
annotations in Java classes so it can completely replace Kapt).
- Remove all modules related to manual dependency injection
configuration.
- Rename `AppModule` to `StorageModule` since it now only contains
configuration to retrieve the DataBase and SharedPreferences.
- Annotate all entry points (Activities, Fragments, BroadcastReceivers
and Services) with `@AndroidEntryPoint`.
- Annotate all injected ViewModels with `@HiltViewModel` and replace the
custom ViewModel Factory with the default one (which integrates with the
one generated by Hilt).
- Add a public field to allow overriding the default
ViewModelProvider.Factory in `BaseActivity` in tests.
- Annotate tested Activities with `@OptionalInject` since Activity tests
currently rely on the Activities not being injected automatically.
- Annotate injected `Context` arguments with `@ApplicationContext`. Hilt
provides the `Context` binding automatically but requires to specify if
the Application or Activity Context is wanted.
- Add WorkManager Hilt integration so all Workers are injected by Hilt
automatically using `HiltWorkerFactory`.
- Lazily initialize WorkManager in `TuskyApplication`.
- Remove Kapt and Kapt workarounds.
- ~~Remove toolchain configuration for Java 21. Toolchains force the
Java bytecode to match the JDK version used to build the project, and
apparently Hilt doesn't run inside the toolchain so cannot process the
source code if the JDK version of the toolchain is higher than the JDK
used to run Gradle. [And configuring a toolchain for an older Java
version causes other
issues](https://jakewharton.com/gradle-toolchains-are-rarely-a-good-idea/).
**Removing toolchains configuration doesn't prevent the project from
being built using JDK 21** or more recent versions but allows to build
the project using older JDKs as well.~~
Added a fix to allow Hilt to properly use the JDK toolchain.
- ~~Set the Java and Kotlin bytecode target to Java 17. The standard
bytecode target for Android projects is usually Java 8 or 11 (any higher
version doesn't provide any benefit but may cause compatibility issues).
However, since the app currently uses a library built against Java 17
bytecode (`networkresult-calladapter`), it needs to target at least Java
17 bytecode as well.~~
- Update the Dagger 2 URL in the licenses screen. Hilt is part of Dagger
2 so the label wasn't changed.
Konrad Pozniak
renovate[bot]
Levi Bard
Christophe Beyls
Manuel
Ümit Solmaz
fin-w
Salif Mehmed
Stavros
Quentí
Hồ Nhất Duy
Danial Behzadi
fries1234
