-
Notifications
You must be signed in to change notification settings - Fork 11
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
Split VC isCollapsed fix (issue 54) #56
Conversation
47b0c24
to
6c967cc
Compare
@enhorn can you take a look again? I'm very happy to not use the new name |
@mansbernhardt thought you might have some input on this PR? |
I like the idea of the enum instead of an optional boolean. Opens for clearer documentation as well. |
} | ||
|
||
/// Returns a signal that will signal when collapsing or expanding. Current value can be nil the collapsed state cannot be determined reliably yet. | ||
public var collapsedState: ReadSignal<Bool?> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the Bool?
as it states nicely what is going on (true, false or unknown), hence I doubt an enum will make things clearer (perhaps even the opposite). However, we could consider just naming this isCollapsed
(as it is not taken and it is not that uncommon that we name read or readwrite signals without the signal postfix, especially when we don't have a name collision). If however you like to stay with the state variant, I would prefer isCollapsedState
instead.
public var isCollapsedSignal: ReadSignal<Bool> { | ||
return isCollapsedProperty.readOnly().map { $0 ?? true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a readonly as map will make it readable anyway.
/// Note that its value can be unreliable until it is rendered on the screen. | ||
public init(collapsedState: Future<Bool>) { | ||
super.init() | ||
initialCollapsedStateDisposable += collapsedState.onValue { [weak self] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is initialCollapsedStateDisposable
necessary, as we hold self weakly anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the bag only to remove the subscription earlier if one of the other 2 delegate methods setting the state get triggered before the future completes. However that might not be very realistic case, what do you think?
/// Creates a delegate that will manage a split view controller with the specified initial collapsed state | ||
/// - Parameter collapsedState: Use to report the initial `isCollapsed` state of the split view controller. | ||
/// Note that its value can be unreliable until it is rendered on the screen. | ||
public init(collapsedState: Future<Bool>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we pass this as a future instead of as a signal? Sound more like something that might change over time than something that takes time to complete? Rename to isCollapsed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source of truth for the collapsed state is the split view controller. I think its isCollapsed
property is not kvo compliant. It seems like once it's on screen the delegate methods track properly the collapsing/expanding so I modelled it as a future to make it clear it's only the first time it gets added to a window that we don't know what's the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree for the renaming.
|
||
/// Creates a new instance using changes in `elements` and `isCollapsed` to maintain the selected detail index (provided signal). | ||
/// - Parameters: | ||
/// - isSame: Is it the same row (same identity) | ||
/// - needsUpdate: For the same row, does the row have updates that requires presenting new details (refresh details) | ||
/// - isCollapsed: Whether or not details are displayed. | ||
public init(elements: ReadSignal<Elements>, isSame: @escaping (Element, Element) -> Bool, needsUpdate: @escaping (Element, Element) -> Bool = { _, _ in false }, isCollapsed: ReadSignal<Bool>) { | ||
public init(elements: ReadSignal<Elements>, isSame: @escaping (Element, Element) -> Bool, needsUpdate: @escaping (Element, Element) -> Bool = { _, _ in false }, collapsedState: ReadSignal<Bool?>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can keep the same parameter name isCollapsed
, but just change the type and let both the deprecated and the new init live side by side only differ by the signal name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's working 👍
|
||
bag += isCollapsed.atOnce().onValueDisposePrevious { [weak self] isCollapsed in | ||
guard let `self` = self else { return NilDisposer() } | ||
bag += collapsedState.atOnce().onValueDisposePrevious { [weak self] isCollapsed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use with(weak: self)
here?
bag += isCollapsed.atOnce().onValueDisposePrevious { [weak self] isCollapsed in | ||
guard let `self` = self else { return NilDisposer() } | ||
bag += collapsedState.atOnce().onValueDisposePrevious { [weak self] isCollapsed in | ||
guard let `self` = self, let isCollapsed = isCollapsed else { return NilDisposer() } | ||
return self.keepSelection.atOnce().enumerate().onValue { [weak self] (eventCount, indexAndElement) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use with(weak: self)
here?
@@ -116,7 +121,7 @@ public final class MasterDetailSelection<Elements: BidirectionalCollection>: Sig | |||
guard let `self` = self else { | |||
return NilDisposer() | |||
} | |||
onSet(self.isCollapsed.value ? nil : self.current) | |||
onSet((self.isCollapsed.value == nil || self.isCollapsed.value == true) ? nil: self.current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onSet((self.isCollapsed.value ?? true) ? nil: self.current)
@@ -156,7 +162,7 @@ public extension MasterDetailSelection { | |||
|
|||
immediate = true | |||
let presentDisposable = vc.present(presentation.onDismiss { | |||
if self.isCollapsed.value && !immediate { | |||
if isCollapsed && !immediate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this changing the behavior as you using an old value in an async op?
If not perhaps isCollapsed
should be name wasCollapsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch, I missed the fact that this block of code is async. I'm struggling to decide what to do in the nil case here because I think that's not gonna happen (see a bit above we have an early return when isCollapsed is nil and it will never change from true/false to nil). I'm just gonna return early in that case. Btw this is the behaviour that this code influences and it was broken with the previous commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assertion failure since that will notify us if this behaviour is not as we expect it in debug builds.
@niil-ohlin I'm getting some strange swiftlint failure on the CI for unused |
Use static swiftlint version
I'm sorry!
I think Swiftlint should be happy now. Can you re-review? |
Pressed merge instead of squash.. oh well 😔 |
1.9.0
Tested with the Messages app on iOS 11 and 13. Tested in the main iZettle project too.