Update activity indicators on leaving room #5630
Conversation
Signed-off-by: Andy Uhnak <[email protected]>
|
📱 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/VaVQcr |
| func present() { | ||
| // Find the current top navigation controller | ||
| var vc: UIViewController? = viewController | ||
| while vc?.navigationController != nil { |
There was a problem hiding this comment.
SwiftLint warning here Identifier Name Violation: Variable name should be between 3 and 40 characters long: 'vc' (identifier_name)
There was a problem hiding this comment.
You probably need to update .swiftlint with the path to CommonKit. Do me a favor and check the other targets while you're at it.
There was a problem hiding this comment.
This is happening outside the CommonKit, so I must have some incomplete setup.
Riot/Modules/Common/ActivityIndicator/FullscreenActivityIndicatorPresenter.swift
Outdated
Show resolved
Hide resolved
Riot/Modules/Common/ActivityIndicator/FullscreenActivityIndicatorPresenter.swift
Outdated
Show resolved
Hide resolved
| private weak var viewController: UIViewController? | ||
| private var activity: Activity? | ||
|
|
||
| init(label: String, on viewController: UIViewController) { |
There was a problem hiding this comment.
This on parameter has the semantics of an action happening. We should keep it simple with init(title: String, viewController: UIViewController). Same applies to the FullscreenLoadingActivityPresenter`
There was a problem hiding this comment.
This is following the API requirements of ActivityIndicatorPresenterType, which has present(on view: UIView), but I can change at least the init.
| @@ -73,6 +76,10 @@ final class TabBarCoordinator: NSObject, TabBarCoordinatorType { | |||
| self.activityIndicatorPresenter = ActivityIndicatorPresenter() | |||
| } | |||
|
|
|||
| deinit { | |||
| activities.cancelAll() | |||
There was a problem hiding this comment.
What's the difference between cancel and complete in this context?
Also, I was under the impression this happens automatically on activity deinit?
There was a problem hiding this comment.
complete should not be public API, only cancel is. But this manual cancellation isn't necessary, it does indeed automatically deinit, so removing.
| switch type { | ||
| case let .loading(label): | ||
| let presenter = ToastActivityPresenter( | ||
| viewState: .init( |
There was a problem hiding this comment.
It's a bit weird that the presenter is advertising that it's using a RoundedToastView by requiring one of its viewStates. That's an implementation detail.
There was a problem hiding this comment.
Indeed, I extracted the view state out of the context of RoundedToastView so now it can be a shared type between the presenter and the view
Riot/Modules/Common/ActivityIndicator/FullscreenActivityIndicatorPresenter.swift
Show resolved
Hide resolved
| @@ -47,7 +48,9 @@ class ActivityIndicatorToastPresenter: ActivityPresentable { | |||
| ]) | |||
|
|
|||
| view.alpha = 0 | |||
| CATransaction.flush() | |||
There was a problem hiding this comment.
It was meant to solve the issue described in the previous comment, where calling dismiss right after present did not respect the current alpha, presumably because all the changes happen within one run loop, but testing it now, it does not actually work. Reverting for now and will find another solution in the next PR.
| imageView.translatesAutoresizingMaskIntoConstraints = false | ||
| NSLayoutConstraint.activate([ | ||
| imageView.widthAnchor.constraint(equalToConstant: Constants.imageViewSize), | ||
| imageView.heightAnchor.constraint(equalToConstant: Constants.imageViewSize), |
There was a problem hiding this comment.
Trailing Comma Violation: Collection literals should not have trailing commas. (trailing_comma)
There was a problem hiding this comment.
I have my eyes on automatic swift formatter as next improvement
|
|
||
| func present() { | ||
| // Find the current top navigation controller | ||
| var vc: UIViewController? = viewController |
There was a problem hiding this comment.
Might as well do it on the app's window.
There was a problem hiding this comment.
True, but then I have the problem of needing to inject the window (Or do the ugly UIApplication.shared.windows ... ). As I am going to work with these in the room indicators, I will revisit the approach.
| return indicator | ||
| }() | ||
|
|
||
| private lazy var imagView: UIImageView = { |
Resolves #5605
Note that this captures 2 most common out of 4 ways a user may leave the room. The other less common 2 ways are captured in #5608