Show number of unread messages #378

JcMinarro · 2024-09-30T23:33:59Z

The server doesn't provide an easy way to obtain the number of unread messages on every application, and the current implementation of the Android App didn't store the messages even in memory, so there wasn't a way to render that data.

To achieve it, the first I needed to do was create a mechanism to store data in memory. For that, I have created a repository that keeps data in memory and provides the list of Applications/Messages in a reactive approach, which helps us in the update process of the UI in real-time.

The Repository is updated directly from the WebSocket connection, so the messages don't need to be parsed on different parts and we keep a Single-Source-of-Truth.

After that, I created a new ViewModel for the MessageScreen in a more "Android Approach" and refactored the MessageActivity, keeping the previous features and adding the number of unread messages on the sidebar.

Old unused classes have been removed, as they are not needed anymore.

Fix #313

🎨 UI Changes

Add relevant screenshots

Before	After

…update data

…nread messages

jmattheis · 2024-10-06T17:48:41Z

app/src/main/kotlin/com/github/gotify/messages/MessagesActivity.kt

+                    setActionView(R.layout.action_menu_counter)
+                    actionView?.findViewById<TextView>(
+                        R.id.counter
+                    )?.text = applicationState.counterLabel


The current implementation of the Android App didn't store the messages even in memory, so there wasn't a way to render that data.

It does via viewModel.messages.get(appid).

Well, It is storing messages there, but this list is not populated when the app starts, then, if the user doesn't navigate to every app section, the list of messages is not updated.
Apart of that, the data within MessageStateHolder is not "announced" outside of the instance, and the ViewModel doesn't act as a ViewModel but as a "data provider" that you need to ask for data.

Well, It is storing messages there, but this list is not populated when the app starts, then, if the user doesn't navigate to every app section, the list of messages is not updated.

I get the last part, but the first part seems a little dishonest. The current implementation doesn't populate all messages from all apps because it doesn't need to. It sounds like "It's not possible with the current implementation, so there must be a rewrite", which I don't buy, changing the current implementation to load all messages from all apps should be simpler than wholly rewriting the message handling.

(not saying the current implementation should be adjusted like this, it only about the argumentation)

jmattheis · 2024-10-06T17:56:35Z

app/build.gradle

