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

Only set the log level *if* bluetooth is available. #288

Merged
merged 4 commits into from
Apr 8, 2020

Conversation

efortuna
Copy link
Contributor

Without this change, setLogLevel inside FlutterBlue.instance throws a PlatformException

@CLAassistant
Copy link

CLAassistant commented Jun 26, 2019

CLA assistant check
All committers have signed the CLA.

@andretietz
Copy link

Please accept the CLA, so that someone can merge this. Or let us know if you don't want to, so I would create the PR.

@efortuna
Copy link
Contributor Author

My understanding is some other folks (Meidi Tönisson & co) made a version of this CL and submitted it to the repo for me already. this PR can probably be closed.

@andretietz
Copy link

It doesn't seem to be in the master yet, so if there's a PR open for it already, please link it and ask @pauldemarco to close this PR.

@efortuna
Copy link
Contributor Author

ah, hm. Okay. Let me get the CLA okay-ed by Google's lawyers since this change was done on company time. I'll get back with you shortly I hope!

@efortuna
Copy link
Contributor Author

efortuna commented Aug 29, 2019

Hi @andretietz I got the approval from Google lawyers. Can you take another look at this PR?

@andretietz
Copy link

Looks good so far, I personally would remove the version change, since this is up to @pauldemarco to decide. If he merges something else before and increasing the version number, he will change this anyways.

pubspec.yaml Outdated Show resolved Hide resolved
@efortuna
Copy link
Contributor Author

efortuna commented Sep 6, 2019

made the requested changes

@andretietz
Copy link

@efortuna Did you check that it doesn't crash, if there's a log before setting the loglevel?

@pauldemarco
Copy link
Owner

Hi @efortuna @andretietz,

Sorry for the delay on this, I have some questions.

It was my intention that setting the log level should succeed whether or not Bluetooth is available. On future platforms, we may need to set verbosity during times when Bluetooth is unavailable.

It looks like this will only error out if FlutterBlue is being used on an unsupported platform... something other than Android/iOS. Was this the case?

@efortuna
Copy link
Contributor Author

It's been a long time since I encountered this issue so I could be wrong, but if memory serves, this happens if you run it on an Android or iOS simulator? IIRC you have to construct a FlutterBlue object to be able to check the "platform is supported" call (it's not a static method) so you have to construct an object to do that check. Hence this constructor dealing with the resulting crash of setting logging on an "unsupported platform" (aka a simulator).

@andretietz
Copy link

It's basically crashing when (for some reason) running on a device that doesn't support bluetooth. Usually this cannot happen, since the play store is not showing apps to devices without bluetooth if bluetooth is required, but in cases of manually installing an apk and the emulator, this just crashes.

@efortuna
Copy link
Contributor Author

efortuna commented Oct 1, 2019

This happens every time you run on a simulator. Which is a very common occurrence if you're developing an app. And this plugin shouldn't crash just because a user has run it on some "unsupported device" (perhaps they have bluetooth off?).

@pauldemarco pauldemarco merged commit df44e5c into pauldemarco:master Apr 8, 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.

4 participants