-
Notifications
You must be signed in to change notification settings - Fork 493
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
[Android] distinguish read and notification for future completion #1118
base: master
Are you sure you want to change the base?
[Android] distinguish read and notification for future completion #1118
Conversation
thanks for the update.
|
don't like this so much. ideally make it clear it is android only, or check for |
@@ -479,6 +486,7 @@ class BmCharacteristicData { | |||
required this.success, | |||
required this.errorCode, | |||
required this.errorString, | |||
this.receiveEvent = BmReceiveEventEnum.unknown, |
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.
lets make it required, for consistency
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.
Leaving it optional means that we need not make this a breaking change and would be better if possible.
We could default to BmReceiveEventEnum.unknown
or we could make the field nullable as done for pin
.
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.
breaking is fine tho. I still dont really understand why we wouldn't want to break.
now is the perfect time to break since we have no other users of this interface.
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.
@@ -468,6 +474,7 @@ class BmCharacteristicData { | |||
final bool success; | |||
final int errorCode; | |||
final String errorString; | |||
final BmReceiveEventEnum receiveEvent; |
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.
lets make this nullable
@@ -491,6 +499,7 @@ class BmCharacteristicData { | |||
success: json['success'] != 0, | |||
errorCode: json['error_code'], | |||
errorString: json['error_string'], | |||
receiveEvent: BmReceiveEventEnum.values[json['receive_event']], |
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.
lets make an explicit 'parse' helper function
unknown, // 0 | ||
read, // 1 | ||
notify, // 2 | ||
} |
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.
lets just have this:
enum BmReceiveType {
read, // 1
notify, // 2
}
and then make it nullable, and have a BmReceiveType? _parseReceiveTypeFromInteger
function
Hi Chip,
sorry I did not reply to your comments on #1080.
I have rebased it, rewritten it to use an enumeration and also made "receiveEvent" optional in the constructor of BmCharacteristicData, so that it does not break the Linux and web implementations. I think both of them support this distinction, so if you want me to, I can implement and test it.