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

fix: resolves #1103 - crash on disconnect #1111

Merged
merged 9 commits into from
Oct 16, 2023

Conversation

gmiszewski-intent
Copy link
Collaborator

@gmiszewski-intent gmiszewski-intent commented Oct 10, 2023

The issue origin starts in RxAndroidBle library - for now I only added global handling of undeliverable RxJava errors, with message in logcat.

@gmiszewski-intent gmiszewski-intent force-pushed the bugfix/handle-android-connecton-problems branch from d2cf645 to 95ae38b Compare October 10, 2023 09:53
@@ -36,22 +36,36 @@
import com.bleplx.converter.ServiceToJsObjectConverter;
import com.bleplx.utils.ReadableArrayConverter;
import com.bleplx.utils.SafePromise;
import com.polidea.rxandroidble2.internal.RxBleLog;
Copy link

@eliaslecomte eliaslecomte Oct 11, 2023

Choose a reason for hiding this comment

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

Should we really use an internal log method here?
The benefits are it follows log level settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are more places in library where it was used...
We should think about global refactor of this.

@@ -21,7 +21,7 @@ export function TestStateDisplay({ label, state, value }: TestStateDisplayProps)
<Container>
<Header>
<Label>{label}</Label>
<AppText>{marks[state]}</AppText>
{state && <AppText>{marks[state]}</AppText>}
Copy link

@eliaslecomte eliaslecomte Oct 11, 2023

Choose a reason for hiding this comment

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

It's better to use a terniary in views, so:

{state ? <AppText>{marks[state]}</AppText> : null}

If a boolean false would be passed to state it would now visualize False which probably is not intended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure? As far as I know, visualizing False was removed a few years ago. When conditional rendering gets false it just not rendering (renders nothing in this place). The problem will only appear when you use the number 0 as a condition. The component allows you to pass only 'DONE' | 'WAITING' | 'ERROR' | 'IN_PROGRESS' values in this place. As a result, in this case, adding : null does not help at all, but prolongs the code. To be fully safe, all we need to do is parse the condition to boolean.
eg.
Screenshot 2023-10-11 at 11 02 17

@gmiszewski-intent
Copy link
Collaborator Author

I added one more small fix for Android.
Because of native library that we use, we need to handle situation where user runs connect method multiple times.

There was a bug when user tried to run connectWithDevice method multiple times.

@gmiszewski-intent gmiszewski-intent merged commit c1cdb03 into master Oct 16, 2023
9 checks passed
@gmiszewski-intent gmiszewski-intent deleted the bugfix/handle-android-connecton-problems branch October 16, 2023 09:53
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