This PR fixes https://github.com/tuskyapp/Tusky/issues/2798 and is
mostly based on and supersedes
https://github.com/tuskyapp/Tusky/pull/2826 but I have fixed all merge
conflicts and unit tests.
I tested the changes locally and the setting takes effect immediately
for replies, and persists across killing the app.
---------
Co-authored-by: Eva Tatarka <eva@tatarka.me>
Co-authored-by: Konrad Pozniak <connyduck@users.noreply.github.com>
As discussed in our contributors meeting.
Advantages:
- last element of list never obscured by action button
- less code that runs on every scroll
- less settings to worry about
Additionally:
- Added a (smaller) padding to the bottom of lists without action
button, I think it looks nice if there is a bit of white space and the
nav bar divider and the last list divider don't touch.
- The list of filters had no dividers, I added them.
- Recyclerviews with fixed height (Drafts, Filters, edits) now have
scrollbars
- code formatted all touched xml files
closes https://github.com/tuskyapp/Tusky/issues/1563
<img
src="https://github.com/tuskyapp/Tusky/assets/10157047/cd50199f-e84f-4402-93e4-a5a1beba2a08"
width="280"/>
Currently translated at 100.0% (645 of 645 strings)
Translated using Weblate (Ukrainian)
Currently translated at 100.0% (645 of 645 strings)
Co-authored-by: Ihor Hordiichuk <igor_ck@outlook.com>
Translate-URL: https://weblate.tusky.app/projects/tusky/tusky/uk/
Translation: Tusky/Tusky
Currently translated at 100.0% (645 of 645 strings)
Translated using Weblate (Italian)
Currently translated at 100.0% (643 of 643 strings)
Co-authored-by: Manuel <mannivuwiki@gmail.com>
Translate-URL: https://weblate.tusky.app/projects/tusky/tusky/it/
Translation: Tusky/Tusky
At first I thought simply changing the regex might help, but then I
found more and more differences between Mastodon and Tusky, so I decided
to reimplement the thing. I added 74 testcases that I all compared to
Mastodon to make sure they are correct.
On an Fairphone 4 the new implementation is faster, on an Samsung Galaxy
Tab S3 slower.
Testcases for the benchmark:
```
test of a status with #one hashtag http
```
```
test
http:// #hashtag https://connyduck.at/http://example.org
this is a #test
and this is a @mention@test.com @test @test@test456@test.com
```
```
@mention@test.social Just your ordinary mention with a hashtag
#test
```
```
@mention@test.social Just your ordinary mention with a url
https://riot.im/app/#/room/#Tusky:matrix.org
```
FP4:
```
11.159 ns 15 allocs Benchmark.new_1
119.701 ns 43 allocs Benchmark.new_2
21.895 ns 24 allocs Benchmark.new_3
87.512 ns 32 allocs Benchmark.new_4
16.592 ns 46 allocs Benchmark.old_1
134.381 ns 169 allocs Benchmark.old_2
28.355 ns 68 allocs Benchmark.old_3
45.221 ns 77 allocs Benchmark.old_4
```
SGT3:
```
43,785 ns 18 allocs Benchmark.new_1
446,074 ns 43 allocs Benchmark.new_2
78,802 ns 26 allocs Benchmark.new_3
315,478 ns 32 allocs Benchmark.new_4
42,186 ns 45 allocs Benchmark.old_1
353,570 ns 157 allocs Benchmark.old_2
72,376 ns 66 allocs Benchmark.old_3
122,985 ns 74 allocs Benchmark.old_4
```
benchmark code is here: https://github.com/tuskyapp/tusky-span-benchmark
closes https://github.com/tuskyapp/Tusky/issues/4425
[](https://renovatebot.com)
This PR contains the following updates:
| Package | Update | Change |
|---|---|---|
| [gradle](https://gradle.org)
([source](https://togithub.com/gradle/gradle)) | minor | `8.7` -> `8.8`
|
---
### Release Notes
<details>
<summary>gradle/gradle (gradle)</summary>
### [`v8.8`](https://togithub.com/gradle/gradle/compare/v8.7.0...v8.8.0)
[Compare
Source](https://togithub.com/gradle/gradle/compare/v8.7.0...v8.8.0)
</details>
---
### Configuration
📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).
🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.
♻ **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.
---
- [ ] <!-- rebase-check -->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/tuskyapp/Tusky).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzcuOCIsInVwZGF0ZWRJblZlciI6IjM3LjM3Ny44IiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCIsImxhYmVscyI6W119-->
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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
Quentí
Deleted User
Eliot Lash
Weblate
renovate[bot]
Levi Bard
Konrad Pozniak
Ihor Hordiichuk
Ümit Solmaz
fin-w
Hồ Nhất Duy
Danial Behzadi
Manuel
XoseM
Christophe Beyls
