Skip to content
This repository was archived by the owner on May 23, 2025. It is now read-only.

818 group notifications #3452

Draft
Lakoja wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

818 group notifications #3452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
714bde6
818: Show +numbers for favorites and boosts
Lakoja Mar 12, 2023
f1c115c
818: Track favorites and filter old duplicated entries
Lakoja Mar 14, 2023
6ca832f
818: Also support boosts; rework logging a bit
Lakoja Mar 15, 2023
ff6f991
818: Add another todo comment about tracker id refresh (edge case bug)
Lakoja Apr 17, 2023
6c794f3
818: Add another bug todo
Lakoja Apr 17, 2023
7994c63
818: Use proper id comparison
Lakoja Jul 5, 2023
860e7d9
818: Avoid "list is empty" (due to a crash in load())
Lakoja Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,13 @@ class NotificationsFragment :
(binding.recyclerView.itemAnimator as SimpleItemAnimator?)!!.supportsChangeAnimations =
false

// Signal the user that a refresh has loaded new items above their current position
// by scrolling up slightly to disclose the new content

adapter.registerAdapterDataObserver(object : RecyclerView.AdapterDataObserver() {
override fun onItemRangeInserted(positionStart: Int, itemCount: Int) {
removeDuplicateOlderEntries(positionStart)
Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a good location? "Consolidate complete list after inserts..."

I tried it below in "pagingData.collectLatest" first but I have no real idea what that does (or my idea does not match reality really).

Copy link
Copy Markdown
Contributor

@nikclayton nikclayton Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK for prototyping, but in the final version this probably belongs in the viewmodel.

Rule of thumb.

  • Fragment displays data, can make decisions about what the UI of that data looks like (e.g., is a status shown expanded or collapsed)
  • ViewModel determines what the actual data is

So if data is going to be removed from the list it should probably happen in the viewmodel.

This is not a hard and fast rule, but it does make testing easier.

Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See below - this is only the second filtering.)


// Signal the user that a refresh has loaded new items above their current position
// by scrolling up slightly to disclose the new content
if (positionStart == 0 && adapter.itemCount != itemCount) {
binding.recyclerView.post {
if (getView() != null) {
Expand All @@ -250,6 +253,7 @@ class NotificationsFragment :
viewModel.pagingData.collectLatest { pagingData ->
Log.d(TAG, "Submitting data to adapter")
adapter.submitData(pagingData)
// TODO why is this called always _before_ anything from NotificationsPagingSource is loaded (and with what data)?
Copy link
Copy Markdown
Contributor

@nikclayton nikclayton Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short:

This code will run any time viewModel.pagingData is modified. So the position of this block of code (the inner launch block) doesn't especially matter within the larger repeatOnLifecycle block.

Long:

One of the first things the viewmodel does is start fetching data, using NotificationsRepository. When it has new data it is emitted in to pagingData.

The block of code here runs in a coroutine (i.e., it's asynchronous) and waits for new data to be emitted in pagingData. When it does the block of code inside collectLatest is run.

When collectLatest has run the code inside the lambda it goes back to waiting for more data to be emitted. When more data is emitted collectLatest runs the lambda again. And so on -- every time there's new data the lambda inside collectLatest is re-run.

There's two possible scenarios at start up:

  1. This code is run after the viewmodel has fetched data and emitted it. In which case the lambda inside collectLatest will run immediately.
  2. This code is run before the viewmodel has fetched data and emitted it. In which case the collectLatest call will block until data is available. But that's fine, because this is all running in a coroutine (because of the launch), so collectLatest blocking doesn't block anything else. When the data is emitted the collectLatest call executes the lambda, and then blocks again waiting for new data.

Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always see this in the log:

(me pulling refresh)

  • submitting data (here from collectLatest)
  • then 3 fetching data (from the NotificationsPagingSource)

I would have expected the fetch to happen before the submit.

(Or conversely: How do the fetched iteams after the submit end up in the list?)

Copy link
Copy Markdown
Contributor

@nikclayton nikclayton Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. So lets walk through what happens.

Here's a sample log I got just now from doing swipe-refresh on my notifications:

Submitting data to adapter
load() with Refresh for key: 131541099
--> GET https://mastodon.social/api/v1/notifications/131541099
--> GET https://mastodon.social/api/v1/notifications?max_id=131541099&limit=90
<-- 200 https://mastodon.social/api/v1/notifications/131541099 (182ms, unknown-length body)
<-- 200 https://mastodon.social/api/v1/notifications?max_id=131541099&limit=90 (1377ms, unknown-length body)
load() with Prepend for key: 131541099
--> GET https://mastodon.social/api/v1/notifications?min_id=131541099&limit=30
<-- 200 https://mastodon.social/api/v1/notifications?min_id=131541099&limit=30 (197ms, unknown-length body)

The swipe is caught, and the swipe listener will be called.

The listener was set earlier in the code with binding.swipeRefreshLayout.setOnRefreshListener(this), so the swipe results in a call to NotificationsFragment.onRefresh().

That does:

override fun onRefresh() {
    binding.progressBar.isVisible = false
    adapter.refresh()
}

Per the documentation for the adapter's refresh() method:

refresh triggers the creation of a new PagingData with a new instance of PagingSource to represent an updated snapshot of the backing dataset.

So this creates a new PagingData, which means there's a new value in viewModel.pagingData, which means anything that's collecting values from that flow is going to be run, which means this code in the fragment runs:

launch {
    viewModel.pagingData.collectLatest { pagingData ->
        Log.d(TAG, "Submitting data to adapter")
        adapter.submitData(pagingData)
    }
}

That explains the first log message. The PagingData is new, so it's collected, and submitted to the adapter.

The new (empty) PagingData has now been submitted to the adapter.

The adapter will now try and collect pages of data from this PagingData. It's new, so there aren't any pages, so the adapter eventually calls the load method on the PagingSource that is wrapped by the PagingData.

That triggers the load() with... log messages.

It's only then that the API calls happen, and the --> GET and similar log entries come from okhttp.

Finally, the adapter receives the new pages of data, diffs them against the old pages, inserting or deleting items as appropriate.

If you haven't already used it I strongly recommend Android Studio's Navigate menu, especially the Declaration or usages feature (Ctrl+B). You can use to, for example, navigate from the adapter.refresh() call in fun onRefresh() and drop down through the different layers of code in the framework libraries where all this happens.

Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the important thing is that the PagingData is empty initially.
(That isn't obvious I guess. I would expect elements to be transmitted/collected and not placeholder to trigger something else. :-) )
(While debugging you cannot really see that PagingData is empty. At least not I.)

}
}

Expand Down Expand Up @@ -437,6 +441,23 @@ class NotificationsFragment :
}
}

private fun removeDuplicateOlderEntries(positionStart: Int) {
val dataList = adapter.snapshot().items

for (pos in dataList.lastIndex downTo positionStart) {
val notificationViewData = dataList[pos]

val status = notificationViewData.statusViewData?.status
?: continue

if (!viewModel.hasNewestNotificationId(notificationViewData.type, status.id, notificationViewData.id)) {
Log.d(TAG, "Removing old notification at "+pos+" for "+status.id+" at "+status.createdAt)

adapter.notifyItemRemoved(pos)
Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply "notify removed" seems odd as I don't have removed something but I want it to be removed in the list.

However I guess it might be correct as the backing list is api calls and what is presented in the list is or should not be exactly the api data.

Copy link
Copy Markdown
Contributor

@nikclayton nikclayton Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good argument for doing this in the viewmodel, so that the data removal happens before the data is inserted in to the adapter (per my previous comment).

Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two types of filtering here:

One happens in the view model (pagingData.filter) but the one here is to remove old entries that are made only obsolete by other new entries.

}
}
}

override fun onCreateMenu(menu: Menu, menuInflater: MenuInflater) {
menuInflater.inflate(R.menu.fragment_notifications, menu)
menu.findItem(R.id.action_refresh)?.apply {
Expand Down
8 changes: 7 additions & 1 deletion ...c/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,15 @@ class NotificationsPagingSource @Inject constructor(
return LoadResult.Error(Throwable("HTTP $code: $msg"))
}

// TODO this is the only location with the "raw" data for answering the TODO in NotificationsViewModel.hasNewestNotificationId()?
// However the heuristic there would need (at least) the full range of notification ids here (so multiple calls to this method)?
val notificationList = response.body()!!
val apiNotificationIds = notificationList.map { it.id }
Log.w(TAG, (if (params is LoadParams.Refresh) "R" else "A/P") +" Got notifications from api "+apiNotificationIds.firstOrNull()+"-"+apiNotificationIds.lastOrNull())

val links = Links.from(response.headers()["link"])
return LoadResult.Page(
data = response.body()!!,
data = notificationList,
nextKey = links.next,
prevKey = links.prev
)
Expand Down
46 changes: 45 additions & 1 deletion app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.paging.PagingData
import androidx.paging.cachedIn
import androidx.paging.filter
import androidx.paging.map
import at.connyduck.calladapter.networkresult.getOrThrow
import com.keylesspalace.tusky.R
Expand All @@ -40,6 +41,7 @@ import com.keylesspalace.tusky.settings.PrefKeys
import com.keylesspalace.tusky.usecase.TimelineCases
import com.keylesspalace.tusky.util.StatusDisplayOptions
import com.keylesspalace.tusky.util.deserialize
import com.keylesspalace.tusky.util.isLessThanOrEqual
import com.keylesspalace.tusky.util.serialize
import com.keylesspalace.tusky.util.throttleFirst
import com.keylesspalace.tusky.util.toViewData
Expand Down Expand Up @@ -386,6 +388,7 @@ class NotificationsViewModel @Inject constructor(
)

viewModelScope.launch {
// TODO small: With this starting with filterIsInstance from a flow I cannot handle any other events?
eventHub.events
.filterIsInstance<PreferenceChangedEvent>()
.filter { StatusDisplayOptions.prefKeys.contains(it.preferenceKey) }
Expand Down Expand Up @@ -505,13 +508,32 @@ class NotificationsViewModel @Inject constructor(
)
}

// Status id -> (highest) Notification id
private val seenFavorites = HashMap<String, String>()
private val seenBoosts = HashMap<String, String>()

private fun getNotifications(
filters: Set<Notification.Type>,
initialKey: String? = null
): Flow<PagingData<NotificationViewData>> {
var n = 0

return repository.getNotificationsStream(filter = filters, initialKey = initialKey)
.map { pagingData ->
pagingData.map { notification ->
pagingData.filter { notification ->
val status = notification.status
?: return@filter true

n += 1

return@filter if (hasNewestNotificationId(notification.type, status.id, notification.id)) {
true
} else {
Log.d(TAG, "Filtering notification "+(n-1)+" for "+status.id+"/"+notification.id+" at "+status.createdAt)
false
}
}
.map { notification ->
notification.toViewData(
isShowingContent = statusDisplayOptions.value.showSensitiveMedia ||
!(notification.status?.actionableStatus?.sensitive ?: false),
Expand All @@ -533,6 +555,28 @@ class NotificationsViewModel @Inject constructor(
return initialKey
}

fun hasNewestNotificationId(type: Notification.Type, statusId: String, notificationId: String): Boolean {
val trackerArray = when(type) {
Notification.Type.FAVOURITE -> seenFavorites
Notification.Type.REBLOG -> seenBoosts
else -> null
} ?: return true

val highestNotificationId = trackerArray[statusId]

return if (highestNotificationId == null || highestNotificationId.isLessThanOrEqual(notificationId)) {
trackerArray[statusId] = notificationId

true
} else {
// TODO edge case: a newer favorite has been removed: the old notification will not be added again
Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this edge case is worth a fix.

(It will be correct for an app restart.)

Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have been trying to do the heuristic now. But I didn't really find a suitable place in all that paging and flowing.

(Added a few Todo comments, though.)

// (because the removed id is still in the seen array)
// The code could find this out only heuristically: "looking at these notification ids (range), one in the array is not amongst them"

false
}
}

/**
* @return Flow of relevant preferences that change the UI
*/
Expand Down
14 changes: 11 additions & 3 deletions ...ain/java/com/keylesspalace/tusky/components/notifications/StatusNotificationViewHolder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,9 @@ internal class StatusNotificationViewHolder(
animateEmojis: Boolean
) {
val statusViewData = notificationViewData.statusViewData
val displayName = notificationViewData.account.name.unicodeWrap()
val favoritesCount = statusViewData?.status?.favouritesCount ?: 0
val reblogCount = statusViewData?.status?.reblogsCount ?: 0
var userPart = notificationViewData.account.name.unicodeWrap()
val type = notificationViewData.type
val context = binding.notificationTopText.context
val format: String
Expand All @@ -251,10 +253,16 @@ internal class StatusNotificationViewHolder(
Notification.Type.FAVOURITE -> {
icon = getIconWithColor(context, R.drawable.ic_star_24dp, R.color.tusky_orange)
format = context.getString(R.string.notification_favourite_format)
if (favoritesCount > 1) {
userPart += " +" + (favoritesCount - 1)
Copy link
Copy Markdown
Collaborator Author

@Lakoja Lakoja Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this should be looking exactly is the second question (after the first if this whole thing actually works above).

I'd prefer a text of "and 7 others". However this introduces singular plural problems.

A more fancy display (list of avatars for example) is probably out of scope here.

Copy link
Copy Markdown
Contributor

@nikclayton nikclayton Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different strings for pluralisation is fine, see https://developer.android.com/guide/topics/resources/string-resource#Plurals (the app does this elsewhere).

Thinking about the UX, if this showed something like:

Lakoja and 7 others favourited your post

I would expect to be able to tap on that and see a dialog that listed everyone. Tapping on anyone in that dialog should take me to their profile page.

}
}
Notification.Type.REBLOG -> {
icon = getIconWithColor(context, R.drawable.ic_repeat_24dp, R.color.tusky_blue)
format = context.getString(R.string.notification_reblog_format)
if (reblogCount > 1) {
userPart += " +" + (reblogCount - 1)
}
}
Notification.Type.STATUS -> {
icon = getIconWithColor(context, R.drawable.ic_home_24dp, R.color.tusky_blue)
Expand All @@ -275,13 +283,13 @@ internal class StatusNotificationViewHolder(
null,
null
)
val wholeMessage = String.format(format, displayName)
val wholeMessage = String.format(format, userPart)
val str = SpannableStringBuilder(wholeMessage)
val displayNameIndex = format.indexOf("%s")
str.setSpan(
StyleSpan(Typeface.BOLD),
displayNameIndex,
displayNameIndex + displayName.length,
displayNameIndex + userPart.length,
Spanned.SPAN_EXCLUSIVE_EXCLUSIVE
)
val emojifiedText = str.emojify(
Expand Down