[WIP] Base for push notification system using websockets #426
Conversation
|
The implementation should use a separate ws endpoint, and keep the downloader ws as is for now otherwise a breaking change will occur |
Yep this ws endpoint is different itself, it's just the notifying service has been extracted and made abstract so that the new WS for toasts and other notifications can reuse the code. The current change in downloader is backwards compatible. |
|
@meta-boy Are you not going to finish this PR? |
I will, just taking some time with Okhttp, never worked with it by itself, but only with something like retrofit. So taking my time to explore and write it in the best way possible. |
okhttp is for making requests, how does this PR even need sending requests? |
Isn't checking for new release updates also a part of this feature? That would mean querying the github api to check for new releases, which requires making an http call. |
Not really? You should instead focus on small incremental pull requests! bigger PRs take a long time to review and merge. Make numerous small PRs instead. |
oh then this is ready |
meta-boy
changed the title
meta-boy
changed the title
|
|
||
| // to be consumed in the client based on | ||
| // the event type in a notifications screen | ||
| fun queue() = eventQueue |
There was a problem hiding this comment.
I don't understand what this should be used for. Could you expand on what would be the content of "Notifications screen"?
| @@ -0,0 +1,8 @@ | |||
| package suwayomi.tachidesk.event.enums | |||
|
|
|||
| enum class EventType { | |||
There was a problem hiding this comment.
What would be some example events for these types?
| } | ||
|
|
||
| fun unqueue(chapterIndex: Int, mangaId: Int) { | ||
| downloadQueue.removeIf { it.mangaId == mangaId && it.chapterIndex == chapterIndex } | ||
| notifyAllClients() | ||
| val status = getStatus() | ||
| eventDispatcher.dequeue(status) |
There was a problem hiding this comment.
I don't think this would ever do anything. getStatus() always returns new Event which has new randomUUID(). dequeue(status) then look for this new id for removal.
Also, I don't think this unqueue is supposed to dequeue the status. unqueue is used for removing specific download job. It should notify client with new status (which will be missing the unqueued download task) which it does below. I'm not sure what this code is meant to do.
| downloader!!.start() | ||
| } | ||
|
|
||
| notifyAllClients() | ||
| eventDispatcher.notifyAllClients(getStatus()) |
There was a problem hiding this comment.
Could notifyAllClients() maybe have been left in and only its implementation changed to use eventDispatcher? It seems like this line is repeated a lot.
There was a problem hiding this comment.
I have several problems with this kind of design:
1- EventDispatcher should be a singleton service class and just do the part of handling of clients and message to them and provide a ws endpoint as the single channel for providing notifications, Each part of the code that wants to send a notification can inject the Event service class and call it when it wants to send a new notification or it could act like some message passing channel or a system job result pipeline for jobs that can't finish quickly like backup and restore, extension install and uninstall
2- Your Event data class couldn't serialize a notification properly it needs to change somehow
3- The EventType has no place here, we wouldn't have any of those event types, instead we'd need types like "download finished", "application update available", "backup finished", "backup restore successful", "backup restore failed with this error", "extension installed successfully", etc.
4- The design is tied to the downloader too tightly, it might eventually let us overhaul the downloader status system but for no means it has to replace the downloader status ws endpoint
5- The design might need to define it's own text protocol(like http for example) or allow for some kind of back and fourth message passing between a client and the server ( i.e. for requests and services that couldn't be done in one request and response cycle like source global search which would return each search result from each source when it's available)
Finally my ideas are not coherent and maybe this needs to be two ws endpoints, one for true notifications and one for multi step jobs and requests(?)
Understood, taking all points into consideration, let me come up with a proposal on how to actually implement this rather than jumping straight away to the code, this way we can identify issues much earlier |
AriaMoradi
changed the title
This abstracts the base websocket implementation to have a better base for notification system