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

Added initial state improvement & a minor fix for BluetoothState methods. #371

Merged
merged 3 commits into from
Aug 27, 2020
Merged

Added initial state improvement & a minor fix for BluetoothState methods. #371

merged 3 commits into from
Aug 27, 2020

Conversation

radumvlad
Copy link
Contributor

@radumvlad radumvlad commented May 6, 2020

This PR will add 1 fix and 1 improvement for BluetoothState methods:

  • report unknown if CBCentralManager doesn't have a state yet. This feels the right state to report in this case, since we can't say for sure that Bluetooth is unsupported.
  • added a new method for initial state on observeState(). This will also be emitted upon subscription, using deferred syntax. The main reason behind this is that perhaps all use-cases when observing the BluetoothState you will also want to check the initial state. All the examples in the code also use this and any app that wants to present BT state will probably also need an initial value. Moreover, the examples in the code currently use startWith, but they don't take into consideration when the subscription happened. If the subscription happens at a later time, that startWith might have a different value, and it becomes useless. In order to fix this, the user will always need to use the deferred syntax to be sure that he actually has the latest bluetooth state. This feels like an improvement that we can do for the users.

…we should report unknown if manager doesn't report anything yet, and not unsupported.
@CLAassistant
Copy link

CLAassistant commented May 6, 2020

CLA assistant check
All committers have signed the CLA.

@radumvlad
Copy link
Contributor Author

ping? :(

@dariuszseweryn
Copy link
Collaborator

Sorry for such a late reply. I am trying to find someone willing to look into this and opinionate.

As far as I remember the lack of initial state was picked because it is cleaner to add it to the beginning of the stream than to skip the first value (as it is an assumption about the internals of the observable) – merely not having this situation needed in the examples does not mean that noone has it and there was such a situation (I do not remember details now though).

I am not sure about .unknown vs .unsupported either — I need to look through the code and iOS implementation but I expect it was like this for a reason? Did you encountered a situation when .unsupported was returned when the capability was there?

@radumvlad
Copy link
Contributor Author

Hello @dariuszseweryn !
Regarding the .unknown vs .unsupported: Yes. It happened few times, on several different devices that had bluetooth and we didn't know exactly why or in what case. After we did this change, it stopped happening so we figured out this was the culprit.
Regarding the initial state, a pattern that already exists in this sense is Apple's existing implementation. Creating a CBCentralManager with delegate will also call the centralManagerDidUpdateState: method on the delegate informing the user of the initial state.
Also, I believe that although the implementation is a little bit simpler, the user might miss-use it and fall in a trap. A common behavior is to check whether the BT state is on/off and act upon that. That means using startWith, and, as I mentioned in my first post, if the users are not careful with it, it might be flaky. Even after acknowledging in a project that we need deferred syntax for that startWith, after a couple of months we accidentally introduced the same bug of forgetting to use it.

Copy link
Collaborator

@minixT minixT left a comment

Choose a reason for hiding this comment

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

Sorry that you had to wait for so long. Can you apply changes which I've suggested? Please, add also tests for new code.

@@ -92,11 +91,19 @@ public class CentralManager: ManagerType {
// MARK: State

public var state: BluetoothState {
return BluetoothState(rawValue: manager.state.rawValue) ?? .unsupported
return BluetoothState(rawValue: manager.state.rawValue) ?? .unknown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. Thanks for suggesting this changes.

Choose a reason for hiding this comment

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

Suggested change
return BluetoothState(rawValue: manager.state.rawValue) ?? .unknown
return BluetoothState(rawValue: manager.state.rawValue).unsafelyUnwrapped

}

public func observeState() -> Observable<BluetoothState> {
return self.delegateWrapper.didUpdateState.asObservable()
return Observable.deferred { [weak self] in
Copy link
Collaborator

@minixT minixT Aug 21, 2020

Choose a reason for hiding this comment

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

I'd rather add a new method which will return also current state than change current implementation. This will require less changes for users who already use our library. Can you create create new method for observing state and put there you code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :). Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping :D

@radumvlad radumvlad requested a review from minixT August 27, 2020 11:53
@minixT minixT merged commit 504eb79 into Polidea:master Aug 27, 2020
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.

5 participants