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

Group details: Add Rooms page #1717

Merged
merged 1 commit into from
Jan 2, 2018
Merged

Group details: Add Rooms page #1717

merged 1 commit into from
Jan 2, 2018

Conversation

giomfo
Copy link
Member

@giomfo giomfo commented Dec 31, 2017

@giomfo giomfo requested a review from manuroe December 31, 2017 11:53
- (void)cancelRegistrationOnGroupChangeNotifications
{
// Remove any pending observers
[[NSNotificationCenter defaultCenter] removeObserver:self];
Copy link
Member

Choose a reason for hiding this comment

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

That kills the listener on kRiotDesignValuesDidChangeThemeNotification too.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, I will fix

Copy link
Member Author

Choose a reason for hiding this comment

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

@manuroe after checking it. I disagree, the kRiotDesignValuesDidChangeThemeNotification observer is not remove here. But I will update the code to remove only the listeners on group update

Copy link
Member

Choose a reason for hiding this comment

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

ok. I am going to merge the PR as is. Note that there is same behavior in GroupParticipantsViewController and GroupHomeViewController

// Update here the displayed group instance with the one stored in the session (if any).
MXGroup *group = [self.mxSession groupWithGroupId:_group.groupId];

[self refreshDisplayWithGroup:(group ? group : _group)];
Copy link
Member

Choose a reason for hiding this comment

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

Does it worth to update if the updated group is not the current one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided here to keep up to date the displayed information of the group even if the update has been observed/triggered on another instance of the group

Copy link
Member

Choose a reason for hiding this comment

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

ack (this comment is in fact an hidden test for push notifications testing)

}
}

- (void)startActivityIndicator
Copy link
Member

Choose a reason for hiding this comment

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

The activity indicator is not used for the moment (this is just a notice)

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, I kept it because it will be used when we will enable the admin to remove rooms

@manuroe manuroe merged commit 4ad8eec into develop Jan 2, 2018
@giomfo giomfo deleted the group_details_rooms branch January 2, 2018 15:39
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