From ef97a9aaa9d67509678fbd05e66964fc38371d78 Mon Sep 17 00:00:00 2001 From: manuroe Date: Wed, 14 Apr 2021 09:25:35 +0200 Subject: [PATCH] MXRoomMembers: Fix wrong view of room members when paginating vector-im/element-ios/issues/4204 Fix a regression introduced by https://github.com/matrix-org/matrix-ios-sdk/pull/1040. While paginating, the kept MXRoomState instance could be released, creating a bad view of room members. Remove the bad dependency to MXRoomState to avoid any retain cycle. --- CHANGES.rst | 1 + MatrixSDK/Data/MXRoomMembers.h | 18 --------- MatrixSDK/Data/MXRoomMembers.m | 65 ++++++++++++++++++++----------- MatrixSDK/Data/MXRoomState.m | 2 +- MatrixSDKTests/MXRoomStateTests.m | 2 +- 5 files changed, 46 insertions(+), 42 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 190524de91..5855b0ae39 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -9,6 +9,7 @@ Changes to be released in next version 🐛 Bugfix * MXSession: Fix deadlock regression in resume() (vector-im/element-ios/issues/4202). + * MXRoomMembers: Fix wrong view of room members when paginating (vector-im/element-ios/issues/4204). ⚠️ API Changes * diff --git a/MatrixSDK/Data/MXRoomMembers.h b/MatrixSDK/Data/MXRoomMembers.h index 6c3fdf7270..fda572b0f8 100644 --- a/MatrixSDK/Data/MXRoomMembers.h +++ b/MatrixSDK/Data/MXRoomMembers.h @@ -38,12 +38,6 @@ @return The newly-initialized MXRoomMembers. */ - (instancetype)initWithRoomState:(MXRoomState*)state andMatrixSession:(MXSession*)matrixSession; - -/** - The room state belonging to this object. - */ -@property (nonatomic, readonly) MXRoomState *roomState; - /** A copy of the list of room members. */ @@ -115,16 +109,4 @@ */ - (BOOL)handleStateEvents:(NSArray *)stateEvents; -#pragma mark - NSCopying - -/** - Copy the receiver and update the room state reference. This is useful when the copying happens - as part of copying an MXRoomState object. - - @param zone a memory zone. - @param state the new/copied room state. - @return a copy of the receiver. - */ -- (id)copyWithZone:(NSZone *)zone andState:(MXRoomState *)state; - @end diff --git a/MatrixSDK/Data/MXRoomMembers.m b/MatrixSDK/Data/MXRoomMembers.m index 09bfebdc52..3635acfbcd 100644 --- a/MatrixSDK/Data/MXRoomMembers.m +++ b/MatrixSDK/Data/MXRoomMembers.m @@ -23,7 +23,11 @@ @interface MXRoomMembers () { MXSession *mxSession; - __weak MXRoomState *state; + + // Context for this MXRoomMembers instance + BOOL isConferenceUserRoom; + NSString *conferenceUserId; + BOOL isLive; /** Members ordered by userId. @@ -47,7 +51,10 @@ - (instancetype)initWithRoomState:(MXRoomState *)roomState andMatrixSession:(MXS if (self) { mxSession = matrixSession; - state = roomState; + + isConferenceUserRoom = roomState.isConferenceUserRoom; + conferenceUserId = roomState.conferenceUserId; + isLive = roomState.isLive; members = [NSMutableDictionary dictionary]; membersNamesInUse = [NSMutableDictionary dictionary]; @@ -55,11 +62,6 @@ - (instancetype)initWithRoomState:(MXRoomState *)roomState andMatrixSession:(MXS return self; } -- (MXRoomState *)roomState -{ - return state; -} - - (NSArray *)members { return [members allValues]; @@ -158,12 +160,12 @@ - (NSString*)memberSortedName:(NSString*)userId { NSArray *membersWithoutConferenceUser; - if (state.isConferenceUserRoom) + if (isConferenceUserRoom) { // Show everyone in a 1:1 room with a conference user membersWithoutConferenceUser = self.members; } - else if (![self memberWithUserId:state.conferenceUserId]) + else if (![self memberWithUserId:conferenceUserId]) { // There is no conference user. No need to filter membersWithoutConferenceUser = self.members; @@ -172,7 +174,7 @@ - (NSString*)memberSortedName:(NSString*)userId { // Filter the conference user from the list NSMutableDictionary *membersWithoutConferenceUserDict = [NSMutableDictionary dictionaryWithDictionary:members]; - [membersWithoutConferenceUserDict removeObjectForKey:state.conferenceUserId]; + [membersWithoutConferenceUserDict removeObjectForKey:conferenceUserId]; membersWithoutConferenceUser = membersWithoutConferenceUserDict.allValues; } @@ -183,14 +185,14 @@ - (NSString*)memberSortedName:(NSString*)userId { NSArray *membersWithMembership; - if (includeConferenceUser || state.isConferenceUserRoom) + if (includeConferenceUser || isConferenceUserRoom) { // Show everyone in a 1:1 room with a conference user membersWithMembership = [self membersWithMembership:theMembership]; } else { - MXRoomMember *conferenceUserMember = [self memberWithUserId:state.conferenceUserId]; + MXRoomMember *conferenceUserMember = [self memberWithUserId:conferenceUserId]; if (!conferenceUserMember || conferenceUserMember.membership != theMembership) { // The conference user is not in list of members with the passed membership @@ -207,7 +209,7 @@ - (NSString*)memberSortedName:(NSString*)userId } } - [membersWithMembershipDict removeObjectForKey:state.conferenceUserId]; + [membersWithMembershipDict removeObjectForKey:conferenceUserId]; membersWithMembership = membersWithMembershipDict.allValues; } } @@ -255,7 +257,7 @@ - (BOOL)handleStateEvents:(NSArray *)stateEvents; } } - MXRoomMember *roomMember = [[MXRoomMember alloc] initWithMXEvent:event andEventContent:[state contentOfEvent:event]]; + MXRoomMember *roomMember = [[MXRoomMember alloc] initWithMXEvent:event andEventContent:[self contentOfEvent:event]]; if (roomMember) { /// Update membersNamesInUse @@ -292,7 +294,7 @@ - (BOOL)handleStateEvents:(NSArray *)stateEvents; // Special handling for presence: update MXUser data in case of membership event. // CAUTION: ignore here redacted state event, the redaction concerns only the context of the event room. - if (state.isLive && !event.isRedactedEvent && roomMember.membership == MXMembershipJoin) + if (isLive && !event.isRedactedEvent && roomMember.membership == MXMembershipJoin) { MXUser *user = [mxSession getOrCreateUser:event.sender]; [user updateWithRoomMemberEvent:event roomMember:roomMember inMatrixSession:mxSession]; @@ -314,18 +316,14 @@ - (BOOL)handleStateEvents:(NSArray *)stateEvents; #pragma mark - NSCopying -- (id)copyWithZone:(NSZone *)zone andState:(MXRoomState *)state { - MXRoomMembers *copy = [self copyWithZone:zone]; - copy->state = state; - return copy; -} - - (id)copyWithZone:(NSZone *)zone { MXRoomMembers *membersCopy = [[MXRoomMembers allocWithZone:zone] init]; membersCopy->mxSession = mxSession; - membersCopy->state = state; + membersCopy->isConferenceUserRoom = isConferenceUserRoom; + membersCopy->conferenceUserId = conferenceUserId; + membersCopy->isLive = isLive; // MXRoomMember objects in members are immutable. A new instance of it is created each time // the sdk receives room member event, even if it is an update of an existing member like a @@ -336,4 +334,27 @@ - (id)copyWithZone:(NSZone *)zone return membersCopy; } + + +#pragma mark - Private methods + +// According to the direction of the instance, we are interested either by +// the content of the event or its prev_content +- (NSDictionary *)contentOfEvent:(MXEvent*)event +{ + NSDictionary *content; + if (event) + { + if (isLive) + { + content = event.content; + } + else + { + content = event.prevContent; + } + } + return content; +} + @end diff --git a/MatrixSDK/Data/MXRoomState.m b/MatrixSDK/Data/MXRoomState.m index e30ec82d04..7843bcfa35 100644 --- a/MatrixSDK/Data/MXRoomState.m +++ b/MatrixSDK/Data/MXRoomState.m @@ -607,7 +607,7 @@ - (id)copyWithZone:(NSZone *)zone stateCopy->stateEvents[key] = [[NSMutableArray allocWithZone:zone] initWithArray:stateEvents[key]]; } - stateCopy->_members = [_members copyWithZone:zone andState:stateCopy]; + stateCopy->_members = [_members copyWithZone:zone]; stateCopy->_membersCount = [_membersCount copyWithZone:zone]; diff --git a/MatrixSDKTests/MXRoomStateTests.m b/MatrixSDKTests/MXRoomStateTests.m index 444e5091db..c1319c31c9 100644 --- a/MatrixSDKTests/MXRoomStateTests.m +++ b/MatrixSDKTests/MXRoomStateTests.m @@ -1269,7 +1269,7 @@ - (void)testCopying mxSession = mxSession2; [room state:^(MXRoomState *roomState) { MXRoomState *roomStateCopy = [roomState copy]; - XCTAssertEqual(roomStateCopy.members.roomState, roomStateCopy); + XCTAssertEqual(roomStateCopy.members.members.count, roomState.members.members.count); [expectation fulfill]; }]; }];