Skip to content

Conversation

@Guardiola31337
Copy link
Contributor

Description

Bumps mapboxBaseAndroid version to 0.6.0.

@Guardiola31337 Guardiola31337 added Core Work related to core navigation and integrations. skip changelog Should not be added into version changelog. labels Mar 28, 2022
@Guardiola31337 Guardiola31337 self-assigned this Mar 28, 2022
@Guardiola31337 Guardiola31337 requested a review from a team as a code owner March 28, 2022 17:27
@LukasPaczos
Copy link

We'll need to change the default log level as well (since this upgrade changed it to error).

@Guardiola31337
Copy link
Contributor Author

@LukasPaczos

We'll need to change the default log level as well (since this upgrade changed it to error).

What do you mean? Isn't that already the case https://github.com/mapbox/mapbox-base-android/blob/1317c85a24cc15e31b922ff6efb146590f71e6a5/liblogger/src/main/java/com/mapbox/common/logger/MapboxLogger.kt#L28

That's the default MapboxLogger used in

internal val logger = MapboxModuleProvider.createModule<Logger>(
MapboxModuleType.CommonLogger
) {
arrayOf()
}
(if not overridden by the client) and what we end up using
LoggingLevel.DEBUG -> {
LoggerProvider.logger.d(tag = tag, msg = msg)
}
LoggingLevel.INFO -> {
LoggerProvider.logger.i(tag = tag, msg = msg)
}
LoggingLevel.WARNING -> {
LoggerProvider.logger.w(tag = tag, msg = msg)
}
LoggingLevel.ERROR -> {
LoggerProvider.logger.e(tag = tag, msg = msg)
}
else -> {
LoggerProvider.logger.v(tag = tag, msg = msg)
}
🤔

@LukasPaczos
Copy link

@Guardiola31337 the v0.6.0 of based change that the default log level of the default logger instance is error, so only errors are logged. Since we control the log level via Common SDK instead, we need to relax that on the Android logger level:

diff --git a/libnavigation-util/build.gradle b/libnavigation-util/build.gradle
index fae24295cf..bf3fe0b109 100644
--- a/libnavigation-util/build.gradle
+++ b/libnavigation-util/build.gradle
@@ -32,6 +32,7 @@ dependencies {
     api dependenciesList.mapboxCommonNative
     implementation dependenciesList.mapboxCommonOkHttp
     api dependenciesList.mapboxAndroidCommon
+    implementation dependenciesList.mapboxLogger
     implementation dependenciesList.mapboxAnnotations
 
     apply from: "${rootDir}/gradle/unit-testing-dependencies.gradle"
diff --git a/libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerProvider.kt b/libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerProvider.kt
index be2409cd3a..81f84a14c0 100644
--- a/libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerProvider.kt
+++ b/libnavigation-util/src/main/java/com/mapbox/navigation/utils/internal/LoggerProvider.kt
@@ -7,6 +7,8 @@ import com.mapbox.base.common.logger.model.Tag
 import com.mapbox.common.LogConfiguration
 import com.mapbox.common.LogWriterBackend
 import com.mapbox.common.LoggingLevel
+import com.mapbox.common.logger.MapboxLogger
+import com.mapbox.common.logger.VERBOSE
 import com.mapbox.common.module.provider.MapboxModuleProvider
 
 private const val TAG = "Mapbox"
@@ -25,6 +27,10 @@ object LoggerProvider {
         MapboxModuleType.CommonLogger
     ) {
         arrayOf()
+    }.apply {
+        if (this is MapboxLogger) {
+            this.logLevel = VERBOSE
+        }
     }
 }

@LukasPaczos
Copy link

Well, the problem now arises with the fact that the logger module is designed to be excluded from the build, which means that we should run into a missing class reference during the cast attempt if a custom logger is used (and the default logger is excluded).

@DzmitryFomchyn @tobrun thinking whether we shouldn't revert mapbox/mapbox-base-android#65 in the end - a library (like Nav SDK) that uses the modularization setup cannot have access to the default implementations, they should be defined as runtimeOnly so that they can be safely excluded. Since the log level was not defined in the loggers interface, we probably need to leave it at verbose.

We face a similar problem in the Common SDK which currently strongly links to MapboxLogger to override the default log level.

I guess there would be an option of using reflection to check if the instance is MapboxLogger but is that worth it? Log level should be controlled through Common SDK anyway.

@Guardiola31337
Copy link
Contributor Author

This is exactly what I was thinking. We should probably leave it as is (as long as we want to provide that default logLevel). If a user decides to inject a custom logger they can implement whatever logic they'd like, if default MapboxLogger is used, the logLevel is whatever we think is best.

Also, I thought about reflection as well but I totally agree that it's not worth it here, especially because this is going to be controlled through Common SDK (as above noted).

@LukasPaczos
Copy link

Closing in favor of #5742 which resolves the concern raised in #5620 (comment).

@Guardiola31337 Guardiola31337 deleted the pg-bump-mapbox-base-android branch April 25, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core Work related to core navigation and integrations. skip changelog Should not be added into version changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants