fix: No toast on removal of untrusted mark #16972
Conversation
Jenkins BuildsClick to see older builds (35)
|
caybro
force-pushed
the
16949-no-toast-on-removal-of-untrusted-mark
branch
2 times, most recently
from
3d29949 to
4501e9e
Compare
caybro
force-pushed
the
16949-no-toast-on-removal-of-untrusted-mark
branch
from
4501e9e to
044f802
Compare
caybro
force-pushed
the
16949-no-toast-on-removal-of-untrusted-mark
branch
2 times, most recently
from
83d5fb2 to
d9006a7
Compare
| const displayName = SQUtils.Emoji.parse(ProfileUtils.displayName(contactDetails.localNickname, contactDetails.name, contactDetails.displayName, contactDetails.alias)) | ||
| Global.displaySuccessToastMessage(qsTr("Trust mark removed for %1").arg(displayName)) | ||
| } | ||
| } |
There was a problem hiding this comment.
There are some problems and several potential solutions there.
- According to the guide, stores are just thin wrapper over context properties exposed from Nim. There should be no UI interaction from stores but displaying toast message definitely is.
So, the ContactsStore should have signal trustStatusRemoved(...), and store should just re-emit the signal from contactsModuleInst, e.g. via d.contactsModuleInst.trustStatusRemoved.connect(root.trustStatusRemoved).
The actual handling of that signal should be done outside of the store, ideally in some places dedicated to toasts generation.
-
The target is to eliminate
Utils.getContactDetailsAsJsonbecause we have everything in thecontactsModel. There is even no need to useProfileUtils.displayNamebecause there isPreferredDisplayNameproviding the same thing directly from the model. So instead ofgetContactDetailsAsJsonjustModelUtils.getByKey(...)forpreferredDisplayNamerole can be used there. -
We could consider handling such notifications exclusively on the UI side by observing the model and filtering the operations we want to notify to the user via toasts. Then no specific signal from the store is needed.
There was a problem hiding this comment.
Yeah good ideas; I'll just probably emit a signal here, and catch it in the ToastsManager. One potential pitfall, the ContactsStore is not a toplevel one, it's created as a substore of RootStore
Option nr. 3 is something I had on mind too but seemed too complex to do here just for one action. The other (complexity) problem is that the proposed solution is simply incomplete: if you look at Popups.qml, we don't want to display a series of notifications as a result of performing single backend calls, but rather combine and group them in case we're performing several actions (cf Popups.qml, e.g. the noitifications inside RemoveContactPopup)
There was a problem hiding this comment.
One thing that just popped in my mind, is there a chance that we initialized more than one ContactsStore? If so, then we'd fire two toast messages?
That might be an argument to move the Connections to another place, though I don't have an idea where it should be 🤷
There was a problem hiding this comment.
Yeah, if I understood correctly, it's another reason to decouple toast creation from the place where the action is called in UI. By observing model we can register events and emit notifications using Qt.callLater or even some delay, where the only the last call will be actually executed, aggregating all events into single nofification.
There was a problem hiding this comment.
@jrainville that's possible and it would lead to undesired behavior. But first thing is that store shouldn't do that to respect separation of concerns and the architecture we established via the guideline.
There was a problem hiding this comment.
So far it looks like the ContactsStore is really instantiated only once as part of the toplevel RootStore :) But one can never be too sure :D
There was a problem hiding this comment.
Got me thinking, would be a nice test for the stores if we don't instantiate them multiple times 😆 Here it works, otherwise we'd get multiple notifications
- listen to the NIM's signal `SIGNAL_REMOVED_TRUST_STATUS` - emit a signal for QML signal accordingly - emit a toast/notification as a result Fixes #16949
caybro
force-pushed
the
16949-no-toast-on-removal-of-untrusted-mark
branch
from
d9006a7 to
840fd78
Compare
What does the PR do
SIGNAL_REMOVED_TRUST_STATUSFixes #16949
Affected areas
Profile
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Marked as untrusted:
Removed the untrusted mark:
