Skip to content

Commit

Permalink
fix: Ensure actions happen against the correct status (#373)
Browse files Browse the repository at this point in the history
Previously when the user interacted with a status the operation (reblog,
favourite, etc) travels through multiple layers of code, carrying with
it the position of the item in the list that the user operated on.

At some point the status is retrieved from the list using its position
so that the correct status ID can be used in the network operation.

If this happens while the list is also refreshing there's a possible
race condition, and the original status' position may have changed in
the list. Looking up the status by position to determine which status to
perform the action on may cause the action to happen on the wrong
status.

Fix this by passing the status' viewdata to any actions instead of its
position. This includes all the information necessary to make the API
call, so there is no chance of a race.

This is quite an involved change because there are three types of
viewdata:

- `StatusViewData`, used for regular timelines
- `NotificationViewData`, used for notifications, may wrap a status that
can be operated on
- `ConversationViewData`, used for conversations, does wrap a status

The previous code treated them all differently, which is probably why it
operated by position instead of type.

The high level fix is to:

1. Create an interface, `IStatusViewData`, that contains the data
exposed by any viewdata that contains a status.

2. Implement the interface in `StatusViewData`, `NotificationViewData`,
and `ConversationViewData`.

3. Change the code that operates on viewdata (`SFragment`,
`StatusActionListener`, etc) to be generic over anything that implements
`IStatusViewData`.

4. Change the code that handles actions to pass the viewdata instead of
the position.

Fixes #370
  • Loading branch information
nikclayton authored Jan 26, 2024
1 parent 947e5c2 commit fc81e8b
Show file tree
Hide file tree
Showing 34 changed files with 833 additions and 852 deletions.
32 changes: 16 additions & 16 deletions app/lint-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2423,7 +2423,7 @@
errorLine2=" ~~~~~~~">
<location
file="src/main/java/app/pachli/components/conversation/ConversationsFragment.kt"
line="152"
line="154"
column="29"/>
</issue>

Expand All @@ -2434,7 +2434,7 @@
errorLine2=" ~~~~~~~">
<location
file="src/main/java/app/pachli/components/conversation/ConversationsFragment.kt"
line="154"
line="156"
column="37"/>
</issue>

Expand Down Expand Up @@ -2522,7 +2522,7 @@
errorLine2=" ~~~~~~~">
<location
file="src/main/java/app/pachli/util/ListStatusAccessibilityDelegate.kt"
line="75"
line="76"
column="29"/>
</issue>

Expand All @@ -2533,7 +2533,7 @@
errorLine2=" ~~~~~~~~">
<location
file="src/main/java/app/pachli/util/ListStatusAccessibilityDelegate.kt"
line="81"
line="82"
column="21"/>
</issue>

Expand All @@ -2544,7 +2544,7 @@
errorLine2=" ~~~~~~~~~~~">
<location
file="src/main/java/app/pachli/util/ListStatusAccessibilityDelegate.kt"
line="86"
line="87"
column="21"/>
</issue>

Expand All @@ -2555,7 +2555,7 @@
errorLine2=" ~~~~~~~~~">
<location
file="src/main/java/app/pachli/util/ListStatusAccessibilityDelegate.kt"
line="105"
line="107"
column="21"/>
</issue>

Expand All @@ -2566,7 +2566,7 @@
errorLine2=" ~~~~~~~~~">
<location
file="src/main/java/app/pachli/util/ListStatusAccessibilityDelegate.kt"
line="115"
line="117"
column="21"/>
</issue>

Expand Down Expand Up @@ -3523,7 +3523,7 @@
errorLine2=" ~~~~~~~~~">
<location
file="src/main/java/app/pachli/components/notifications/NotificationsFragment.kt"
line="169"
line="161"
column="43"/>
</issue>

Expand All @@ -3534,40 +3534,40 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/java/app/pachli/fragment/SFragment.kt"
line="240"
line="235"
column="48"/>
</issue>

<issue
id="SyntheticAccessor"
message="Access to `private` method `getContentWarningDescription` of class `Companion` requires synthetic accessor"
errorLine1=" getContentWarningDescription(context, status),"
errorLine1=" getContentWarningDescription(context, viewData),"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt"
line="812"
line="808"
column="13"/>
</issue>

<issue
id="SyntheticAccessor"
message="Access to `private` method `getReblogDescription` of class `Companion` requires synthetic accessor"
errorLine1=" getReblogDescription(context, status),"
errorLine1=" getReblogDescription(context, viewData),"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt"
line="816"
line="812"
column="13"/>
</issue>

<issue
id="SyntheticAccessor"
message="Access to `private` method `getMediaDescription` of class `Companion` requires synthetic accessor"
errorLine1=" getMediaDescription(context, status),"
errorLine1=" getMediaDescription(context, viewData),"
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt"
line="821"
line="817"
column="13"/>
</issue>

Expand Down Expand Up @@ -3644,7 +3644,7 @@
errorLine2=" ~~~~~~~~~">
<location
file="src/main/java/app/pachli/components/timeline/TimelineFragment.kt"
line="195"
line="196"
column="47"/>
</issue>

Expand Down
22 changes: 10 additions & 12 deletions app/src/main/java/app/pachli/adapter/FilterableStatusViewHolder.kt
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,25 @@ import app.pachli.core.network.model.Filter
import app.pachli.databinding.ItemStatusWrapperBinding
import app.pachli.interfaces.StatusActionListener
import app.pachli.util.StatusDisplayOptions
import app.pachli.viewdata.StatusViewData
import app.pachli.viewdata.IStatusViewData

open class FilterableStatusViewHolder(
open class FilterableStatusViewHolder<T : IStatusViewData>(
private val binding: ItemStatusWrapperBinding,
) : StatusViewHolder(binding.statusContainer, binding.root) {
) : StatusViewHolder<T>(binding.statusContainer, binding.root) {

override fun setupWithStatus(
status: StatusViewData,
listener: StatusActionListener,
viewData: T,
listener: StatusActionListener<T>,
statusDisplayOptions: StatusDisplayOptions,
payloads: Any?,
) {
super.setupWithStatus(status, listener, statusDisplayOptions, payloads)
setupFilterPlaceholder(status, listener)
super.setupWithStatus(viewData, listener, statusDisplayOptions, payloads)
setupFilterPlaceholder(viewData, listener)
}

private fun setupFilterPlaceholder(
status: StatusViewData,
listener: StatusActionListener,
status: T,
listener: StatusActionListener<T>,
) {
if (status.filterAction !== Filter.Action.WARN) {
showFilteredPlaceholder(false)
Expand Down Expand Up @@ -75,9 +75,7 @@ open class FilterableStatusViewHolder(
matchedFilter.title,
)
binding.statusFilteredPlaceholder.statusFilterShowAnyway.setOnClickListener {
listener.clearWarningAction(
bindingAdapterPosition,
)
listener.clearWarningAction(status)
}
}

Expand Down
Loading

0 comments on commit fc81e8b

Please sign in to comment.