New ChatView: Support incoming updates of messages#1426
Conversation
|
Most of these notifications have a comment, explaining why
Only Background: We don't want incoming stanzas to have side effects through notifications while we are still inside the db transaction wrapping this stanza processing. So We use Incoming stanza processing is one place, we use that context manager to queue notifications. Another one is processing history fetches from MAM. We use it to suppress all notifications for "new" messages in this case and clean the queue instead of letting the notifications "bubble up". There are only a few places (those you found) where we don't want to queue notifications and always handle them immediately. |
Yes, that makes sense :)
No, that's not needed, you can remove that one :) |
| func findAndUpdateMessage(with message: MLMessage) { | ||
| if message.isEqual(to: self.contact.obj) { | ||
| for index in messages.indices { | ||
| if messages[index].message.messageDBId == message.messageDBId { |
There was a problem hiding this comment.
I think, the proper implementation is to extend MLMessage to be a full fledged model class handling updates itself (and being a per-db-id singleton), just like MLContact.
Then use the KVO wrapper to automatically update the UI.
There was a problem hiding this comment.
The problem is, if I just update the MLMessage object, for example:
if messages[index].message.messageDBId == message.messageDBId {
messages[index].message.update(with: message)Then SwiftUI doesn't detect that the messages @State variable changed, and doesn't re-render the view.
By inserting a fresh new entry in the messages array, SwiftUI figures this out correctly and updates the view. (the live update requires a SwiftPM dependencies bump to work correctly. But I did that in my other PR)
There was a problem hiding this comment.
Do I understand correctly, that you can update this PR to use the ObservableKVOWrapper and let MLMessage be a proper model class, once I merge your other PR?
There was a problem hiding this comment.
I think the following can work:
In the ChatViewMessage class, we make the MLMessage property an ObservedObject (using KVOObserver for example).
Then in that same class, we listen for changes to the MLMessage property, and when it does change, we update the properties of the class (the ones that are currently only set in the initializer e.g. the text property)
There was a problem hiding this comment.
Why not just override the properties of the base class in our ChatViewMessage class using computed properties? The could compute the right values using the MLMessage and even properly propagate property changes of the MLMessage properties up the chain (using the ObservableKVOWrapper or something similar).
@matthewrfennell what do you think? imho this should be possible, so that changing the messageText property inside an MLMessage should automatically bubble up the chain all the way to swiftui consuming that message text.
There was a problem hiding this comment.
This makes sense to me, I haven't tried it, but I think it should work
There was a problem hiding this comment.
Yes, that should work, but it's not the question/solution I proposed. I want the MLMessage be a singleton over the db id, just like MLContact is a singleton over the (jid, accountid) tuple. Then The MLMessage should listen for all update notifications and change the values of its properties according to these updates.
The rest of our code in ChatView.swift (etc.?) should make sure that these property updates of the underlying MLMessage are automatically picked up by SwiftUI using our ObservableKVOWrapper etc. (just like in the contact details + MLContact case).
@matthewrfennell My question is now: could you (together with lissine) figure out, what changes/additions etc. need to be done to the ChatViewMessage class (and of course MLMessage) to realize my vision depicted above.
There was a problem hiding this comment.
Additional note: to test, you could use a timer (createTimer(1, (^{ ... }));) in MLMessage to automatically add a dot to the end of every message every second. That should automatically trigger an UI update, if everything works out as intended :)
That was my intuition, but for some reason |
Well, a retraction should update the unread counter. I think we should set the |
Sorry, my fault, of course I meant |
tmolitor-stud-tu
force-pushed
the
develop
branch
8 times, most recently
from
d79dd24 to
0991475
Compare
tmolitor-stud-tu
force-pushed
the
develop
branch
7 times, most recently
from
cbcb0d2 to
4544425
Compare
tmolitor-stud-tu
force-pushed
the
develop
branch
2 times, most recently
from
62cf0df to
1c354de
Compare
tmolitor-stud-tu
force-pushed
the
develop
branch
2 times, most recently
from
21b949a to
d90c975
Compare
tmolitor-stud-tu
force-pushed
the
develop
branch
2 times, most recently
from
d168ab8 to
0f4c75d
Compare
Note that doing this will change the position of the "Unread Messages Below" indicator to under the most recent retracted message. |
lissine0
force-pushed
the
message-updates
branch
from
c040955 to
42c2c60
Compare
Oh, that's an unintended side effect I didn't think of. That's unfortunate. Other than that: is this PR ready to be merged? |
We already have a
It's only missing the handling of Status message update notifications (message received, displayed, error etc.) in MLMessage |
|
Note that the changes in ChatView.swift depend on changes in the ExyteChat fork: the |
@matthewrfennell will we be able to get this change upstream? |
I don't think so, as I made internal structures public. |
Oh great! Then only the calculation of the unread counter has to be updated to take that field into account (or does it already?).
Okay, would you like to create a new PR for these status notifications and me to do a full review and merge this PR now? Or do you want me to review and merge this only after the status handling is complete? |
Unfortunately, the situation is a bit complicated.
I decided against the refactor. So, implementing the missing handling of those notifications shouldn't take a lot time. Thus I'd like to include it in this PR |
|
Hmm we'll have to split the counting for the unread badge from the counting for the unread below marker then. Two datalayer methods instead of one. |
lissine0
force-pushed
the
message-updates
branch
from
42c2c60 to
970f11e
Compare
lissine0
force-pushed
the
message-updates
branch
from
970f11e to
b19756f
Compare
tmolitor-stud-tu
left a comment
tmolitor-stud-tu
left a comment
There was a problem hiding this comment.
That's a full review of all changes now :)
| let dbMessages = DataLayer.sharedInstance().messages(forContact:contact.contactJid, forAccount:contact.accountID) as! [MLMessage] | ||
| for msg in dbMessages { | ||
| messages.append(ChatViewMessage(msg)) | ||
| messages.append(ChatViewMessage(ObservableKVOWrapper(msg))) |
There was a problem hiding this comment.
Maybe don't wrap it into ObservableKVOWrapper outside, but wrap it inside the constructor of ChatViewMessage. That seems a bit cleaner because it doesn't expose these implementation details to the caller.
There was a problem hiding this comment.
Doing that causes the following error on L502:
message: ChatViewMessage(message),
^^^^^^^
Cannot convert value of type 'ObservableKVOWrapper<MLMessage>' to expected argument type 'MLMessage'
If I replace message with message.obj, the automatic view updates won't work anymore
There was a problem hiding this comment.
No, you should change the code to "bubble that change up", aka: only wrap the MLMessage object into the kvo observer once and pass around the kvo observer or some object containing it rather then recreating things over and over again on every render.
| @ObservedObject var viewModel: ExyteChat.ChatViewModel | ||
| let positionInUserGroup: PositionInUserGroup | ||
| let positionInMessagesSection: PositionInMessagesSection | ||
| init(message: ObservableKVOWrapper<MLMessage>, viewModel: ChatViewModel, positionInUserGroup: PositionInUserGroup, positionInMessagesSection: PositionInMessagesSection) { |
There was a problem hiding this comment.
Why don't you pass in the ChatViewMessage here? You can get the underlying MLMessage from it just fine and you won't have to instantiate a new ChatViewMessage in the view body every time this gets rendered.
There was a problem hiding this comment.
I tested this suggestion and the view auto updates stopped working.
If the initializer of ChatViewMessage doesn't run on every view render, how are changes from MLMessage going to be picked up?
There was a problem hiding this comment.
They are going to be picked up by the ObservableKVOWrapper which is emitting a objectWillChange event if a property of MLMessage (being used by the swiftui view) changes. That event is used by swiftui as a rerender signal.
There was a problem hiding this comment.
if a property of MLMessage (being used by the swiftui view) changes
The MessageView isn't using any MLMessage properties directly.
The MessageView uses ChatViewMessage; and ChatViewMessage gets initialized with MLMessage properties.
Thus the need for the initializer of ChatViewMessage to run in order to get updates from MLMessage
There was a problem hiding this comment.
We need to write our custom MessageView, and use MLMessage properties in it directly, in order for it to work how you said
There was a problem hiding this comment.
Yes, I alreay said in some review that you should overwrite the properties of Message in ChatViewMessage using computed properties returning the values of MLMessage and use the MLMessage as @StateObject. That should work.
See #1426 (comment)
That means something like this (maybe you could try a proof of concept first, before updating this PR to save some work in case it doesn't work like depicted below):
- A view using an
@StateObjectto a class namedChatViewMessageand using itstextproperty in the view body - The
ChatViewMessageclass being a child of the ExyteChatopen class Message: ObservableObject, Identifiable - The
ChatViewMessageclass having a propertyinnerMessageof typeObservableKVOWrapper<MLMessage> - The
ChatViewMessageclass having an@Publishedcomputed property of typeStringnamedtext, that always returns theinnerMessage.messageTextvalue. This property overwrites the@Published public var text: Stringproperty of the base class
Okay, step 4 is a problem here, apparently you cannot overwrite a stored property with a computed one :(
I hate Swift, in ObjC that could easily be done :(
Solution:
- Check if the proof of concept above works while not deriving the
ChatViewMessageclass from anything. (Only check if the computed property properly forwards the change event of theObservableKVOWrapperwrappedMLMessageobject (use some timer to periodically change the text). - If that works, then either:
- Change the stored properties in the base class to be computed ones (computed ones can be overwritten).
- Update the ExyteChat to define a
protocolnamedMessageProtocolthat defines all the properties of the ExyteChatMessageclass/struct; let the ExyteChatMessagederive from that protocol and change all mentions ofMessagein the ExyteChat codebase to beMessageProtocolones; then derive our implementation from that protocol, too, instead of deriving it from theMessageclass.
I think introducing a new protocol is the way to go here, because it makes the original ExyteChat stuff more extensible but doesn't change any behavior. I think we can even upstream that change (together with making Message a class rather than a struct).
btw: the public enum Status doesn't contain a state for received, too, so we'll have to update the base class anyways.
There was a problem hiding this comment.
@matthewrfennell what do you think, would that protocol be upstreamable?
| _singletonCache = [NSMutableDictionary new]; | ||
| } | ||
|
|
||
| +(MLMessage*) createMessageFromHistoryID:(NSNumber*) historyID |
There was a problem hiding this comment.
So we have MLMessage messageFromDictionary: which is used in various places by the DataLayer and this method here. That's a duplication I don't like. Especially because one of them has a cache and the other one doesn't. In MLContactfor example, the MLContact contactFromDictionary: method is only called internally in MLContact and isn't exposed publicly.
I know you want to preserve all these high level DataLayer methods returning ready made MLMessage objects and that's a good decision. But then the MLMessage createMessageFromHistoryID: method should be removed and the cache added to MLMessage messageFromDictionary: . Every code currently calling into createMessageFromHistoryID should use the appropriate DataLayer method which is messageForHistoryID:.
Even better: move that DataLayer messageForHistoryID: method to MLMessage and name it createMessageFromHistoryID: (that's possible because this DataLayer method doesn't use any database calls but merely forwards the call to DataLayer messagesForHistoryIDs:), then change every call to that DataLayer method to use the new MLMessage method instead.
Note: caching should be still moved to MLMessage messageFromDictionary:.
So to sum it up:
- Move the cache from to
MLMessage messageFromDictionary: - Replace the code of
MLMessage createMessageFromHistoryID:withDataLayer messageForHistoryID: - Remove
DataLayer messageForHistoryID:and change all calls to useMLMessage createMessageFromHistoryID:now
There was a problem hiding this comment.
Did you solve this one yet?
| [[DataLayer sharedInstance] retractMessageHistory:message.messageDBId]; | ||
| [message updateWithMessage:[[[DataLayer sharedInstance] messagesForHistoryIDs:@[message.messageDBId]] firstObject]]; | ||
|
|
||
| //update table entry |
There was a problem hiding this comment.
Shouldn't the deleted/retracted message still be removed from the messages list? Currently the message is still in there, but its text gets reset to an empty string.
There was a problem hiding this comment.
Retracted and moderated messages don't get deleted, but rather have their text changed to "This message got retracted".
See https://github.com/monal-im/Monal/blob/198778f9889469a/Monal/Classes/chatViewController.m#L2245
That string is not stored in the DB / MLMessage object, in order for it to be localizeable.
The notification name might be a bit confusing. kMonalDeletedMessageNotice is only related to message retraction and moderation, not local deletion.
There was a problem hiding this comment.
Ah okay, on Conversations the moderated/retracted messages are removed from the ui...maybe we should use the opportunity to change Monal's behavior to be the same. What do you think?
There was a problem hiding this comment.
Conversations doesn't support retraction yet.
I think the placeholder for retracted messages should remain. Because retraction is not limited by time, and your contact can retract a message that you already read. And later you can re-read the history and wonder if there a was a message there or you imagined it.
As for moderation, it usually is spam cleaning in public channels. Deletion is fine in this case, since the server removes the messages from the MAM archive anyway (on the other hand, no server currently removes retracted messages from the MAM archive)
Also, spam can come in very large number of messages, which can be annoying if we keep the "this message got retracted" placeholder.
I think Gajim and Cheogram compress all (subsequent?) moderated messages into one, containing the reason of the moderation
There was a problem hiding this comment.
Hmmm, yes, good point.
| "revision": "4b8714a7fb84d42393314ce897127b3939885ec3", | ||
| "version": "3.8.5" | ||
| "revision": "a9ed4b6f9bdedce7d77046f43adfb8ce1fd54114", | ||
| "version": "3.9.0" |
There was a problem hiding this comment.
Not a good idea without adapting to the new release, from the release notes:
DDFileLogger: File Locking is now optional
We need to explicitly turn locking on. We'd get corrupted rawlog files without locking. That was the reason why I added locking upstream. But since some folks complained over here, the locking was made optional.
Please implement the shouldLockLogFile: method in MLLogFileManager.m always returning YES, see https://github.com/CocoaLumberjack/CocoaLumberjack/pull/1425/files
| .padding() | ||
| .addLoadingOverlay(overlay) | ||
| .onReceive(NotificationCenter.default.publisher(for: NSNotification.Name("kXMPPError")).receive(on: RunLoop.main)) { notification in | ||
| .onReceive(NotificationCenter.default.publisher(for: NSNotification.Name(kXMPPError)).receive(on: RunLoop.main)) { notification in |
There was a problem hiding this comment.
Where are all these new constants defined in? I know they are #define constants in ObjC, but these unfortunately don't automatically leak to Swift afaik, see the top of SwiftHelpers.swift and HelperTools getObjcDefinedValue:.
There was a problem hiding this comment.
The constants are defined in MLConstants.h, which is part of monalxmpp.
monalxmpp is globally imported in our Swift source files.
And since commit 12c4d23ed52b93, MLConstants.h has public visibilty.
Thus we don't need to manually import constants from objective-C to Swift anymore.
There was a problem hiding this comment.
Wow, that's great! :)
You can remove all these manual imports and the HelperTools method used for this, then :)
lissine0
force-pushed
the
message-updates
branch
from
b19756f to
fa3cf13
Compare
MLConstants.h is part of monalxmpp, which is globally imported in our Swift source files. And as of commit 12c4d23, MLConstants.h has public visibility. Thus we don't need to manually import Objc constants anymore.
Using constants allows the compiler to catch potential typos.
Use kMonalUpdatedMessageNotice for message corrections and MUC message reflections, instead of using kMonalNewMessageNotice.
Also, delete MLMessage:updateWithMessage: as it's no longer used
Thanks to ObservableKVOWrapper, and to MLMessage being a model class, this gives us automatic view updates when an MLMessage object changes, while still using the default MessageView.
lissine0
force-pushed
the
message-updates
branch
from
fa3cf13 to
f1bb841
Compare
tmolitor-stud-tu
left a comment
tmolitor-stud-tu
left a comment
There was a problem hiding this comment.
Do you want to do the ExyteChat Protocol change in a new PR or in this one?
| let dbMessages = DataLayer.sharedInstance().messages(forContact:contact.contactJid, forAccount:contact.accountID) as! [MLMessage] | ||
| for msg in dbMessages { | ||
| messages.append(ChatViewMessage(msg)) | ||
| messages.append(ChatViewMessage(ObservableKVOWrapper(msg))) |
There was a problem hiding this comment.
No, you should change the code to "bubble that change up", aka: only wrap the MLMessage object into the kvo observer once and pass around the kvo observer or some object containing it rather then recreating things over and over again on every render.
| @ObservedObject var viewModel: ExyteChat.ChatViewModel | ||
| let positionInUserGroup: PositionInUserGroup | ||
| let positionInMessagesSection: PositionInMessagesSection | ||
| init(message: ObservableKVOWrapper<MLMessage>, viewModel: ChatViewModel, positionInUserGroup: PositionInUserGroup, positionInMessagesSection: PositionInMessagesSection) { |
There was a problem hiding this comment.
Yes, I alreay said in some review that you should overwrite the properties of Message in ChatViewMessage using computed properties returning the values of MLMessage and use the MLMessage as @StateObject. That should work.
See #1426 (comment)
That means something like this (maybe you could try a proof of concept first, before updating this PR to save some work in case it doesn't work like depicted below):
- A view using an
@StateObjectto a class namedChatViewMessageand using itstextproperty in the view body - The
ChatViewMessageclass being a child of the ExyteChatopen class Message: ObservableObject, Identifiable - The
ChatViewMessageclass having a propertyinnerMessageof typeObservableKVOWrapper<MLMessage> - The
ChatViewMessageclass having an@Publishedcomputed property of typeStringnamedtext, that always returns theinnerMessage.messageTextvalue. This property overwrites the@Published public var text: Stringproperty of the base class
Okay, step 4 is a problem here, apparently you cannot overwrite a stored property with a computed one :(
I hate Swift, in ObjC that could easily be done :(
Solution:
- Check if the proof of concept above works while not deriving the
ChatViewMessageclass from anything. (Only check if the computed property properly forwards the change event of theObservableKVOWrapperwrappedMLMessageobject (use some timer to periodically change the text). - If that works, then either:
- Change the stored properties in the base class to be computed ones (computed ones can be overwritten).
- Update the ExyteChat to define a
protocolnamedMessageProtocolthat defines all the properties of the ExyteChatMessageclass/struct; let the ExyteChatMessagederive from that protocol and change all mentions ofMessagein the ExyteChat codebase to beMessageProtocolones; then derive our implementation from that protocol, too, instead of deriving it from theMessageclass.
I think introducing a new protocol is the way to go here, because it makes the original ExyteChat stuff more extensible but doesn't change any behavior. I think we can even upstream that change (together with making Message a class rather than a struct).
btw: the public enum Status doesn't contain a state for received, too, so we'll have to update the base class anyways.
| [[DataLayer sharedInstance] retractMessageHistory:message.messageDBId]; | ||
| [message updateWithMessage:[[[DataLayer sharedInstance] messagesForHistoryIDs:@[message.messageDBId]] firstObject]]; | ||
|
|
||
| //update table entry |
There was a problem hiding this comment.
Hmmm, yes, good point.
Let's keep that in a separate PR |
Okay, I created an issue for it over here: #1462 |
3 participants
Use
kMonalUpdatedMessageNoticefor message corrections and MUC message reflections, instead of usingkMonalNewMessageNotice.I added observing of this new notification in all the places that are observing
kMonalNewMessageNotice, except forMonalAppDelegate.I think that's not needed because message updates don't cause the unread counter to be changed.
Also, I'm not sure observing the new notification in
MLContactis strictly necessary, but it's not harmful.On a semi-related note, you once said that when posting notifications,
MLNotificationQueueshould always be used (as opposed toNSNotificationCenter)It turns out there are some places in the code base that use
NSNotificationCenterfor posting notifications.See them with
grep --color=auto -in 'NotificationCenter.*post' Monal/Classes/*.{m,swift}Should I use
MLNotificationQueuein these places?(The
CFNotificationCenterones should remain)