@@ -77,6 +77,8 @@ dependencies {
    implementation 'androidx.swiperefreshlayout:swiperefreshlayout:1.1.0'


The server doesn't provide an easy way to obtain the number of unread messages on every application

I dislike not having the correct count and only having at least x message there. This should be supported server side by returning the message count in the /application endpoint.

I agree with you, but the backend doesn't provide this info.

I don't want this feature to be implemented client side, there must be server side support for this. The refactoring can probably be included without the changed behavior for the message count.

jmattheis · 2024-10-06T18:07:12Z

app/build.gradle

@@ -77,6 +77,8 @@ dependencies {
    implementation 'androidx.swiperefreshlayout:swiperefreshlayout:1.1.0'
    implementation 'androidx.vectordrawable:vectordrawable:1.2.0'


Thanks for you contribution and your effort! Sadly as is, this PR is too big for me to accept. The refactoring of the messages look cool, but it IMO doesn't really fix something with the previous implementation, it's just a different implementation that uses the StateFlow stuff.

I'm not against refactoring, but it's basically a switch from battle tested code, that's running like this for multiple years to something that will have bugs that will fall back onto me.

The new Implementation provides multiple things:

A single SourceOfTruth where the data is stored/obtained

A reactive approach that updates the UI in real time whenever any data is updated

A clean separation of responsibility (eg: In the previous one, the UI wasn't only used to render the UI but even parse the API data that was received from the WS Connection)

A Real MVVM implementation that allows the UI to subscribe to new data from the VM and the VM to be agnostic to what the View does.

I know the PR is a bit bigger, and just because I knew it, I split it into multiple commits that can be merged 1 by 1, keeping the app working as the previous commit worked.
If you want, I can split the PR into smaller PRs If it helps you on the review process

jmattheis

This was just a quick glance, I'll have to test this more in depth with different android api versions, and my last linux/android studio update broke my emulators, so it'll take some time.

jmattheis · 2024-10-11T20:06:42Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+ * @property scope The coroutine scope where the repository will run.
+ * @property baseUrl The base url of the server used to create the full url of the images.
+ * @property applicationApi The application api used to interact with the server.
+ * @property messageApi The message api used to interact with the server.


Can you remove all comments here, I don't think the provide much value as most of the stuff can be already read from the method / var signature.

jmattheis · 2024-10-11T20:11:39Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+ * @property applicationApi The application api used to interact with the server.
+ * @property messageApi The message api used to interact with the server.
+ */
+class Repository


Every delete create a call to /application

jmattheis · 2024-10-11T20:13:40Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+ * @property messageApi The message api used to interact with the server.
+ */
+class Repository
+private constructor(


The previous implementation had info logging for "deleting message with id xy", "Loading more messages for appid" and concrete error logging when the fetching failed e.g "failed requesting messages", "Failed to delete message". These should be readded.

And the println("JcLog should be removed.

jmattheis · 2024-10-11T20:15:18Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+
+    companion object {
+
+        private const val LIMIT = 20


Revert to the old limit of 100.

jmattheis · 2024-10-11T20:17:01Z

app/src/main/kotlin/com/github/gotify/Settings.kt

+        operator fun invoke(context: Context): Settings = Settings(
+            sharedPreferences = context.getSharedPreferences("gotify", Context.MODE_PRIVATE)
+        )


Write this as second constructor:

constructor(c: Context) : this(c.getSharedPreferences("gotify", Context.MODE_PRIVATE))

jmattheis · 2024-10-11T20:18:31Z

app/src/main/kotlin/com/github/gotify/messages/MessagesViewModel.kt

+    /**
+     * Whether the messages are currently being refreshed.
+     */
+    val refreshing = _refreshing


this should probably remove mutability.

Suggested change

val refreshing = _refreshing

val refreshing: StateFlow<Boolean> = _refreshing

jmattheis · 2024-10-11T20:21:09Z

app/src/main/kotlin/com/github/gotify/messages/MessagesActivity.kt

@@ -171,52 +201,56 @@ internal class MessagesActivity :

    private fun refreshAll() {
        CoilInstance.evict(this)
-        startActivity(Intent(this, InitializationActivity::class.java))
-        finish()
+        messagesViewModel.onRefreshAll()


Revert this back to the cal to the InitializationActivity, as this will also reset the WebSocketConnection.

jmattheis · 2024-10-11T20:24:31Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+ */
+class Repository
+private constructor(
+    private val scope: CoroutineScope,


The currently selected application isn't highlighted in the navigation drawer.

noselect.webm

jmattheis · 2024-10-11T20:30:26Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+            false -> emptyList()
+            true -> messageApi.getAppMessages(application.id, limit, currentPagingState.since)
+                .awaitResponse().takeIf { it.isSuccessful }?.body()
+                ?.also { paging.value += (application.id to it.paging.toPagingState()) }?.also {


Most of the .also in this class make this hard to read, especially if it also nested in also. I'd prefer putting these into variables and then using them without nesting.

jmattheis · 2024-10-11T20:31:52Z

app/src/main/kotlin/com/github/gotify/Repository.kt

+                (
+                    appId to (
+                        (
+                            previous.associateBy {
+                                it.id
+                            } + messages.associateBy {
+                                it.id
+                            }
+                            ).values.sortedByDescending {
+                            it.id
+                        }
+                        )
+                    )


This is difficult to understand, due to the weird formatting and many braces.

JcMinarro added 9 commits October 1, 2024 01:38

Upgrade Retrofit dependency to version 2.9.0

ddf88c9

Add coroutine dependency

b337a71

Refactor Settings class

0ca9b12

Create a repository class to provide reactive data

2fb1e99

Keep a weak reference to the Repository within WebSocketService to …

8d59a90

…update data

Create a new ViewModel to be used on the Messages Screen

799d150

Create a view to show number of unread messages

84d3edf

Refactor MessageActivity to use the Repository and show number of u…

940b6ef

…nread messages

Remove unneeded classes

c32c34c

JcMinarro force-pushed the refactor-messages branch from fd8be94 to c32c34c Compare September 30, 2024 23:40

jmattheis reviewed Oct 6, 2024

View reviewed changes

jmattheis requested changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show number of unread messages #378

Show number of unread messages #378

JcMinarro commented Sep 30, 2024

jmattheis Oct 6, 2024

JcMinarro Oct 6, 2024

jmattheis Oct 11, 2024

jmattheis Oct 6, 2024

JcMinarro Oct 6, 2024

jmattheis Oct 11, 2024

jmattheis Oct 6, 2024 •

edited

Loading

JcMinarro Oct 6, 2024

jmattheis left a comment

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

jmattheis Oct 11, 2024

		@@ -77,6 +77,8 @@ dependencies {
		implementation 'androidx.swiperefreshlayout:swiperefreshlayout:1.1.0'

		@@ -77,6 +77,8 @@ dependencies {
		implementation 'androidx.swiperefreshlayout:swiperefreshlayout:1.1.0'
		implementation 'androidx.vectordrawable:vectordrawable:1.2.0'

	val refreshing = _refreshing
	val refreshing: StateFlow<Boolean> = _refreshing

Show number of unread messages #378

Are you sure you want to change the base?

Show number of unread messages #378

Conversation

JcMinarro commented Sep 30, 2024

🎨 UI Changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmattheis Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmattheis left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmattheis Oct 6, 2024 •

edited

Loading