-
-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show number of unread messages #378
Changes from 9 commits
ddf88c9
b337a71
0ca9b12
2fb1e99
8d59a90
799d150
84d3edf
940b6ef
c32c34c
be2684a
b6f3e49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,8 @@ dependencies { | |
implementation 'androidx.swiperefreshlayout:swiperefreshlayout:1.1.0' | ||
implementation 'androidx.vectordrawable:vectordrawable:1.2.0' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new Implementation provides multiple things:
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. |
||
implementation 'androidx.preference:preference-ktx:1.2.1' | ||
implementation 'androidx.lifecycle:lifecycle-viewmodel-ktx:2.8.3' | ||
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1' | ||
|
||
implementation 'com.github.cyb3rko:QuickPermissions-Kotlin:1.1.3' | ||
implementation "io.coil-kt:coil:$coil_version" | ||
|
@@ -94,6 +96,5 @@ dependencies { | |
configurations { | ||
configureEach { | ||
exclude group: 'org.json', module: 'json' | ||
exclude group: 'androidx.lifecycle', module: 'lifecycle-viewmodel-ktx' | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,320 @@ | ||
package com.github.gotify | ||
|
||
import com.github.gotify.api.ClientFactory | ||
import com.github.gotify.client.api.ApplicationApi | ||
import com.github.gotify.client.api.MessageApi | ||
import com.github.gotify.client.model.Application | ||
import com.github.gotify.client.model.Message | ||
import com.github.gotify.client.model.Paging | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.flow.MutableStateFlow | ||
import kotlinx.coroutines.flow.SharingStarted | ||
import kotlinx.coroutines.flow.StateFlow | ||
import kotlinx.coroutines.flow.combine | ||
import kotlinx.coroutines.flow.distinctUntilChanged | ||
import kotlinx.coroutines.flow.map | ||
import kotlinx.coroutines.flow.stateIn | ||
import kotlinx.coroutines.launch | ||
import okhttp3.HttpUrl.Companion.toHttpUrlOrNull | ||
import retrofit2.awaitResponse | ||
|
||
/** | ||
* A class that represents the repository. | ||
* | ||
* @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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
*/ | ||
class Repository | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
private constructor( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
private val scope: CoroutineScope, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The currently selected application isn't highlighted in the navigation drawer. noselect.webm |
||
private val baseUrl: String, | ||
private val applicationApi: ApplicationApi, | ||
private val messageApi: MessageApi | ||
) { | ||
private val applications = MutableStateFlow<List<Application>>(emptyList()) | ||
private val messages = MutableStateFlow<Map<Long, List<Message>>>(emptyMap()) | ||
private val paging = MutableStateFlow<Map<Long, PagingState>>(emptyMap()) | ||
private var allApplicationsPaging: PagingState = PagingState() | ||
|
||
/** | ||
* Get all applications. | ||
* It returns a [StateFlow] object that emits the list of all applications with their states | ||
* whenever the list changes. | ||
*/ | ||
val applicationsState: StateFlow<List<ApplicationState>> = | ||
combine(applications, messages, paging) { apps, messages, paging -> | ||
apps.map { app -> | ||
val unreadCount = messages[app.id]?.size ?: 0 | ||
val hasMoreMessages = paging[app.id]?.hasMore ?: true | ||
ApplicationState( | ||
app, | ||
unreadCount, | ||
hasMoreMessages, | ||
app.image?.let { | ||
( | ||
it.toHttpUrlOrNull() ?: baseUrl.toHttpUrlOrNull()?.newBuilder() | ||
?.addPathSegment(it)?.build()?.toString() | ||
)?.toString() | ||
} | ||
) | ||
}.also { println("JcLog: ${this@Repository} number of applications -> ${it.size}") } | ||
}.stateIn(scope = scope, started = SharingStarted.Lazily, initialValue = emptyList()) | ||
|
||
/** | ||
* Get all messages of all applications. | ||
* It returns a [StateFlow] object that emits the list of all messages whenever the list changes. | ||
*/ | ||
val allMessages: StateFlow<List<Message>> = | ||
messages.map { it.values.flatten() }.distinctUntilChanged() | ||
.stateIn(scope = scope, started = SharingStarted.Lazily, initialValue = emptyList()) | ||
|
||
/** | ||
* Get messages of an application. | ||
* It returns a [StateFlow] object that emits the list of messages of the application | ||
* whenever the list changes. | ||
* | ||
* @param application The application. | ||
* | ||
* @return The list of messages of the application. | ||
*/ | ||
fun getMessages(application: Application): StateFlow<List<Message>> = | ||
messages.map { it[application.id] ?: emptyList() }.distinctUntilChanged() | ||
.stateIn(scope = scope, started = SharingStarted.Lazily, initialValue = emptyList()) | ||
|
||
/** | ||
* Fetch all applications. | ||
* | ||
* @return The list of the fetched applications. | ||
*/ | ||
suspend fun fetchApps(): List<Application> = | ||
applicationApi.getApps().awaitResponse().takeIf { it.isSuccessful }?.body() | ||
?.also { applications.value = it } ?: emptyList() | ||
|
||
/** | ||
* Fetch messages from an application. | ||
* | ||
* @param application The application. | ||
* @param limit The number of messages to fetch. | ||
* | ||
* @return The list of the fetched messages. | ||
*/ | ||
suspend fun fetchMessages(application: Application, limit: Int = LIMIT): List<Message> { | ||
val currentPagingState = paging.value[application.id] ?: PagingState() | ||
return when (currentPagingState.hasMore) { | ||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
println("JcLog: pagin.since -> ${it.paging.since}") | ||
println("JcLog: pagin.next -> ${it.paging.next}") | ||
println("JcLog: lastMessage -> ${it.messages.lastOrNull()?.id}") | ||
}?.also { storeMessages(it.messages, currentPagingState.since == 0L) }?.messages | ||
?: emptyList() | ||
} | ||
} | ||
|
||
/** | ||
* Fetch messages from all applications. | ||
* | ||
* @param limit The number of messages to fetch. | ||
* | ||
* @return The list of the fetched messages. | ||
*/ | ||
suspend fun fetchMessages(limit: Int = LIMIT): List<Message> { | ||
return when (allApplicationsPaging.hasMore) { | ||
false -> return emptyList() | ||
true -> messageApi.getMessages(limit, allApplicationsPaging.since).awaitResponse() | ||
.takeIf { it.isSuccessful }?.body() | ||
?.also { allApplicationsPaging = it.paging.toPagingState() } | ||
?.also { storeMessages(it.messages, false) }?.messages ?: emptyList() | ||
} | ||
} | ||
|
||
/** | ||
* Delete a message. | ||
* | ||
* @param message The message to delete. | ||
* | ||
* @return A boolean value that indicates if the message was deleted. | ||
*/ | ||
suspend fun deleteMessage(message: Message): Boolean = | ||
messageApi.deleteMessage(message.id).awaitResponse().isSuccessful.also { | ||
if (it) { | ||
storeMessages( | ||
messages.value.flatMap { | ||
it.value.filterNot { it.id == message.id } | ||
}, | ||
true | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* Delete all messages of all applications. | ||
* | ||
* @return A boolean value that indicates if the messages were deleted. | ||
*/ | ||
suspend fun deleteAllMessages(): Boolean = | ||
messageApi.deleteMessages().awaitResponse().isSuccessful.also { | ||
if (it) messages.value = emptyMap() | ||
} | ||
|
||
/** | ||
* Delete all messages of an application. | ||
* | ||
* @param application The application. | ||
* | ||
* @return A boolean value that indicates if the messages were deleted. | ||
*/ | ||
suspend fun deleteAllMessages(application: Application): Boolean = | ||
messageApi.deleteAppMessages(application.id) | ||
.awaitResponse().isSuccessful.also { if (it) messages.value -= application.id } | ||
|
||
/** | ||
* Store a message in the repository. | ||
* | ||
* @param newMessage The message to store. | ||
*/ | ||
fun storeMessage(newMessage: Message) { | ||
storeMessages(listOf(newMessage), false) | ||
} | ||
|
||
/** | ||
* Store messages in the repository. | ||
* | ||
* @param newMessages The list of messages to store. | ||
* @param overrideOld A boolean value that indicates if the old messages should be overridden. | ||
*/ | ||
private fun storeMessages(newMessages: List<Message>, overrideOld: Boolean) { | ||
fetchMissingApplications(newMessages) | ||
val previousMessages = messages.value.takeUnless { overrideOld } ?: emptyMap() | ||
val mappedMessages: Map<Long, List<Message>> = | ||
newMessages.groupBy { it.appid }.map { (appId, messages) -> | ||
val previous = previousMessages[appId] ?: emptyList() | ||
( | ||
appId to ( | ||
( | ||
previous.associateBy { | ||
it.id | ||
} + messages.associateBy { | ||
it.id | ||
} | ||
).values.sortedByDescending { | ||
it.id | ||
} | ||
) | ||
) | ||
Comment on lines
+193
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is difficult to understand, due to the weird formatting and many braces. |
||
}.toMap() | ||
messages.value += mappedMessages | ||
} | ||
|
||
/** | ||
* Fetch applications that are missing from the list of messages. | ||
* | ||
* @param messages The list of messages. | ||
*/ | ||
private fun fetchMissingApplications(messages: List<Message>) { | ||
scope.launch { | ||
val appIds = applications.value.map { it.id } | ||
[email protected] { messages.all { appIds.contains(it.id) } }?.fetchApps() | ||
} | ||
} | ||
|
||
/** | ||
* Refresh all messages of all applications. | ||
* | ||
* @return The list of refreshed messages. | ||
*/ | ||
suspend fun refreshAllMessages() { | ||
applications.value.forEach { app -> | ||
refreshMessages(app) | ||
} | ||
allApplicationsPaging = PagingState() | ||
fetchMessages() | ||
} | ||
|
||
/** | ||
* Refresh the messages of an application. | ||
* | ||
* @param application The application to refresh. | ||
* | ||
* @return The list of refreshed messages. | ||
*/ | ||
suspend fun refreshMessages(application: Application): List<Message> { | ||
paging.value -= application.id | ||
return fetchMessages(application, messages.value[application.id]?.size ?: LIMIT) | ||
} | ||
|
||
/** | ||
* Initialize the repository. Fetch all applications and their last messages. | ||
*/ | ||
private suspend fun initialize() { | ||
fetchApps().forEach { app -> | ||
fetchMessages(app) | ||
} | ||
fetchMessages() | ||
} | ||
|
||
/** | ||
* Delete an application. | ||
* | ||
* @param app The application to delete. | ||
* | ||
* @return A boolean value that indicates if the application was deleted. | ||
*/ | ||
suspend fun deleteApp(app: Application): Boolean = applicationApi.deleteApp(app.id) | ||
.awaitResponse().isSuccessful.also { if (it) applications.value -= app } | ||
|
||
companion object { | ||
|
||
private const val LIMIT = 20 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert to the old limit of 100. |
||
|
||
/** | ||
* Create a new instance of the repository. The repository will be initialized after creation. | ||
* | ||
* @param scope The coroutine scope. | ||
* @param settings The settings. | ||
*/ | ||
internal fun create(scope: CoroutineScope, settings: Settings): Repository { | ||
val apiClient = ClientFactory.clientToken(settings) | ||
val applicationApi = apiClient.createService(ApplicationApi::class.java) | ||
val messageApi = apiClient.createService(MessageApi::class.java) | ||
return Repository(scope, settings.url, applicationApi, messageApi).also { | ||
scope.launch { it.initialize() } | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* A function that converts a [Paging] object to a [PagingState] object. | ||
*/ | ||
private fun Paging.toPagingState(): PagingState = PagingState(since, next != null) | ||
|
||
/** | ||
* A class that represents the state of the paging. | ||
* | ||
* @property since The older message id value of the paging. | ||
* @property hasMore A boolean value that indicates if there are more messages. | ||
*/ | ||
private data class PagingState( | ||
val since: Long = 0, | ||
val hasMore: Boolean = true | ||
) | ||
|
||
/** | ||
* A class that represents the state of the application. | ||
* | ||
* @property application The application. | ||
* @property unreadCount The number of unread messages. | ||
* @property hasMoreMessages A boolean value that indicates if there are more messages. | ||
* @property iconUrl The icon url of the application. | ||
*/ | ||
data class ApplicationState( | ||
val application: Application, | ||
val unreadCount: Int, | ||
val hasMoreMessages: Boolean, | ||
val iconUrl: String? | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, but the backend doesn't provide this info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.