search

Rightpoint / RZBluetooth

Core Bluetooth helper library
Other
136 stars 47 forks source link

Added user cancelled state #72

Closed tahirmt closed 5 years ago

tahirmt commented 7 years ago

Added a new connection event state user cancelled used when user explicitly calls cancelPeripheralConnection

KingOfBrian commented 7 years ago

Hey @matsoft90 -- thanks for the PR! The change looks good, but it looks like it caused a failure in one of the unit tests around maintaining the connection. Can you look into fixing the test? Also, it would be great if you could add a specific unit test for the user cancelled event type.

tahirmt commented 7 years ago

@KingOfBrian I can look into it... however I don't have a lot of knowledge of unit testing so it could take me some time to fix it.

KingOfBrian commented 7 years ago

Nice @matsoft90! -- Can I suggest a small improvement to the test? If you model cancelCount and disconnectCount we can verify the new enumeration a bit better. I believe the only other change will be changing XCTAssert(self.disconnectCount == i + 1); to XCTAssert((self.disconnectCount + self.cancelCount) == i + 1);

I can accept the PR as is, but every additional bit of testing helps.

cpatterson-lilly commented 7 years ago

It just occurred to me -- with this change, everywhere in my code that I am handling a RZBPeripheralStateEventDisconnected event, I will now have to check for either RZBPeripheralStateEventDisconnected or RZBPeripheralStateEventUserCancelled, even if I don't care why the disconnect occurred. Correct?

This will likely break existing code that relies on receiving the disconnect event. Would it make sense to issue both events in the case where the user cancels the connection?

KingOfBrian commented 7 years ago

Yea, this will be a breaking change and will need a major version bump. I don't see this causing too much code change though. It will need migration notes, and I'm also noticing that the comment block for the enum may be a bit dated. I'm probably going to have to leave this as a branch for a few weeks until I can get some time to define that process - this month is a bit crazy.

Duplicating the event will obscure the value of the case a bit so don't want to do that.

tahirmt commented 7 years ago

@KingOfBrian You are absolutely correct. I'll make the change to the test as it can help with future tests as well. @cpatterson-lilly I agree with @KingOfBrian that generating duplicate events would effectively remove the user cancelled case. As with all enums you have to handle all cases so it will become a breaking change for large projects.

tahirmt commented 7 years ago

@KingOfBrian Is there any eta on when you want to release this?? I want to add RZBluetooth using pods instead of copying the project files to my project. But I need the change in this plus the fix in the setNotify:NO reconnect issue in the repo to do that. Or if you can suggest how I can do it before you making a new release, I would appreciate it.

KingOfBrian commented 5 years ago

Sorry for the delay here, I'm going to push a new release out this week!