maplibre / maplibre-native

MapLibre Native - Interactive vector tile maps for iOS, Android and other platforms.
https://maplibre.org
BSD 2-Clause "Simplified" License
925 stars 280 forks source link

Replace `Timber` with `org.maplibre.android.log.Logger` #2576

Open kodebach opened 2 weeks ago

kodebach commented 2 weeks ago

Replace using Timber directly with org.maplibre.android.log.Logger.

It looks like the few places where this is done is after the fork:

https://github.com/maplibre/maplibre-native/blame/main/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapGestureDetector.java#L506


Orignal message:

Is your feature request related to a problem? Please describe. Currently MapLibre uses Timber for logging. If an app also uses Timber for its logging, it will also receive all the log messages from MapLibre without a good way to filter them out.

Describe the solution you'd like The ideal solution would be if MapLibre used its own logging interface. So for example instead of

Timber.e("Some error")

use

mapLibreLogger.e("Some error")

Where mapLibreLogger is an instance of a custom interface Logger. This instance could default to a Timber-based implementation for backwards compatibility, but should be configurable.

Instead of passing the instance to all the places the need logging, it could also be stored in a singleton similar to how Timber does it. But again it should be configurable.

Describe alternatives you've considered A simpler, but less customizable, solution would be to use

Timber.tag("MapLibre").e("Some error")

Then apps could filter out messages based on the MapLibre tag. However, this requires using the tag consistently everywhere, which is inherently error-prone.

Additional context

See also #2575 , for an example why using Timber directly may be annoying to app developers.

louwers commented 1 week ago

Have you tried this? https://maplibre.org/maplibre-native/android/api/-map-libre%20-native%20-android/org.maplibre.android.log/-logger/set-logger-definition.html?query=open%20fun%20setLoggerDefinition(loggerDefinition:%20LoggerDefinition)

It looks like we already have a replaceable logger, but we are using Timber directly instead of using Logger in some places.

kodebach commented 1 week ago

I actually did not know this LoggerDefinition existed. In that case, treat this issue as a tracking issue for removing direct use of Timber.

louwers commented 1 week ago

Looks like the few places where this happens is after the fork: https://github.com/maplibre/maplibre-native/blame/main/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapGestureDetector.java#L506

artakka commented 1 week ago

There are some added advantages in going through our own logging classes, like logging added information, collecting warning errors statistics and sending out metrics, etc...