From 32d9a183aed5859ab6bb32bf75640d17385cac13 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 14:21:12 +0100 Subject: [PATCH 1/9] Add BuildMeta to the project --- .../io/element/android/x/di/AppModule.kt | 18 +++++++++++ .../android/libraries/core/meta/BuildMeta.kt | 30 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt diff --git a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt index b83a98f7f3..2621045c83 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt @@ -21,9 +21,12 @@ import com.squareup.anvil.annotations.ContributesTo import dagger.Module import dagger.Provides import io.element.android.libraries.core.coroutine.CoroutineDispatchers +import io.element.android.libraries.core.meta.BuildMeta import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.di.SingleIn +import io.element.android.x.BuildConfig +import io.element.android.x.R import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -48,6 +51,21 @@ object AppModule { return MainScope() + CoroutineName("ElementX Scope") } + @Provides + @SingleIn(AppScope::class) + fun providesBuildMeta(@ApplicationContext context: Context) = BuildMeta( + isDebug = BuildConfig.DEBUG, + applicationName = context.getString(R.string.app_name), + applicationId = BuildConfig.APPLICATION_ID, + lowPrivacyLoggingEnabled = false, // TODO EAx Config.LOW_PRIVACY_LOG_ENABLE, + versionName = BuildConfig.VERSION_NAME, + gitRevision = "TODO", // BuildConfig.GIT_REVISION, + gitRevisionDate = "TODO", // BuildConfig.GIT_REVISION_DATE, + gitBranchName = "TODO", // BuildConfig.GIT_BRANCH_NAME, + flavorDescription = "TODO", // BuildConfig.FLAVOR_DESCRIPTION, + flavorShortDescription = "TODO", // BuildConfig.SHORT_FLAVOR_DESCRIPTION, + ) + @Provides @SingleIn(AppScope::class) fun providesCoroutineDispatchers(): CoroutineDispatchers { diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt new file mode 100644 index 0000000000..35deb30dc0 --- /dev/null +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.core.meta + +data class BuildMeta( + val isDebug: Boolean, + val applicationName: String, + val applicationId: String, + val lowPrivacyLoggingEnabled: Boolean, + val versionName: String, + val gitRevision: String, + val gitRevisionDate: String, + val gitBranchName: String, + val flavorDescription: String, + val flavorShortDescription: String, +) From a68b3f80ebafaaeff8b2c316484dde46b4f429d0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 15:31:27 +0100 Subject: [PATCH 2/9] Add `:libraries:network` module --- app/build.gradle.kts | 3 + .../io/element/android/x/di/AppModule.kt | 2 + gradle/libs.versions.toml | 5 ++ libraries/core/build.gradle.kts | 3 +- .../android/libraries/core/meta/BuildMeta.kt | 23 +++--- libraries/network/build.gradle.kts | 41 ++++++++++ .../libraries/network/NetworkModule.kt | 58 ++++++++++++++ .../libraries/network/RetrofitFactory.kt | 38 ++++++++++ .../interceptors/FormattedJsonHttpLogger.kt | 75 +++++++++++++++++++ .../kotlin/extension/DependencyHandleScope.kt | 1 + settings.gradle.kts | 1 + 11 files changed, 239 insertions(+), 11 deletions(-) create mode 100644 libraries/network/build.gradle.kts create mode 100644 libraries/network/src/main/kotlin/io/element/android/libraries/network/NetworkModule.kt create mode 100644 libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt create mode 100644 libraries/network/src/main/kotlin/io/element/android/libraries/network/interceptors/FormattedJsonHttpLogger.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index a7d44ac510..effa886c4d 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -219,6 +219,9 @@ dependencies { implementation(libs.androidx.startup) implementation(libs.coil) + implementation(platform(libs.network.okhttp.bom)) + implementation("com.squareup.okhttp3:logging-interceptor") + implementation(libs.dagger) kapt(libs.dagger.compiler) diff --git a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt index 2621045c83..c553f3f7f0 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt @@ -33,6 +33,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.MainScope import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.plus +import okhttp3.logging.HttpLoggingInterceptor import java.io.File import java.util.concurrent.Executors @@ -64,6 +65,7 @@ object AppModule { gitBranchName = "TODO", // BuildConfig.GIT_BRANCH_NAME, flavorDescription = "TODO", // BuildConfig.FLAVOR_DESCRIPTION, flavorShortDescription = "TODO", // BuildConfig.SHORT_FLAVOR_DESCRIPTION, + okHttpLoggingLevel = if (BuildConfig.DEBUG) HttpLoggingInterceptor.Level.BODY else HttpLoggingInterceptor.Level.BASIC, ) @Provides diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index c1616b878e..68b8dbc8af 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -92,6 +92,11 @@ accompanist_flowlayout = { module = "com.google.accompanist:accompanist-flowlayo # Libraries squareup_seismic = "com.squareup:seismic:1.0.3" +# network +network_okhttp_bom = "com.squareup.okhttp3:okhttp-bom:4.10.0" +network_retrofit = "com.squareup.retrofit2:retrofit:2.9.0" +network_retrofit_converter_serialization = "com.jakewharton.retrofit:retrofit2-kotlinx-serialization-converter:0.8.0" + # Test test_core = { module = "androidx.test:core", version.ref = "test_core" } test_corektx = { module = "androidx.test:core-ktx", version.ref = "test_core" } diff --git a/libraries/core/build.gradle.kts b/libraries/core/build.gradle.kts index ef4a882cb3..7a576b356b 100644 --- a/libraries/core/build.gradle.kts +++ b/libraries/core/build.gradle.kts @@ -1,4 +1,3 @@ - /* * Copyright (c) 2022 New Vector Ltd * @@ -29,4 +28,6 @@ java { dependencies { implementation(libs.coroutines.core) + implementation(platform(libs.network.okhttp.bom)) + implementation("com.squareup.okhttp3:logging-interceptor") } diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt index 35deb30dc0..8fefe19919 100644 --- a/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/meta/BuildMeta.kt @@ -16,15 +16,18 @@ package io.element.android.libraries.core.meta +import okhttp3.logging.HttpLoggingInterceptor + data class BuildMeta( - val isDebug: Boolean, - val applicationName: String, - val applicationId: String, - val lowPrivacyLoggingEnabled: Boolean, - val versionName: String, - val gitRevision: String, - val gitRevisionDate: String, - val gitBranchName: String, - val flavorDescription: String, - val flavorShortDescription: String, + val isDebug: Boolean, + val applicationName: String, + val applicationId: String, + val lowPrivacyLoggingEnabled: Boolean, + val versionName: String, + val gitRevision: String, + val gitRevisionDate: String, + val gitBranchName: String, + val flavorDescription: String, + val flavorShortDescription: String, + val okHttpLoggingLevel: HttpLoggingInterceptor.Level, ) diff --git a/libraries/network/build.gradle.kts b/libraries/network/build.gradle.kts new file mode 100644 index 0000000000..fb242d4f83 --- /dev/null +++ b/libraries/network/build.gradle.kts @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +plugins { + id("io.element.android-library") + alias(libs.plugins.anvil) +} + +android { + namespace = "io.element.android.libraries.network" +} + +anvil { + generateDaggerFactories.set(true) +} + +dependencies { + implementation(libs.dagger) + implementation(projects.libraries.core) + implementation(projects.libraries.di) + implementation(platform(libs.network.okhttp.bom)) + implementation("com.squareup.okhttp3:okhttp") + implementation("com.squareup.okhttp3:logging-interceptor") + + implementation(libs.network.retrofit) + implementation(libs.network.retrofit.converter.serialization) + implementation(libs.serialization.json) +} diff --git a/libraries/network/src/main/kotlin/io/element/android/libraries/network/NetworkModule.kt b/libraries/network/src/main/kotlin/io/element/android/libraries/network/NetworkModule.kt new file mode 100644 index 0000000000..69c1d78369 --- /dev/null +++ b/libraries/network/src/main/kotlin/io/element/android/libraries/network/NetworkModule.kt @@ -0,0 +1,58 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.network + +import com.squareup.anvil.annotations.ContributesTo +import dagger.Module +import dagger.Provides +import io.element.android.libraries.core.meta.BuildMeta +import io.element.android.libraries.di.AppScope +import io.element.android.libraries.di.SingleIn +import okhttp3.OkHttpClient +import okhttp3.Protocol +import io.element.android.libraries.network.interceptors.FormattedJsonHttpLogger +import java.util.concurrent.TimeUnit +import okhttp3.logging.HttpLoggingInterceptor + +@Module +@ContributesTo(AppScope::class) +object NetworkModule { + + @Provides + @JvmStatic + fun providesHttpLoggingInterceptor(buildMeta: BuildMeta): HttpLoggingInterceptor { + val logger = FormattedJsonHttpLogger(buildMeta.okHttpLoggingLevel) + val interceptor = HttpLoggingInterceptor(logger) + interceptor.level = buildMeta.okHttpLoggingLevel + return interceptor + } + + @Provides + @SingleIn(AppScope::class) + fun providesOkHttpClient( + httpLoggingInterceptor: HttpLoggingInterceptor, + ): OkHttpClient { + return OkHttpClient.Builder() + // workaround for #4669 + .protocols(listOf(Protocol.HTTP_1_1)) + .connectTimeout(30, TimeUnit.SECONDS) + .readTimeout(60, TimeUnit.SECONDS) + .writeTimeout(60, TimeUnit.SECONDS) + .addInterceptor(httpLoggingInterceptor) + .build() + } +} diff --git a/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt b/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt new file mode 100644 index 0000000000..cba09525f9 --- /dev/null +++ b/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.network + +import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import io.element.android.libraries.core.uri.ensureTrailingSlash +import kotlinx.serialization.json.Json +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import retrofit2.Retrofit +import javax.inject.Inject + +class RetrofitFactory @Inject constructor( + private val okHttpClient: OkHttpClient, +) { + fun create(baseUrl: String): Retrofit { + val contentType = "application/json".toMediaType() + return Retrofit.Builder() + .baseUrl(baseUrl.ensureTrailingSlash()) + .addConverterFactory(Json.asConverterFactory(contentType)) + .client(okHttpClient) + .build() + } +} diff --git a/libraries/network/src/main/kotlin/io/element/android/libraries/network/interceptors/FormattedJsonHttpLogger.kt b/libraries/network/src/main/kotlin/io/element/android/libraries/network/interceptors/FormattedJsonHttpLogger.kt new file mode 100644 index 0000000000..d6c3144c5c --- /dev/null +++ b/libraries/network/src/main/kotlin/io/element/android/libraries/network/interceptors/FormattedJsonHttpLogger.kt @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.network.interceptors + +import okhttp3.logging.HttpLoggingInterceptor +import org.json.JSONArray +import org.json.JSONException +import org.json.JSONObject +import timber.log.Timber + +internal class FormattedJsonHttpLogger( + private val level: HttpLoggingInterceptor.Level +) : HttpLoggingInterceptor.Logger { + + companion object { + private const val INDENT_SPACE = 2 + } + + /** + * Log the message and try to log it again as a JSON formatted string. + * Note: it can consume a lot of memory but it is only in DEBUG mode. + * + * @param message + */ + @Synchronized + override fun log(message: String) { + Timber.v(message) + + // Try to log formatted Json only if there is a chance that [message] contains Json. + // It can be only the case if we log the bodies of Http requests. + if (level != HttpLoggingInterceptor.Level.BODY) return + + if (message.startsWith("{")) { + // JSON Detected + try { + val o = JSONObject(message) + logJson(o.toString(INDENT_SPACE)) + } catch (e: JSONException) { + // Finally this is not a JSON string... + Timber.e(e) + } + } else if (message.startsWith("[")) { + // JSON Array detected + try { + val o = JSONArray(message) + logJson(o.toString(INDENT_SPACE)) + } catch (e: JSONException) { + // Finally not JSON... + Timber.e(e) + } + } + // Else not a json string to log + } + + private fun logJson(formattedJson: String) { + formattedJson + .lines() + .dropLastWhile { it.isEmpty() } + .forEach { Timber.v(it) } + } +} diff --git a/plugins/src/main/kotlin/extension/DependencyHandleScope.kt b/plugins/src/main/kotlin/extension/DependencyHandleScope.kt index 426be4fcdb..3d134bcab4 100644 --- a/plugins/src/main/kotlin/extension/DependencyHandleScope.kt +++ b/plugins/src/main/kotlin/extension/DependencyHandleScope.kt @@ -55,6 +55,7 @@ fun DependencyHandlerScope.allLibrariesImpl() { implementation(project(":libraries:designsystem")) implementation(project(":libraries:matrix:impl")) implementation(project(":libraries:matrixui")) + implementation(project(":libraries:network")) implementation(project(":libraries:core")) implementation(project(":libraries:architecture")) implementation(project(":libraries:dateformatter:impl")) diff --git a/settings.gradle.kts b/settings.gradle.kts index e28b354cf3..7a534a5778 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -49,6 +49,7 @@ include(":libraries:dateformatter:api") include(":libraries:dateformatter:impl") include(":libraries:dateformatter:test") include(":libraries:elementresources") +include(":libraries:network") include(":libraries:ui-strings") include(":libraries:testtags") include(":libraries:designsystem") From 771855dfe375be453fc0306c0b87d1a004de0ef9 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 14:47:41 +0100 Subject: [PATCH 3/9] Let coil use our OkHttpClient. --- .../libraries/matrix/ui/media/ImageLoaderFactories.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt index cf89678318..a52cfd380a 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt @@ -21,15 +21,18 @@ import coil.ImageLoader import coil.ImageLoaderFactory import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.matrix.api.MatrixClient +import okhttp3.OkHttpClient import javax.inject.Inject class LoggedInImageLoaderFactory @Inject constructor( @ApplicationContext private val context: Context, private val matrixClient: MatrixClient, + private val okHttpClient: OkHttpClient, ) : ImageLoaderFactory { override fun newImageLoader(): ImageLoader { return ImageLoader .Builder(context) + .okHttpClient(okHttpClient) .components { add(AvatarKeyer()) add(MediaKeyer()) @@ -42,10 +45,12 @@ class LoggedInImageLoaderFactory @Inject constructor( class NotLoggedInImageLoaderFactory @Inject constructor( @ApplicationContext private val context: Context, + private val okHttpClient: OkHttpClient, ) : ImageLoaderFactory { override fun newImageLoader(): ImageLoader { return ImageLoader .Builder(context) + .okHttpClient(okHttpClient) .build() } } From 0d73b962c2f1c83ed54c8189ab8792eae370a80e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 15:11:55 +0100 Subject: [PATCH 4/9] Add explicit dependency to okhttp (was provided by coil), and inject the OkHttpClient to DefaultBugReporter. --- features/rageshake/impl/build.gradle.kts | 2 ++ .../features/rageshake/impl/reporter/DefaultBugReporter.kt | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/features/rageshake/impl/build.gradle.kts b/features/rageshake/impl/build.gradle.kts index c23bbe74cc..d09bb4025d 100644 --- a/features/rageshake/impl/build.gradle.kts +++ b/features/rageshake/impl/build.gradle.kts @@ -43,6 +43,8 @@ dependencies { api(libs.squareup.seismic) api(projects.features.rageshake.api) implementation(libs.androidx.datastore.preferences) + implementation(platform(libs.network.okhttp.bom)) + implementation("com.squareup.okhttp3:okhttp") implementation(libs.coil) implementation(libs.coil.compose) ksp(libs.showkase.processor) diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt index 27bae6ee72..4e031703ab 100755 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt @@ -64,6 +64,7 @@ class DefaultBugReporter @Inject constructor( private val screenshotHolder: ScreenshotHolder, private val crashDataStore: CrashDataStore, private val coroutineDispatchers: CoroutineDispatchers, + private val okHttpClient: OkHttpClient, /* private val activeSessionHolder: ActiveSessionHolder, private val versionProvider: VersionProvider, @@ -88,9 +89,6 @@ class DefaultBugReporter @Inject constructor( private const val BUFFER_SIZE = 1024 * 1024 * 50 } - // the http client - private val mOkHttpClient = OkHttpClient() - // the pending bug report call private var mBugReportCall: Call? = null @@ -346,7 +344,7 @@ class DefaultBugReporter @Inject constructor( // trigger the request try { - mBugReportCall = mOkHttpClient.newCall(request) + mBugReportCall = okHttpClient.newCall(request) response = mBugReportCall!!.execute() responseCode = response.code } catch (e: Exception) { From fc2dfacd7e40c2dace973b4f23f229668d0130a8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 15:46:52 +0100 Subject: [PATCH 5/9] Change API of BugReporter (make it suspend) --- .../rageshake/api/reporter/BugReporter.kt | 8 +- .../impl/bugreport/BugReportPresenter.kt | 1 - .../impl/reporter/DefaultBugReporter.kt | 536 +++++++++--------- .../impl/bugreport/FakeBugReporter.kt | 41 +- 4 files changed, 284 insertions(+), 302 deletions(-) diff --git a/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt b/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt index deddbd4faa..0af13dcdda 100644 --- a/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt +++ b/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt @@ -16,15 +16,10 @@ package io.element.android.features.rageshake.api.reporter -import io.element.android.features.rageshake.api.reporter.BugReporterListener -import io.element.android.features.rageshake.api.reporter.ReportType -import kotlinx.coroutines.CoroutineScope - interface BugReporter { /** * Send a bug report. * - * @param coroutineScope The coroutine scope * @param reportType The report type (bug, suggestion, feedback) * @param withDevicesLogs true to include the device log * @param withCrashLogs true to include the crash logs @@ -36,8 +31,7 @@ interface BugReporter { * @param customFields fields which will be sent with the report * @param listener the listener */ - fun sendBugReport( - coroutineScope: CoroutineScope, + suspend fun sendBugReport( reportType: ReportType, withDevicesLogs: Boolean, withCrashLogs: Boolean, diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt index bce4e96709..5acb2bd4b1 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt @@ -132,7 +132,6 @@ class BugReportPresenter @Inject constructor( listener: BugReporterListener, ) = launch { bugReporter.sendBugReport( - coroutineScope = this, reportType = ReportType.BUG_REPORT, withDevicesLogs = formState.sendLogs, withCrashLogs = hasCrashLogs && formState.sendCrashLogs, diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt index 4e031703ab..d9e56a91bc 100755 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt @@ -21,13 +21,13 @@ import android.os.Build import androidx.core.net.toFile import androidx.core.net.toUri import com.squareup.anvil.annotations.ContributesBinding +import io.element.android.features.rageshake.api.crash.CrashDataStore import io.element.android.features.rageshake.api.reporter.BugReporter import io.element.android.features.rageshake.api.reporter.BugReporterListener import io.element.android.features.rageshake.api.reporter.ReportType -import io.element.android.features.rageshake.impl.R -import io.element.android.features.rageshake.api.crash.CrashDataStore -import io.element.android.features.rageshake.impl.logs.VectorFileLogger import io.element.android.features.rageshake.api.screenshot.ScreenshotHolder +import io.element.android.features.rageshake.impl.R +import io.element.android.features.rageshake.impl.logs.VectorFileLogger import io.element.android.libraries.androidutils.file.compressFile import io.element.android.libraries.androidutils.file.safeDelete import io.element.android.libraries.core.coroutine.CoroutineDispatchers @@ -35,9 +35,7 @@ import io.element.android.libraries.core.extensions.toOnOff import io.element.android.libraries.core.mimetype.MimeTypes import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.ApplicationContext -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.first -import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import okhttp3.Call import okhttp3.MediaType.Companion.toMediaTypeOrNull @@ -116,7 +114,6 @@ class DefaultBugReporter @Inject constructor( /** * Send a bug report. * - * @param coroutineScope The coroutine scope * @param reportType The report type (bug, suggestion, feedback) * @param withDevicesLogs true to include the device log * @param withCrashLogs true to include the crash logs @@ -128,8 +125,7 @@ class DefaultBugReporter @Inject constructor( * @param customFields fields which will be sent with the report * @param listener the listener */ - override fun sendBugReport( - coroutineScope: CoroutineScope, + override suspend fun sendBugReport( reportType: ReportType, withDevicesLogs: Boolean, withCrashLogs: Boolean, @@ -144,282 +140,280 @@ class DefaultBugReporter @Inject constructor( // enumerate files to delete val mBugReportFiles: MutableList = ArrayList() - coroutineScope.launch { - var serverError: String? = null - var reportURL: String? = null - withContext(coroutineDispatchers.io) { - var bugDescription = theBugDescription - val crashCallStack = crashDataStore.crashInfo().first() + var serverError: String? = null + var reportURL: String? = null + withContext(coroutineDispatchers.io) { + var bugDescription = theBugDescription + val crashCallStack = crashDataStore.crashInfo().first() - if (crashCallStack.isNotEmpty() && withCrashLogs) { - bugDescription += "\n\n\n\n--------------------------------- crash call stack ---------------------------------\n" - bugDescription += crashCallStack - } + if (crashCallStack.isNotEmpty() && withCrashLogs) { + bugDescription += "\n\n\n\n--------------------------------- crash call stack ---------------------------------\n" + bugDescription += crashCallStack + } - val gzippedFiles = ArrayList() + val gzippedFiles = ArrayList() - val vectorFileLogger = VectorFileLogger.getFromTimber() - if (withDevicesLogs && vectorFileLogger != null) { - val files = vectorFileLogger.getLogFiles() - files.mapNotNullTo(gzippedFiles) { f -> - if (!mIsCancelled) { - compressFile(f) - } else { - null - } - } - } - - if (!mIsCancelled && (withCrashLogs || withDevicesLogs)) { - val gzippedLogcat = saveLogCat(false) - - if (null != gzippedLogcat) { - if (gzippedFiles.size == 0) { - gzippedFiles.add(gzippedLogcat) - } else { - gzippedFiles.add(0, gzippedLogcat) - } - } - } - - /* - activeSessionHolder.getSafeActiveSession() - ?.takeIf { !mIsCancelled && withKeyRequestHistory } - ?.cryptoService() - ?.getGossipingEvents() - ?.let { GossipingEventsSerializer().serialize(it) } - ?.toByteArray() - ?.let { rawByteArray -> - File(context.cacheDir.absolutePath, KEY_REQUESTS_FILENAME) - .also { - it.outputStream() - .use { os -> os.write(rawByteArray) } - } - } - ?.let { compressFile(it) } - ?.let { gzippedFiles.add(it) } - */ - - var deviceId = "undefined" - var userId = "undefined" - var olmVersion = "undefined" - - /* - activeSessionHolder.getSafeActiveSession()?.let { session -> - userId = session.myUserId - deviceId = session.sessionParams.deviceId ?: "undefined" - olmVersion = session.cryptoService().getCryptoVersion(context, true) - } - */ - - if (!mIsCancelled) { - val text = when (reportType) { - ReportType.BUG_REPORT -> bugDescription - ReportType.SUGGESTION -> "[Suggestion] $bugDescription" - ReportType.SPACE_BETA_FEEDBACK -> "[spaces-feedback] $bugDescription" - ReportType.THREADS_BETA_FEEDBACK -> "[threads-feedback] $bugDescription" - ReportType.AUTO_UISI_SENDER, - ReportType.AUTO_UISI -> bugDescription - } - - // build the multi part request - val builder = BugReporterMultipartBody.Builder() - .addFormDataPart("text", text) - .addFormDataPart("app", rageShakeAppNameForReport(reportType)) - // .addFormDataPart("user_agent", matrix.getUserAgent()) - .addFormDataPart("user_id", userId) - .addFormDataPart("can_contact", canContact.toString()) - .addFormDataPart("device_id", deviceId) - // .addFormDataPart("version", versionProvider.getVersion(longFormat = true)) - // .addFormDataPart("branch_name", buildMeta.gitBranchName) - // .addFormDataPart("matrix_sdk_version", Matrix.getSdkVersion()) - .addFormDataPart("olm_version", olmVersion) - .addFormDataPart("device", Build.MODEL.trim()) - // .addFormDataPart("verbose_log", vectorPreferences.labAllowedExtendedLogging().toOnOff()) - .addFormDataPart("multi_window", inMultiWindowMode.toOnOff()) - // .addFormDataPart( - // "os", Build.VERSION.RELEASE + " (API " + sdkIntProvider.get() + ") " + - // Build.VERSION.INCREMENTAL + "-" + Build.VERSION.CODENAME - // ) - .addFormDataPart("locale", Locale.getDefault().toString()) - // .addFormDataPart("app_language", vectorLocale.applicationLocale.toString()) - // .addFormDataPart("default_app_language", systemLocaleProvider.getSystemLocale().toString()) - // .addFormDataPart("theme", ThemeUtils.getApplicationTheme(context)) - .addFormDataPart("server_version", serverVersion) - .apply { - customFields?.forEach { (name, value) -> - addFormDataPart(name, value) - } - } - - // add the gzipped files - for (file in gzippedFiles) { - builder.addFormDataPart("compressed-log", file.name, file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())) - } - - mBugReportFiles.addAll(gzippedFiles) - - if (withScreenshot) { - screenshotHolder.getFileUri() - ?.toUri() - ?.toFile() - ?.let { screenshotFile -> - try { - builder.addFormDataPart( - "file", - screenshotFile.name, screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) - ) - } catch (e: Exception) { - Timber.e(e, "## sendBugReport() : fail to write screenshot") - } - } - } - - // add some github labels - // builder.addFormDataPart("label", buildMeta.versionName) - // builder.addFormDataPart("label", buildMeta.flavorDescription) - // builder.addFormDataPart("label", buildMeta.gitBranchName) - - // Possible values for BuildConfig.BUILD_TYPE: "debug", "nightly", "release". - // builder.addFormDataPart("label", BuildConfig.BUILD_TYPE) - - when (reportType) { - ReportType.BUG_REPORT -> { - /* nop */ - } - ReportType.SUGGESTION -> builder.addFormDataPart("label", "[Suggestion]") - ReportType.SPACE_BETA_FEEDBACK -> builder.addFormDataPart("label", "spaces-feedback") - ReportType.THREADS_BETA_FEEDBACK -> builder.addFormDataPart("label", "threads-feedback") - ReportType.AUTO_UISI -> { - builder.addFormDataPart("label", "Z-UISI") - builder.addFormDataPart("label", "android") - builder.addFormDataPart("label", "uisi-recipient") - } - ReportType.AUTO_UISI_SENDER -> { - builder.addFormDataPart("label", "Z-UISI") - builder.addFormDataPart("label", "android") - builder.addFormDataPart("label", "uisi-sender") - } - } - - if (crashCallStack.isNotEmpty() && withCrashLogs) { - builder.addFormDataPart("label", "crash") - } - - val requestBody = builder.build() - - // add a progress listener - requestBody.setWriteListener { totalWritten, contentLength -> - val percentage = if (-1L != contentLength) { - if (totalWritten > contentLength) { - 100 - } else { - (totalWritten * 100 / contentLength).toInt() - } - } else { - 0 - } - - if (mIsCancelled && null != mBugReportCall) { - mBugReportCall!!.cancel() - } - - Timber.v("## onWrite() : $percentage%") - try { - listener?.onProgress(percentage) - } catch (e: Exception) { - Timber.e(e, "## onProgress() : failed") - } - } - - // build the request - val request = Request.Builder() - .url(context.getString(R.string.bug_report_url)) - .post(requestBody) - .build() - - var responseCode = HttpURLConnection.HTTP_INTERNAL_ERROR - var response: Response? = null - var errorMessage: String? = null - - // trigger the request - try { - mBugReportCall = okHttpClient.newCall(request) - response = mBugReportCall!!.execute() - responseCode = response.code - } catch (e: Exception) { - Timber.e(e, "response") - errorMessage = e.localizedMessage - } - - // if the upload failed, try to retrieve the reason - if (responseCode != HttpURLConnection.HTTP_OK) { - if (null != errorMessage) { - serverError = "Failed with error $errorMessage" - } else if (response?.body == null) { - serverError = "Failed with error $responseCode" - } else { - try { - val inputStream = response.body!!.byteStream() - - serverError = inputStream.use { - buildString { - var ch = it.read() - while (ch != -1) { - append(ch.toChar()) - ch = it.read() - } - } - } - - // check if the error message - serverError?.let { - try { - val responseJSON = JSONObject(it) - serverError = responseJSON.getString("error") - } catch (e: JSONException) { - Timber.e(e, "doInBackground ; Json conversion failed") - } - } - - // should never happen - if (null == serverError) { - serverError = "Failed with error $responseCode" - } - } catch (e: Exception) { - Timber.e(e, "## sendBugReport() : failed to parse error") - } - } + val vectorFileLogger = VectorFileLogger.getFromTimber() + if (withDevicesLogs && vectorFileLogger != null) { + val files = vectorFileLogger.getLogFiles() + files.mapNotNullTo(gzippedFiles) { f -> + if (!mIsCancelled) { + compressFile(f) } else { - /* - reportURL = response?.body?.string()?.let { stringBody -> - adapter.fromJson(stringBody)?.get("report_url")?.toString() - } - */ + null } } } - withContext(coroutineDispatchers.main) { - mBugReportCall = null + if (!mIsCancelled && (withCrashLogs || withDevicesLogs)) { + val gzippedLogcat = saveLogCat(false) - // delete when the bug report has been successfully sent - for (file in mBugReportFiles) { - file.safeDelete() + if (null != gzippedLogcat) { + if (gzippedFiles.size == 0) { + gzippedFiles.add(gzippedLogcat) + } else { + gzippedFiles.add(0, gzippedLogcat) + } + } + } + + /* + activeSessionHolder.getSafeActiveSession() + ?.takeIf { !mIsCancelled && withKeyRequestHistory } + ?.cryptoService() + ?.getGossipingEvents() + ?.let { GossipingEventsSerializer().serialize(it) } + ?.toByteArray() + ?.let { rawByteArray -> + File(context.cacheDir.absolutePath, KEY_REQUESTS_FILENAME) + .also { + it.outputStream() + .use { os -> os.write(rawByteArray) } + } + } + ?.let { compressFile(it) } + ?.let { gzippedFiles.add(it) } + */ + + var deviceId = "undefined" + var userId = "undefined" + var olmVersion = "undefined" + + /* + activeSessionHolder.getSafeActiveSession()?.let { session -> + userId = session.myUserId + deviceId = session.sessionParams.deviceId ?: "undefined" + olmVersion = session.cryptoService().getCryptoVersion(context, true) + } + */ + + if (!mIsCancelled) { + val text = when (reportType) { + ReportType.BUG_REPORT -> bugDescription + ReportType.SUGGESTION -> "[Suggestion] $bugDescription" + ReportType.SPACE_BETA_FEEDBACK -> "[spaces-feedback] $bugDescription" + ReportType.THREADS_BETA_FEEDBACK -> "[threads-feedback] $bugDescription" + ReportType.AUTO_UISI_SENDER, + ReportType.AUTO_UISI -> bugDescription } - if (null != listener) { - try { - if (mIsCancelled) { - listener.onUploadCancelled() - } else if (null == serverError) { - listener.onUploadSucceed(reportURL) - } else { - listener.onUploadFailed(serverError) + // build the multi part request + val builder = BugReporterMultipartBody.Builder() + .addFormDataPart("text", text) + .addFormDataPart("app", rageShakeAppNameForReport(reportType)) + // .addFormDataPart("user_agent", matrix.getUserAgent()) + .addFormDataPart("user_id", userId) + .addFormDataPart("can_contact", canContact.toString()) + .addFormDataPart("device_id", deviceId) + // .addFormDataPart("version", versionProvider.getVersion(longFormat = true)) + // .addFormDataPart("branch_name", buildMeta.gitBranchName) + // .addFormDataPart("matrix_sdk_version", Matrix.getSdkVersion()) + .addFormDataPart("olm_version", olmVersion) + .addFormDataPart("device", Build.MODEL.trim()) + // .addFormDataPart("verbose_log", vectorPreferences.labAllowedExtendedLogging().toOnOff()) + .addFormDataPart("multi_window", inMultiWindowMode.toOnOff()) + // .addFormDataPart( + // "os", Build.VERSION.RELEASE + " (API " + sdkIntProvider.get() + ") " + + // Build.VERSION.INCREMENTAL + "-" + Build.VERSION.CODENAME + // ) + .addFormDataPart("locale", Locale.getDefault().toString()) + // .addFormDataPart("app_language", vectorLocale.applicationLocale.toString()) + // .addFormDataPart("default_app_language", systemLocaleProvider.getSystemLocale().toString()) + // .addFormDataPart("theme", ThemeUtils.getApplicationTheme(context)) + .addFormDataPart("server_version", serverVersion) + .apply { + customFields?.forEach { (name, value) -> + addFormDataPart(name, value) } - } catch (e: Exception) { - Timber.e(e, "## onPostExecute() : failed") } + + // add the gzipped files + for (file in gzippedFiles) { + builder.addFormDataPart("compressed-log", file.name, file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())) + } + + mBugReportFiles.addAll(gzippedFiles) + + if (withScreenshot) { + screenshotHolder.getFileUri() + ?.toUri() + ?.toFile() + ?.let { screenshotFile -> + try { + builder.addFormDataPart( + "file", + screenshotFile.name, screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) + ) + } catch (e: Exception) { + Timber.e(e, "## sendBugReport() : fail to write screenshot") + } + } + } + + // add some github labels + // builder.addFormDataPart("label", buildMeta.versionName) + // builder.addFormDataPart("label", buildMeta.flavorDescription) + // builder.addFormDataPart("label", buildMeta.gitBranchName) + + // Possible values for BuildConfig.BUILD_TYPE: "debug", "nightly", "release". + // builder.addFormDataPart("label", BuildConfig.BUILD_TYPE) + + when (reportType) { + ReportType.BUG_REPORT -> { + /* nop */ + } + ReportType.SUGGESTION -> builder.addFormDataPart("label", "[Suggestion]") + ReportType.SPACE_BETA_FEEDBACK -> builder.addFormDataPart("label", "spaces-feedback") + ReportType.THREADS_BETA_FEEDBACK -> builder.addFormDataPart("label", "threads-feedback") + ReportType.AUTO_UISI -> { + builder.addFormDataPart("label", "Z-UISI") + builder.addFormDataPart("label", "android") + builder.addFormDataPart("label", "uisi-recipient") + } + ReportType.AUTO_UISI_SENDER -> { + builder.addFormDataPart("label", "Z-UISI") + builder.addFormDataPart("label", "android") + builder.addFormDataPart("label", "uisi-sender") + } + } + + if (crashCallStack.isNotEmpty() && withCrashLogs) { + builder.addFormDataPart("label", "crash") + } + + val requestBody = builder.build() + + // add a progress listener + requestBody.setWriteListener { totalWritten, contentLength -> + val percentage = if (-1L != contentLength) { + if (totalWritten > contentLength) { + 100 + } else { + (totalWritten * 100 / contentLength).toInt() + } + } else { + 0 + } + + if (mIsCancelled && null != mBugReportCall) { + mBugReportCall!!.cancel() + } + + Timber.v("## onWrite() : $percentage%") + try { + listener?.onProgress(percentage) + } catch (e: Exception) { + Timber.e(e, "## onProgress() : failed") + } + } + + // build the request + val request = Request.Builder() + .url(context.getString(R.string.bug_report_url)) + .post(requestBody) + .build() + + var responseCode = HttpURLConnection.HTTP_INTERNAL_ERROR + var response: Response? = null + var errorMessage: String? = null + + // trigger the request + try { + mBugReportCall = okHttpClient.newCall(request) + response = mBugReportCall!!.execute() + responseCode = response.code + } catch (e: Exception) { + Timber.e(e, "response") + errorMessage = e.localizedMessage + } + + // if the upload failed, try to retrieve the reason + if (responseCode != HttpURLConnection.HTTP_OK) { + if (null != errorMessage) { + serverError = "Failed with error $errorMessage" + } else if (response?.body == null) { + serverError = "Failed with error $responseCode" + } else { + try { + val inputStream = response.body!!.byteStream() + + serverError = inputStream.use { + buildString { + var ch = it.read() + while (ch != -1) { + append(ch.toChar()) + ch = it.read() + } + } + } + + // check if the error message + serverError?.let { + try { + val responseJSON = JSONObject(it) + serverError = responseJSON.getString("error") + } catch (e: JSONException) { + Timber.e(e, "doInBackground ; Json conversion failed") + } + } + + // should never happen + if (null == serverError) { + serverError = "Failed with error $responseCode" + } + } catch (e: Exception) { + Timber.e(e, "## sendBugReport() : failed to parse error") + } + } + } else { + /* + reportURL = response?.body?.string()?.let { stringBody -> + adapter.fromJson(stringBody)?.get("report_url")?.toString() + } + */ + } + } + } + + withContext(coroutineDispatchers.main) { + mBugReportCall = null + + // delete when the bug report has been successfully sent + for (file in mBugReportFiles) { + file.safeDelete() + } + + if (null != listener) { + try { + if (mIsCancelled) { + listener.onUploadCancelled() + } else if (null == serverError) { + listener.onUploadSucceed(reportURL) + } else { + listener.onUploadFailed(serverError) + } + } catch (e: Exception) { + Timber.e(e, "## onPostExecute() : failed") } } } diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt index dd5d3e76c7..ac8940a1ac 100644 --- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt +++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt @@ -20,13 +20,10 @@ import io.element.android.features.rageshake.api.reporter.BugReporter import io.element.android.features.rageshake.api.reporter.BugReporterListener import io.element.android.features.rageshake.api.reporter.ReportType import io.element.android.libraries.matrix.test.A_FAILURE_REASON -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.delay -import kotlinx.coroutines.launch class FakeBugReporter(val mode: FakeBugReporterMode = FakeBugReporterMode.Success) : BugReporter { - override fun sendBugReport( - coroutineScope: CoroutineScope, + override suspend fun sendBugReport( reportType: ReportType, withDevicesLogs: Boolean, withCrashLogs: Boolean, @@ -38,27 +35,25 @@ class FakeBugReporter(val mode: FakeBugReporterMode = FakeBugReporterMode.Succes customFields: Map?, listener: BugReporterListener?, ) { - coroutineScope.launch { - delay(100) - listener?.onProgress(0) - delay(100) - listener?.onProgress(50) - delay(100) - when (mode) { - FakeBugReporterMode.Success -> Unit - FakeBugReporterMode.Failure -> { - listener?.onUploadFailed(A_FAILURE_REASON) - return@launch - } - FakeBugReporterMode.Cancel -> { - listener?.onUploadCancelled() - return@launch - } + delay(100) + listener?.onProgress(0) + delay(100) + listener?.onProgress(50) + delay(100) + when (mode) { + FakeBugReporterMode.Success -> Unit + FakeBugReporterMode.Failure -> { + listener?.onUploadFailed(A_FAILURE_REASON) + return + } + FakeBugReporterMode.Cancel -> { + listener?.onUploadCancelled() + return } - listener?.onProgress(100) - delay(100) - listener?.onUploadSucceed(null) } + listener?.onProgress(100) + delay(100) + listener?.onUploadSucceed(null) } } From 8302af2dc6a19f27791bf7ff6adc97c5e8e90fec Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 15:39:38 +0100 Subject: [PATCH 6/9] Fix close error dialog has no effect. --- .../features/rageshake/impl/bugreport/BugReportEvents.kt | 2 ++ .../features/rageshake/impl/bugreport/BugReportPresenter.kt | 4 ++++ .../features/rageshake/impl/bugreport/BugReportView.kt | 1 + .../rageshake/impl/bugreport/BugReportPresenterTest.kt | 5 +++++ 4 files changed, 12 insertions(+) diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportEvents.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportEvents.kt index 9ab6c46a9a..372805bca5 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportEvents.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportEvents.kt @@ -19,6 +19,8 @@ package io.element.android.features.rageshake.impl.bugreport sealed interface BugReportEvents { object SendBugReport : BugReportEvents object ResetAll : BugReportEvents + object ClearError : BugReportEvents + data class SetDescription(val description: String) : BugReportEvents data class SetSendLog(val sendLog: Boolean) : BugReportEvents data class SetSendCrashLog(val sendCrashlog: Boolean) : BugReportEvents diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt index 5acb2bd4b1..1a472d0820 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt @@ -109,6 +109,10 @@ class BugReportPresenter @Inject constructor( is BugReportEvents.SetSendScreenshot -> updateFormState(formState) { copy(sendScreenshot = event.sendScreenshot) } + BugReportEvents.ClearError -> { + sendingProgress.value = 0f + sendingAction.value = Async.Uninitialized + } } } diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt index 46a4f7b5c3..794f0f3cea 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt @@ -204,6 +204,7 @@ fun BugReportView( } is Async.Failure -> ErrorDialog( content = state.sending.error.toString(), + onDismiss = { state.eventSink(BugReportEvents.ClearError) } ) else -> Unit } diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt index 93ed963543..1390dd1c8c 100644 --- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt +++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt @@ -222,6 +222,11 @@ class BugReportPresenterTest { // Failure assertThat(awaitItem().sendingProgress).isEqualTo(0f) assertThat((awaitItem().sending as Async.Failure).error.message).isEqualTo(A_FAILURE_REASON) + // Reset failure + initialState.eventSink.invoke(BugReportEvents.ClearError) + val lastItem = awaitItem() + assertThat(lastItem.sendingProgress).isEqualTo(0f) + assertThat(lastItem.sending).isInstanceOf(Async.Uninitialized::class.java) } } From ac8d071d4375ebc5750f20f0346ef45f22a9936e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 16:31:44 +0100 Subject: [PATCH 7/9] Cleanup and use indeterminate progress indicator --- .../features/rageshake/impl/bugreport/BugReportView.kt | 2 +- .../features/rageshake/impl/reporter/DefaultBugReporter.kt | 4 ++-- ...roup_BugReportViewDarkPreview_0_null_2,NEXUS_5,1.0,en].png | 4 ++-- ...oup_BugReportViewLightPreview_0_null_2,NEXUS_5,1.0,en].png | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt index 794f0f3cea..04ec38923b 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt @@ -197,8 +197,8 @@ fun BugReportView( } when (state.sending) { is Async.Loading -> { + // Indeterminate indicator, to avoid the freeze effect if the connection takes time to initialize. CircularProgressIndicator( - progress = state.sendingProgress, modifier = Modifier.align(Alignment.Center) ) } diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt index d9e56a91bc..874da06acb 100755 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt @@ -484,9 +484,9 @@ class DefaultBugReporter @Inject constructor( return compressFile(logCatErrFile) } catch (error: OutOfMemoryError) { - Timber.e(error, "## saveLogCat() : fail to write logcat$error") + Timber.e(error, "## saveLogCat() : fail to write logcat OOM") } catch (e: Exception) { - Timber.e(e, "## saveLogCat() : fail to write logcat$e") + Timber.e(e, "## saveLogCat() : fail to write logcat") } return null diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewDarkPreview_0_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewDarkPreview_0_null_2,NEXUS_5,1.0,en].png index d3203d0fd9..14bcacaede 100644 --- a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewDarkPreview_0_null_2,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewDarkPreview_0_null_2,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:5c34404a33be8eb234025442da98fc85e29236769c200bd0e40cc2cf8f9db3e4 -size 52604 +oid sha256:9b432f300926890ddc7dcc59051b3b31a2a1185ba1d527d776378547433905d9 +size 52789 diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewLightPreview_0_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewLightPreview_0_null_2,NEXUS_5,1.0,en].png index 2b0f06c2da..f2b8ac0c03 100644 --- a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewLightPreview_0_null_2,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.rageshake.impl.bugreport_null_DefaultGroup_BugReportViewLightPreview_0_null_2,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:e628e65234a8632350cdc9ab014b36f3c2bd9e35977bc6d919314a32c2f8a1cf -size 50965 +oid sha256:08cb1e38ee31bf4931d3835d471e61e5cc51d8b6ca5fca3d3044ba85686777ea +size 51075 From 52b62a2018017bd5b4c9f7e4b29468664d688ef0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 16 Mar 2023 16:34:36 +0100 Subject: [PATCH 8/9] Add preview for CircularProgressIndicator --- .../components/CircularProgressIndicator.kt | 27 +++++++++++++++++++ ...atorDarkPreview_0_null,NEXUS_5,1.0,en].png | 3 +++ ...torLightPreview_0_null,NEXUS_5,1.0,en].png | 3 +++ 3 files changed, 33 insertions(+) create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorDarkPreview_0_null,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorLightPreview_0_null,NEXUS_5,1.0,en].png diff --git a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/CircularProgressIndicator.kt b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/CircularProgressIndicator.kt index 00a0f8c0de..0050eb97b1 100644 --- a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/CircularProgressIndicator.kt +++ b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/CircularProgressIndicator.kt @@ -16,11 +16,17 @@ package io.element.android.libraries.designsystem.theme.components +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column import androidx.compose.material3.ProgressIndicatorDefaults import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color +import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.Dp +import androidx.compose.ui.unit.dp +import io.element.android.libraries.designsystem.preview.ElementPreviewDark +import io.element.android.libraries.designsystem.preview.ElementPreviewLight @Composable fun CircularProgressIndicator( @@ -49,3 +55,24 @@ fun CircularProgressIndicator( strokeWidth = strokeWidth, ) } + +@Preview +@Composable +internal fun CircularProgressIndicatorLightPreview() = ElementPreviewLight { ContentToPreview() } + +@Preview +@Composable +internal fun CircularProgressIndicatorDarkPreview() = ElementPreviewDark { ContentToPreview() } + +@Composable +private fun ContentToPreview() { + Column(verticalArrangement = Arrangement.spacedBy(4.dp)) { + // Indeterminate progress + CircularProgressIndicator( + ) + // Fixed progress + CircularProgressIndicator( + progress = 0.75F + ) + } +} diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorDarkPreview_0_null,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorDarkPreview_0_null,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..04fd53bd9b --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorDarkPreview_0_null,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:a100c468dce7b2f7d568dcf0890f680c8f46c063fe0f03f20aed90ed0ad12565 +size 6630 diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorLightPreview_0_null,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorLightPreview_0_null,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..bfff58a5c3 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.libraries.designsystem.theme.components_null_DefaultGroup_CircularProgressIndicatorLightPreview_0_null,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d7e7e2d22e590304ff9921505bce7eca67846299838a6345dc1d27047f2db33c +size 6518 From c0cb056328d446de31b06c38068f20049b9c04ac Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 17 Mar 2023 10:02:51 +0100 Subject: [PATCH 9/9] Inject Lazy and then use callFactory instead of setting client manually. It'll allow to initialise retrofit/okhttp lazily. --- .../io/element/android/libraries/network/RetrofitFactory.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt b/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt index cba09525f9..86531d3680 100644 --- a/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt +++ b/libraries/network/src/main/kotlin/io/element/android/libraries/network/RetrofitFactory.kt @@ -17,6 +17,7 @@ package io.element.android.libraries.network import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import dagger.Lazy import io.element.android.libraries.core.uri.ensureTrailingSlash import kotlinx.serialization.json.Json import okhttp3.MediaType.Companion.toMediaType @@ -25,14 +26,14 @@ import retrofit2.Retrofit import javax.inject.Inject class RetrofitFactory @Inject constructor( - private val okHttpClient: OkHttpClient, + private val okHttpClient: Lazy, ) { fun create(baseUrl: String): Retrofit { val contentType = "application/json".toMediaType() return Retrofit.Builder() .baseUrl(baseUrl.ensureTrailingSlash()) .addConverterFactory(Json.asConverterFactory(contentType)) - .client(okHttpClient) + .callFactory { request -> okHttpClient.get().newCall(request) } .build() } }