Skip to content
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

Fix an occurence of null displayname issue #605

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

hberenger
Copy link

@hberenger hberenger commented Nov 29, 2018

Hi there,

After having upgraded our Matrix client app to use matrix-ios-sdk v0.11.5, we noticed that some rooms happen to lose their display name and avatar on a seemingly random basis.
I can see from PR #599 that you are also facing similar issues.

From user feedbacks, it appears that Direct chats are the most affected.

I could reproduce this problem with the latest Riot iOS client, so I guess v0.11.6 does not fix that.

Steps to reproduce :

  • with Riot iOS client, create a Direct Chat with some user Foo
  • kill Riot iOS app
  • from another client (e.g the webclient), go to the Foo chat, and send ~25 messages (enough to trigger a limited sync)
  • now open Riot iOS : "sometimes" (about half the time from my tests) you'll find that the Foo chat has been renamed Empty room, and has lost its avatar (used to be Foo avatar, now it's the first letter of the roomId)

What happens is that :

  1. due to the large number of messages sent, Riot receives a limited sync of the chat.
  2. In MXEventTimeline.handleJoinedRoomSync, this limited sync triggers a kMXRoomDidFlushDataNotification notification.
  3. This notification is handled by MXRoomSummary.resetRoomStateData, which resets the chat avatar, displayname, topic and aliases, and tries to recompute them from state events.
  4. Unfortunately, the state event list of the direct chat does not contain enough information to recompute a displayname or an avatar.
  5. If the limited sync data also contains a server room summary, then the displayname and avatar are restored soon after, in MXRoomSummary.handleJoinedRoomSync
  6. Yet if the sync data does not contain the server room summary, the displayname and avatar are now reset for good (the null value is persisted), until the user either clears the cache or logs out & in.

I came up with the following workaround, which I submit to your approval : since the summary can't be recomputed from the state events, we can still reconstruct it by calling the standard Matrix name/avatar computation algorithm, via the update delegate's session:updateRoomSummary:withServerRoomSummary:roomState: method, passing it a nil server room summary.
(A similar trick is already used to compute the room summary of invited rooms in session:updateRoomSummary:withStateEvents:roomState:)

Not sure this fix is the most relevant (maybe this should be fixed on the server side after all, by sending the server room summary more often ?), but at least it can start a discussion ^^

Thank you !

Signed-off-by: Hervé Bérenger [email protected]

Copy link
Contributor

@manuroe manuroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for having chased and fixed this case.

I have written an integration test (called testRoomSummaryDisplayNameInDirectChat) that shows your changes do fix the issue.

if ([self.mxSession.roomSummaryUpdateDelegate session:self.mxSession updateRoomSummary:self withStateEvents:roomState.stateEvents roomState:roomState])
BOOL updated = [self.mxSession.roomSummaryUpdateDelegate session:self.mxSession updateRoomSummary:self withStateEvents:roomState.stateEvents roomState:roomState];

if (room.isDirect && (_displayname == nil || _avatar == nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (room.isDirect && (_displayname == nil || _avatar == nil))
if (self.displayname == nil || self.avatar == nil))

We could make it more general and not only for direct rooms.

Prefer to use self.displayname to avoid implicit retains. The SDK project fails to build on such error now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

If a direct chat receives a "limited" sync, a DidFlushData notification
is triggered. This notification resets the room summary, which in some
cases can't be recomputed from state events. If this happens, we apply
the Matrix algorithm to recompute the direct chat name and avatar.

Signed-off-by: Hervé Bérenger <[email protected]>
@hberenger
Copy link
Author

Thank you @manuroe for your prompt answer.
I've just amended my commit to take your suggestions into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants