chipweinberger / flutter_blue_plus

Flutter plugin for connecting and communicationg with Bluetooth Low Energy devices, on Android, iOS, macOS
Other
791 stars 478 forks source link

Allow custom log handling #830

Closed benthillerkus closed 7 months ago

benthillerkus commented 8 months ago

This addresses concerns raised in #455, #190 and #153.

Using FlutterBluePlus.setLogger it is now possible to provide your own method to print logs.

If such a method is provided, the native plugins will send their logs over the MethodChannel into Dart land, so that they can be handled by the user too.

Users can now programmatically handle logs, use debugPrint over print or a logging package such as logging or logger if they want.

The API currently exposes the message, the level and the domain of the message (i.e. [FBP] [Android] etc.).


It is also now possible to set both the LogLevel and the Logger inside of setOptions. This addresses the problem where even if the first call into FlutterBluePlus is

FlutterBluePlus.setLogLevel(LogLevel.none)

some native logs would still be printed to stdout before the setLogLevel method would be handled.

Since setOptions is sort of fast-tracked to be able to run before the rest of the plugin is initialised, I added options for a custom Logger and LogLevel. That way that problem can be avoided.

Also fixes the bug where on macOS logs would say iOS.

Also changes the return type of setLogLevel from void to Future<void>, though that method had been async beforehand already.


I've updated the example app to use this feature and the docs to explain setOptions and setLogger.

chipweinberger commented 8 months ago

this is a very nice PR. thanks for it

im not sure if i want to merge it

i would just use setLogger. don't change setOptions

chipweinberger commented 8 months ago

instead of

setSendLogsToDart

i would just call it "setLogger" as well

seems clearer

chipweinberger commented 8 months ago

also i don't understand why "domain" is needed. we already know these logs are coming from the native platform

chipweinberger commented 8 months ago

setOptions is just for BLE related options

chipweinberger commented 8 months ago

also you have some cocoa pods and other unrelated changes

chipweinberger commented 8 months ago

last you should know that sending the logs back over the platform channel will cause delay, so logs can appear out of order

im not sure of this is a good idea

chipweinberger commented 8 months ago

also should be a single commit

benthillerkus commented 8 months ago

i would just use setLogger. don't change setOptions

I did that because setOptions runs before any logs are produced, so nothing slips by. I could change setLogLevel and setLogger to get the same preferential treatment that setOptions gets. EDIT: did that in 82ddcd5.

setSendLogsToDart i would just call it "setLogger" as well

Tbh I kinda disagree, because we aren't actually setting the logger on native side; we're just telling it to forward logs back to us.

also i don't understand why "domain" is needed. we already know these logs are coming from the native platform

Yeah, it's basically hardcoded on the two native implementations right now, but the infrastructure is there for both to be more specific if they wanted to. I feel like it's easier to just be consistent here with the Dart impl.

also you have some cocoa pods and other unrelated changes

That's just in the example project and happens automatically when you try to run it. It's just some version check counter that increases or so. The next time you'll edit / run the example project it would go up too, so I typically don't bother with trying to exclude these changes from git. I can revert them though if you want.

last you should know that sending the logs back over the platform channel will cause delay, so logs can appear out of order

Well that applies to all things on the platform channel, doesn't it? It's the best we can do short of rewriting it with ffi.

Should I include a timestamp field in the log?

also should be a single commit

If you click on the small arrow right next to the merge button, GitHub lets you pick "Squash Merge", which will merge everything as a single commit.

The merge options menu on a GitHub Pull Request

(screenshot from a different pull request in a different repository)

chipweinberger commented 8 months ago

in short, too many changes. make the diff & logger feature as minimal as you can.

in a later release we can consider your other changes.

chipweinberger commented 7 months ago

deciding not to merge at this time.

benthillerkus commented 7 months ago

Does that mean "no, I will not merge this in any state" or "I will merge this after the feedback has been addressed" or "I am not interested in this feature" or "This will have to work entirely differently"?

chipweinberger commented 7 months ago

It's a good question. in its current state I do not plan to merge it.

with feedback addressed / in its minimal state, id probably merge it. but still not decided.

I'm not sure if this feature is really useful enough. but if the implementation was small enough, it could maybe justify the added complexity.

chipweinberger commented 2 weeks ago

I added this in 1.34.5

FlutterBluePlus.logs