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.

Sign up for GitHub

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

Jump to bottom

Do not show user indicators when the view controller is not visible #5804

Merged
merged 3 commits into from
Mar 11, 2022
Merged

Do not show user indicators when the view controller is not visible #5804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18e7886
Do not show user indicators when the view controller is not visible
Anderas Mar 10, 2022
9b72e91
Name changes
Anderas Mar 10, 2022
64652b4
Add more comments
Anderas Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Riot/Modules/Common/ActivityIndicator/UserIndicatorPresenterWrapper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ import CommonKit
return
}

MXLog.debug("[UserIndicatorPresenterWrapper] Present loading indicator")
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
MXLog.debug("[UserIndicatorPresenterWrapper] Present loading indicator")
MXLog.debug("[UserIndicatorPresenterWrapper] Present activity indicator")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think activity does not belong here, because that name is currently only associated with UIActivityIndicator, which this system is not using. If anything I can probably rename the presentActivityIndicator methods.

loadingIndicator = presenter.present(.loading(label: label, isInteractionBlocking: false))
}

@objc func dismissActivityIndicator() {
MXLog.debug("[UserIndicatorPresenterWrapper] Dismiss loading indicator")
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
MXLog.debug("[UserIndicatorPresenterWrapper] Dismiss loading indicator")
MXLog.debug("[UserIndicatorPresenterWrapper] Dismiss activity indicator")

loadingIndicator = nil
}

Expand Down
11 changes: 9 additions & 2 deletions Riot/Modules/Common/Recents/RecentsViewController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ @interface RecentsViewController () <CreateRoomCoordinatorBridgePresenterDelegat
// when the user selects it.
UISearchBar *tableSearchBar;

// Flag determining whether the view controller is ready to use (potentially shared) user indicators
// depending on whether the controller is visible or not
BOOL isUserIndicatorEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing mandatory. This name confused me a bit at first because I believed that you wanted to add the capability to enable / disable the activity indicator from other instance or subclasses. Why not not calling this property isViewVisible as it is exactly what you aim to check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I guess that is more typical for cases like this and will be clearer.


// Observe kThemeServiceDidChangeThemeNotification to handle user interface theme change.
__weak id kThemeServiceDidChangeThemeNotificationObserver;
}
Expand Down Expand Up @@ -266,6 +270,8 @@ - (void)viewWillAppear:(BOOL)animated
[super viewWillAppear:animated];

[self.screenTracker trackScreen];

isUserIndicatorEnabled = YES;

// Reset back user interactions
self.userInteractionEnabled = YES;
Expand Down Expand Up @@ -317,6 +323,7 @@ - (void)viewWillDisappear:(BOOL)animated
kMXNotificationCenterDidUpdateRulesObserver = nil;
}

isUserIndicatorEnabled = NO;
[self stopActivityIndicator];
}

Expand Down Expand Up @@ -2408,15 +2415,15 @@ - (BOOL)providesCustomActivityIndicator {
}

- (void)startActivityIndicatorWithLabel:(NSString *)label {
if (self.indicatorPresenter) {
if (self.indicatorPresenter && isUserIndicatorEnabled) {
[self.indicatorPresenter presentActivityIndicatorWithLabel:label];
} else {
[super startActivityIndicator];
}
}

- (void)startActivityIndicator {
if (self.indicatorPresenter) {
if (self.indicatorPresenter && isUserIndicatorEnabled) {
[self.indicatorPresenter presentActivityIndicator];
} else {
[super startActivityIndicator];
Expand Down
14 changes: 12 additions & 2 deletions Riot/Modules/Room/RoomViewController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,9 @@ - (void)viewWillDisappear:(BOOL)animated
isAppeared = NO;

[VoiceMessageMediaServiceProvider.sharedProvider pauseAllServices];
[self stopActivityIndicator];

// Stop the loading indicator even if the session is still in progress
[self stopLoadingIndicator];
}

- (void)viewDidAppear:(BOOL)animated
Expand Down Expand Up @@ -956,14 +958,22 @@ - (void)stopActivityIndicator
notificationTaskProfile = nil;
}
if ([self providesCustomActivityIndicator]) {
// The legacy super implementation of `stopActivityIndicator` contains a number of checks grouped under `canStopActivityIndicator`
// to determine whether the indicator can be stopped or not (and the method should thus rather be called `stopActivityIndicatorIfPossible`).
// Since the newer indicators are not calling super implementation, the check for `canStopActivityIndicator` has to be performed manually.
if ([self canStopActivityIndicator]) {
[self.delegate roomViewControllerDidStopLoading:self];
[self stopLoadingIndicator];
}
} else {
[super stopActivityIndicator];
}
}

- (void)stopLoadingIndicator
Copy link
Contributor

Choose a reason for hiding this comment

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

As a pending of presentActivityIndicator shouldn't we call this method stopActivityIndicator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There already is a stopActivityIndicator, which makes this all the more confusing, ie the legacy stopActivityIndicator will call the newer stopLoadingIndicator. The reason I extracted this out is so that we can call the latter directly from viewWillDissapear and avoid the canStop.... I think we need to move away from the legacy implementation ASAP, supporting both is turning this into a mess.

{
[self.delegate roomViewControllerDidStopLoading:self];
}

- (void)displayRoom:(MXKRoomDataSource *)dataSource
{
// Remove potential preview Data
Expand Down
1 change: 1 addition & 0 deletions changelog.d/5801.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Activity Indicators: Do not show user indicators when the view controller is not visible