Refactor notifications to Kotlin & paging #4026

connyduck · 2023-09-19T19:10:40Z

This refactors the NotificationsFragment and related classes to Kotlin & paging.
While trying to preserve as much of the original behavior as possible, this adds the following improvements as well:

The "show notifications filter" preference was added again
The "load more" button now has a background ripple effect when clicked
The "legal" report category of Mastodon 4.2 is now supported in report notifications
Unknown notifications now display "unknown notification type" instead of an empty line

Other code quality improvements:

All views from xml layouts are now referenced via ViewBindings
the classes responsible for showing system notifications were moved to a new package systemnotifications while the classes from this refactoring are in notifications
the id of the local Tusky account is now called tuskyAccountId in all places I could find

closes #3429

connyduck · 2023-10-30T19:19:16Z

The basic database setup is done, it is a bit more complicated than the timeline one and needs some cleanup but works fine. @charlag maybe you can have a look at it, I would be very interested in your opinion.
I think it would be nice to not have duplicate tables
Merging NotificationAccountEntity and TimelineAccountEntity should be easy.
NotificationStatusEntity and TimelineStatusEntity not so much, since the timeline table is used for the home timeline and I can't just mix other statuses in there. Maybe add a boolean flag isInHomeTimeline?
Thoughts?

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt # app/src/main/java/com/keylesspalace/tusky/fragment/NotificationsFragment.java # app/src/main/res/values/strings.xml

charlag · 2023-11-05T19:54:47Z

app/src/main/java/com/keylesspalace/tusky/db/NotificationEntity.kt

+import com.keylesspalace.tusky.entity.Status
+import java.util.Date
+
+data class NotificationDataEntity(


in the end we should give each class here a one-sentence doc explaining what they are for

I was confused for a while before I realized that this class is not actually a DB table

charlag · 2023-11-05T20:12:57Z

The basic database setup is done, it is a bit more complicated than the timeline one and needs some cleanup but works fine. @charlag maybe you can have a look at it, I would be very interested in your opinion. I think it would be nice to not have duplicate tables Merging NotificationAccountEntity and TimelineAccountEntity should be easy. NotificationStatusEntity and TimelineStatusEntity not so much, since the timeline table is used for the home timeline and I can't just mix other statuses in there. Maybe add a boolean flag isInHomeTimeline? Thoughts?

I couldn't get too deeply into it but as far as I understand you did the same trick as with posts, just more complicated. Nice!

I think merging accounts could work if we can take care of cleanup but it gets a bit complicated.

We could merge statuses but it would be a lot of work. We would need a table with just IDs of the posts in timeline and a separate table with just posts (that's how Masto work internally too, with timelines being in Redis and posts in db). Then we could cache arbitrary posts in there.

I think at least for now your approach makes a lot of sense?

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/TuskyApplication.kt # app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt # app/src/main/java/com/keylesspalace/tusky/components/notifications/ReportNotificationViewHolder.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelinePagingAdapter.kt # app/src/main/java/com/keylesspalace/tusky/db/AppDatabase.java # app/src/main/java/com/keylesspalace/tusky/network/MastodonApi.kt

connyduck · 2024-04-18T19:50:28Z

Ok I think I have addressed everything

Goooler · 2024-04-19T06:55:55Z

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt

+            ListStatusAccessibilityDelegate(
+                binding.recyclerView,
+                this,
+                StatusProvider { pos: Int ->
+                    if (pos in 0 until adapter.itemCount) {
+                        val notification = adapter.peek(pos)
+                        // We support replies only for now
+                        if (notification is NotificationViewData.Concrete) {
+                            return@StatusProvider notification.statusViewData
+                        } else {
+                            return@StatusProvider null
+                        }
+                    } else {
+                        null
+                    }
+                }
+            )
+        )


Suggested change

ListStatusAccessibilityDelegate(

binding.recyclerView,

this,

StatusProvider { pos: Int ->

if (pos in 0 until adapter.itemCount) {

val notification = adapter.peek(pos)

// We support replies only for now

if (notification is NotificationViewData.Concrete) {

return@StatusProvider notification.statusViewData

} else {

return@StatusProvider null

}

} else {

null

}

}

)

)

ListStatusAccessibilityDelegate(

binding.recyclerView,

this

) { pos: Int ->

if (pos in 0 until adapter.itemCount) {

val notification = adapter.peek(pos)

// We support replies only for now

if (notification is NotificationViewData.Concrete) {

notification.statusViewData

} else {

null

}

} else {

null

}

}

Goooler · 2024-04-19T06:58:21Z

...src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingAdapter.kt

