Do not show user indicators when the view controller is not visible #5804
Conversation
Anderas
requested review from
a team and
gileluard
and removed request for
a team
| @@ -41,10 +41,12 @@ import CommonKit | |||
| return | |||
| } | |||
|
|
|||
| MXLog.debug("[UserIndicatorPresenterWrapper] Present loading indicator") | |||
There was a problem hiding this comment.
| MXLog.debug("[UserIndicatorPresenterWrapper] Present loading indicator") | |
| MXLog.debug("[UserIndicatorPresenterWrapper] Present activity indicator") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
| MXLog.debug("[UserIndicatorPresenterWrapper] Dismiss loading indicator") | |
| MXLog.debug("[UserIndicatorPresenterWrapper] Dismiss activity indicator") |
| } | ||
| } else { | ||
| [super stopActivityIndicator]; | ||
| } | ||
| } | ||
|
|
||
| - (void)stopLoadingIndicator |
There was a problem hiding this comment.
As a pending of presentActivityIndicator shouldn't we call this method stopActivityIndicator?
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yea I guess that is more typical for cases like this and will be clearer.
|
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/gyeEuK |
Not at all, thanks for your helpful feedback |
Fixes #5801
The architecture around user indicators is still not quite ideal, now that we have global user indicators it becomes problematic when view controllers attempt to display something whilst they are not actually visible. For example most view controllers subclass from
MXViewControllerwhich monitors theMXSessionat all times and starts / stops activity indicator accordingly.For now I am simply adding some flags to avoid presenting when the view controller is not visible, but this is not a robust enough solution and something else will need to be put in place. In an idea case the loading is more centralized and each room communicates its state changes via some delegate, but this obviously requires a bigger change.
Another angle to this is removing all the state change monitoring from each view controller when the view controller isn't actually visible. This should be done anyways but could have further consequences, so best done outside of a hotfix.