status-im / status-desktop

Status Desktop client made in Nim & QML
https://status.app
Mozilla Public License 2.0
276 stars 76 forks source link

Toasts - Analyse toasts management in qml layer and explore if we can have / or already have a central manager as well in nim backend #12661

Open noeliaSD opened 8 months ago

noeliaSD commented 8 months ago

Description

Nowadays we don't have central point to manage Toasts at least from the qml layer and it's not obvious to track which are the current existing toasts.

Not sure which is the current state on the nim site but this task pretends to analyse also this layer and provide an unified management solution.

A first step has been already done in here where a new ToastsManager.qml class has been created and there will be a single object instance of this class managed by appMain where all toasts management will be centralized. The shared pr creates the new manager object and adds toasts logic for just one specific context (community transfer ownership related toasts). Next steps needed in here will be:

  1. Collect all Global.displayToastMessage calls in qml files and refactor them to be called from the new toastsManager object instantiated in AppMain.qml. In case the new ToastsManager file grows considerably, consider creating different toasts sub-managers per topic / context and just instantiate them in the main one.
  2. Review ephemeral_notifcation_item/model analysing the current toasts item's needs. Now these toasts will also trigger some actions apart from opening an external url. We need to take a look at this definition and provide an unified and generic solution to deal with these actions too (previous pr shared introduces 2 different roles actionType and actionData but there's already a details role that can, maybe, be useful, so just give it another thought and propose an unified solution.
  3. Review src/app/modules/main/view.nim api. It would make sense to unify it or, on the contrary, create specific calls for specific needs / params. The cleaner and more readable, the better. It doesn't worth it, neither makes it readable, to have just one method to create a toast with 6 parameters, and only use 2 or 3 parameters each time it's called, passing "null" data to the rest of params.
  4. Review src/app/modules/main/module.nim. It's the main app module and contains a lot of logic in it. Would it make sense to create a sub-module specific for toasts and remove "weight" from the main module?
  5. Analyse how the rest of toasts are triggered from the nim site. There are probably different situations and sometimes it can be the need of directly request a toast from the UI as per design, and there's no backend logic needed for this specific scenario but in the major part of the cases, the toast (or at least from my point of view) should be ONLY driven by the backend and we should try to avoid the current ping-pong flow that doesn't seem to have a lot of sense, neither adding value. The flow described is: backend signal to trigger toast --> UI connects to it and calls to backend to add the toast to the corresponding model --> backend recives the request and updates the model accordingly --> finally UI displays model updates I guess that this ping-pong flow has been introduced mainly to deal with translations, since we cannot or do not want to manage them from nim. However, there may be other solutions than those described above to solve this problem. For example: we can create a toast messages dictionary with a key and the corresponding text. So nim backend is only responsible for working and providing keys and the UI will be responsible for the final visualization. Then we could end up with this flow: backend signal to trigger toast that updates model --> finally UI displays model updates

These are some bullet points and proposals, but it's just an open discussion.

noeliaSD commented 8 months ago

Take into consideration concerns / comments posted in here: https://github.com/status-im/status-desktop/pull/12645#pullrequestreview-1724812945

iurimatias commented 5 months ago

moved to 2.29 due to lack of space in this milestone, however we might move this back for the stabilization period

noeliaSD commented 4 months ago

It would probly make sense to give it a thought to the Activity Notifications at the same time we start thinking on the Ephemeral Notifications.

noeliaSD commented 4 months ago

There were some discussions done with the team and some initial investigation on the existing code. Then it's been postponed since other priority tasks came up.

Moved again to the next milestone until we have slot for technical debt work.

noeliaSD commented 1 month ago

This is a good candidate for stabilization phases.