+import com.keylesspalace.tusky.util.StatusDisplayOptions
+import com.keylesspalace.tusky.viewdata.NotificationViewData
+
+interface NotificationActionListener {


Suggested change

interface NotificationActionListener {

fun interface NotificationActionListener {

Goooler · 2024-04-19T06:58:30Z

...src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingAdapter.kt

+    fun onViewReport(reportId: String?)
+}
+
+interface NotificationsViewHolder {


Suggested change

interface NotificationsViewHolder {

fun interface NotificationsViewHolder {

Goooler · 2024-04-19T07:03:37Z

app/src/main/java/com/keylesspalace/tusky/viewdata/NotificationViewData.kt

+sealed class NotificationViewData {
+
+    abstract val id: String
+
+    data class Concrete(
+        override val id: String,
+        val type: Notification.Type,
+        val account: TimelineAccount,
+        val statusViewData: StatusViewData.Concrete?,
+        val report: Report?
+    ) : NotificationViewData()
+
+    data class Placeholder(
+        override val id: String,
+        val isLoading: Boolean
+    ) : NotificationViewData()


Suggested change

sealed class NotificationViewData {

abstract val id: String

data class Concrete(

override val id: String,

val type: Notification.Type,

val account: TimelineAccount,

val statusViewData: StatusViewData.Concrete?,

val report: Report?

) : NotificationViewData()

data class Placeholder(

override val id: String,

val isLoading: Boolean

) : NotificationViewData()

sealed interface NotificationViewData {

val id: String

data class Concrete(

override val id: String,

val type: Notification.Type,

val account: TimelineAccount,

val statusViewData: StatusViewData.Concrete?,

val report: Report?

) : NotificationViewData

data class Placeholder(

override val id: String,

val isLoading: Boolean

) : NotificationViewData

Tak · 2024-04-19T14:48:50Z

Should the android notifications get dismissed when we open the notifications view?

connyduck · 2024-04-19T17:31:57Z

Should the android notifications get dismissed when we open the notifications view?

yes

cbeyls · 2024-04-21T20:16:24Z

...src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingAdapter.kt

+        bindViewHolder(viewHolder, position, payloads)
+    }
+
+    private fun bindViewHolder(viewHolder: RecyclerView.ViewHolder, position: Int, payloads: List<Any>?) {


The payloads argument in a RecyclerView.Adapter is actually never null, it will be an empty list by default. You can change all the associated methods signatures to non-null.

I still recommend to change all the payloads arguments to be non-null, then all the associated .isNullOrEmpty() tests to .isEmpty()

cbeyls · 2024-04-21T20:27:02Z

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt

+                shouldFilterStatus(notificationViewData) != Filter.Action.HIDE
+            }
+        }
+        .flowOn(Dispatchers.Default)


You don't need to change the upstream dispatcher if you already specify it in pagingData.map(<Dispatcher>) and pagingData.filter(<Dispatcher>).
It's probably better to keep this line but remove the custom dispatcher for map and filter.

...c/main/java/com/keylesspalace/tusky/components/notifications/StatusNotificationViewHolder.kt

cbeyls · 2024-04-21T21:18:50Z

...c/main/java/com/keylesspalace/tusky/components/notifications/StatusNotificationViewHolder.kt

+                        bindingAdapterPosition
+                    )
+                }
+                binding.notificationContent.visibility =


You can also use ktx extension properties for view visibility:

binding.notificationContent.isVisible = statusViewData.isExpanded

Note that the custom View.visible extension method used in the project serves the same purpose with a different syntax.

...src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingAdapter.kt

cbeyls · 2024-04-21T21:33:29Z

...c/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt

-                val translation = translations[timelineStatus.status.serverId]
-                timelineStatus.toViewData(
-                    moshi,
+            pagingData.map(Dispatchers.Default.asExecutor()) { timelineData ->


Same remark here, it's probably overkill to specify a dispatcher both here and in flowOn()

app/src/main/java/com/keylesspalace/tusky/db/dao/CleanupDao.kt

app/src/main/java/com/keylesspalace/tusky/viewdata/NotificationViewData.kt

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/util/ShareShortcutHelper.kt

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/adapter/NotificationsAdapter.java

… request

The menu doesn't show up anymore after merging #4026. I assume this was unintentional since the Fragment still implements `MenuProvider`.

The option was removed in #3877, then forgotten to be added again in #4015. #4026 added it again, but took to long to merge, so we made #4225 as well and that is how we ended up with the option twice.

connyduck added 5 commits September 18, 2023 13:22

rename notifications package to systemnotifications

92cb7b6

add the "showNotificationsFilter" preference back

2a55757

Rename .java to .kt

cfc8a40

add new NotificationsFragment, implement NotificationsPagingAdapter

075f76b

notis

6dbd7c1

connyduck added 2 commits October 30, 2023 20:26

Merge branch 'develop' into refactor_notifications

be54fe4

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt # app/src/main/java/com/keylesspalace/tusky/fragment/NotificationsFragment.java # app/src/main/res/values/strings.xml

support filters

a1c1e76

charlag reviewed Nov 5, 2023

View reviewed changes

Merge branch 'develop' into refactor_notifications

9a821f1

connyduck mentioned this pull request Feb 28, 2024

Replace RxJava3 code with coroutines #4290

Merged

connyduck added 18 commits March 12, 2024 20:23

revert schema files

16267a6

fix build

47f2049

revert schema files

bdeb899

new hometimeline db schema

9b4becc

use same entities for home timeline and notifications

61a1932

update indizes and database cleanup

f229a41

fix room warnings

824c6a4

fix codeformatting in some places

d3b5bfe

implement some of the missing fragment functionality

aab880d

add fetching status from db back into ViewThreadViewModel

bedfe01

make clearing notifications work again

17ce0be

add preference change listener back

88d79cd

Merge branch 'develop' into refactor_notifications

6980bb3

improve menu/filter bar

4ce685b

add filter functionality back

bf3d55c

remove unused code

f077425

add missing license headers

f135672

connyduck added 3 commits April 18, 2024 20:55

use start/end instead of left/right in item_report_notification.xml

9882c27

rename mock* to fake*

7186d2e

refactor respondToFollowRequest to return Deferred

5ba10f2

connyduck requested review from cbeyls and Tak April 18, 2024 19:50

Tak approved these changes Apr 19, 2024

View reviewed changes

Goooler approved these changes Apr 19, 2024

View reviewed changes

cbeyls reviewed Apr 21, 2024

View reviewed changes

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt Outdated Show resolved Hide resolved

Tak mentioned this pull request Apr 26, 2024

3771 mark post as reply #3778

Open

connyduck added 10 commits April 29, 2024 20:00

Merge branch 'develop' into refactor_notifications

135e619

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/util/ShareShortcutHelper.kt

put back notification clearing and timestamp updating

e6a042d

Merge branch 'develop' into refactor_notifications

d74dd63

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/adapter/NotificationsAdapter.java

change payloads param to be non-null

c5633de

use SpannableString instead of SpannableStringBuilder

a78433a

CleanupDao -> DatabaseCleaner

a6e9fc1

improve NotificationViewData

3581e9a

don't specify executor when mapping/filtering pagingData

b3a8978

refresh notifications directly in viewmodel when responding to follow…

7f2e403

… request

refresh notifications directly in viewmodel when responding to follow…

abf0008

… request

connyduck merged commit b2c0b18 into tuskyapp:develop May 3, 2024
3 checks passed

cbeyls mentioned this pull request May 5, 2024

Re-enable options menu in NotificationsFragment #4419

Merged

connyduck pushed a commit that referenced this pull request May 10, 2024

Re-enable options menu in NotificationsFragment (#4419)

84fda7b

The menu doesn't show up anymore after merging #4026. I assume this was unintentional since the Fragment still implements `MenuProvider`.

connyduck deleted the refactor_notifications branch May 10, 2024 14:23

connyduck mentioned this pull request May 31, 2024

remove duplicate SHOW_NOTIFICATIONS_FILTER option #4481

Merged

connyduck mentioned this pull request Jun 16, 2024

Glitchy interface jumping/flickering bugs #4516

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor notifications to Kotlin & paging #4026

Refactor notifications to Kotlin & paging #4026

connyduck commented Sep 19, 2023 •

edited

Loading

connyduck commented Oct 30, 2023

charlag Nov 5, 2023 •

edited

Loading

charlag Nov 5, 2023

charlag commented Nov 5, 2023

connyduck commented Apr 18, 2024

Goooler Apr 19, 2024

Goooler Apr 19, 2024

Goooler Apr 19, 2024

Goooler Apr 19, 2024

Tak commented Apr 19, 2024

connyduck commented Apr 19, 2024

cbeyls Apr 21, 2024

cbeyls Apr 21, 2024

cbeyls Apr 21, 2024

cbeyls Apr 21, 2024

	interface NotificationActionListener {
	fun interface NotificationActionListener {

	interface NotificationsViewHolder {
	fun interface NotificationsViewHolder {

Refactor notifications to Kotlin & paging #4026

Refactor notifications to Kotlin & paging #4026

Conversation

connyduck commented Sep 19, 2023 • edited Loading

connyduck commented Oct 30, 2023

charlag Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag commented Nov 5, 2023

connyduck commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tak commented Apr 19, 2024

connyduck commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connyduck commented Sep 19, 2023 •

edited

Loading

charlag Nov 5, 2023 •

edited

Loading