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.

Sign up for GitHub

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

Jump to bottom

[Android] Extracted native Android code #4

Merged
merged 12 commits into from
Sep 5, 2019
Merged

[Android] Extracted native Android code #4

merged 12 commits into from
Sep 5, 2019

Conversation

mikolak
Copy link
Collaborator

@mikolak mikolak commented Sep 2, 2019

No description provided.

mikolak
mikolak commented Sep 3, 2019
View reviewed changes
Copy link
Collaborator Author

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

Left some remarks

android/library/src/main/java/com/polidea/multiplatformbleadapter/BleAdapter.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleAdapter.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/Event.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/RefreshGattMoment.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/RefreshGattMoment.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
Cierpliwy
Cierpliwy suggested changes Sep 3, 2019
View reviewed changes
Copy link

@Cierpliwy Cierpliwy left a comment

Choose a reason for hiding this comment

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

Additionaly, in original implementation I used SafePromise class to be sure that we may emit only one value or one error in Promise based APIs. Currently this assumption doesn't hold with new, two separate callbacks.

I also think that we should have different API for stream based operations like scanning, monitoring etc. This is something which can be discussed.

android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/Event.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/RefreshGattMoment.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/errors/ErrorConverter.java Show resolved Hide resolved
pawelByszewski
pawelByszewski requested changes Sep 3, 2019
View reviewed changes
android/library/build.gradle Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleAdapter.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleAdapter.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Outdated Show resolved Hide resolved
@bartoszwilk
Copy link
Contributor

Additionaly, in original implementation I used SafePromise class to be sure that we may emit only one value or one error in Promise based APIs. Currently this assumption doesn't hold with new, two separate callbacks.

I also think that we should have different API for stream based operations like scanning, monitoring etc. This is something which can be discussed.

Ad. 1 Fixed. I added OneTimeActionExecutor for this case
Ad. 2 We discussed about it and we leave it as it is

mikolak
mikolak commented Sep 4, 2019
View reviewed changes
Copy link
Collaborator Author

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

I have some more remarks, but just read them, create issues for the marked places and merge it if possible. Try not to add more code to this PR.

android/library/src/main/java/com/polidea/multiplatformbleadapter/ConnectionOptions.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleAdapter.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/Characteristic.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/RefreshGattMoment.java Show resolved Hide resolved
...in/java/com/polidea/multiplatformbleadapter/utils/mapper/RxScanResultToScanResultMapper.java Outdated Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
android/library/src/main/java/com/polidea/multiplatformbleadapter/BleModule.java Show resolved Hide resolved
This was referenced Sep 5, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@bartoszwilk bartoszwilk merged commit b7de404 into master Sep 5, 2019
@bartoszwilk bartoszwilk mentioned this pull request Sep 5, 2019
Closed
@mikolak mikolak deleted the android/react-native-codebase-import branch June 10, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cierpliwy Cierpliwy Cierpliwy requested changes

@dariuszseweryn dariuszseweryn dariuszseweryn left review comments

@bartoszwilk bartoszwilk bartoszwilk left review comments

@pawelByszewski pawelByszewski pawelByszewski approved these changes

Assignees

@bartoszwilk bartoszwilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mikolak @bartoszwilk @Cierpliwy @dariuszseweryn @pawelByszewski