ronantakizawa/codereview-bench huggingface dataset:
Repository: thunderbird/thunderbird-android
File: legacy/core/src/main/java/com/fsck/k9/job/SyncDebugWorker.kt (Kotlin)

Code before:
package com.fsck.k9.job

import android.content.Context
import androidx.work.Worker
import androidx.work.WorkerParameters
import com.fsck.k9.K9
import java.io.IOException
import net.thunderbird.core.logging.composite.CompositeLogSink
import net.thunderbird.core.logging.file.FileLogSink

class SyncDebugWorker(
    context: Context,
    val fileLogSink: FileLogSink,
    val syncDebugCompositeSink: CompositeLogSink,
    parameters: WorkerParameters,
) : Worker(context, parameters) {
    override fun doWork(): Result {
        try {
            fileLogSink.export(inputData.getString("exportUriString").toString())
        } catch (_: IOException) {
            return Result.failure()
        }
        syncDebugCompositeSink.manager.remove(fileLogSink)
        K9.isSyncLoggingEnabled = false
        K9.saveSettingsAsync()
        return Result.success()
    }
}


Review comment (suggestion):
I would suggest at least logging an error to logcat when an `IOException` happens here.

Code after:
package com.fsck.k9.job

import android.content.Context
import androidx.work.CoroutineWorker
import androidx.work.WorkerParameters
import com.fsck.k9.K9
import java.io.IOException
import net.thunderbird.core.logging.Logger
import net.thunderbird.core.logging.composite.CompositeLogSink
import net.thunderbird.core.logging.file.FileLogSink

class SyncDebugWorker(
    context: Context,
    val baseLogger: Logger,
    val fileLogSink: FileLogSink,
    val syncDebugCompositeSink: CompositeLogSink,
    parameters: WorkerParameters,
) : CoroutineWorker(context, parameters) {

    override suspend fun doWork(): Result {
        try {
            fileLogSink.export(inputData.getString("exportUriString").toString())
        } catch (e: IOException) {
            baseLogger.error(message = { "Failed to export log" }, throwable = e)
            return Result.failure()
        }
        syncDebugCompositeSink.manager.remove(fileLogSink)
        K9.isSyncLoggingEnabled = false
        K9.saveSettingsAsync()
        return Result.success()
    }
}
Repository: thunderbird/thunderbird-android
File: app-k9mail/src/main/kotlin/app/k9mail/provider/K9AppNotificationIconProvider.kt (Kotlin)

Code before:
package app.k9mail.provider

import android.content.Context
import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
import app.k9mail.core.ui.legacy.theme2.k9mail.R

class K9AppNotificationIconProvider(
    private val context: Context,

) : NotificationIconResourceProvider {
    override val pushNotificationIcon: Int

        get() = R.drawable.ic_logo_k9_white
}


Review comment (suggestion):
The context property is not being used, thus you can remove it:
```suggestion
class K9AppNotificationIconProvider : NotificationIconResourceProvider {
```

Code after:
package app.k9mail.provider

import app.k9mail.core.android.common.provider.NotificationIconResourceProvider
import app.k9mail.core.ui.legacy.theme2.k9mail.R

class K9AppNotificationIconProvider : NotificationIconResourceProvider {
    override val pushNotificationIcon: Int

        get() = R.drawable.ic_logo_k9_white
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/main/java/com/fsck/k9/CoreResourceProvider.kt (Kotlin)

Code before:
import com.fsck.k9.notification.PushNotificationState

interface CoreResourceProvider {
    fun defaultIdentityDescription(): String

    fun contactDisplayNamePrefix(): String
    fun contactUnknownSender(): String
    fun contactUnknownRecipient(): String

    fun messageHeaderFrom(): String
    fun messageHeaderTo(): String
    fun messageHeaderCc(): String
    fun messageHeaderDate(): String
    fun messageHeaderSubject(): String
    fun messageHeaderSeparator(): String

    fun noSubject(): String
    fun userAgent(): String

    fun replyHeader(sender: String): String
    fun replyHeader(sender: String, sentDate: String): String

    fun searchUnifiedInboxTitle(): String
    fun searchUnifiedInboxDetail(): String

    // val iconPushNotification: Int
    fun pushNotificationText(notificationState: PushNotificationState): String
    fun pushNotificationInfoText(): String
    fun pushNotificationGrantAlarmPermissionText(): String
}


Review comment (suggestion):
If we don't need `iconPushNotification` anymore, this should be deleted instead of commented

Code after:
import com.fsck.k9.notification.PushNotificationState

interface CoreResourceProvider {
    fun defaultIdentityDescription(): String

    fun contactDisplayNamePrefix(): String
    fun contactUnknownSender(): String
    fun contactUnknownRecipient(): String

    fun messageHeaderFrom(): String
    fun messageHeaderTo(): String
    fun messageHeaderCc(): String
    fun messageHeaderDate(): String
    fun messageHeaderSubject(): String
    fun messageHeaderSeparator(): String

    fun noSubject(): String
    fun userAgent(): String

    fun replyHeader(sender: String): String
    fun replyHeader(sender: String, sentDate: String): String

    fun searchUnifiedFoldersTitle(): String
    fun searchUnifiedFoldersDetail(): String
    fun pushNotificationText(notificationState: PushNotificationState): String
    fun pushNotificationInfoText(): String
    fun pushNotificationGrantAlarmPermissionText(): String
}
Repository: thunderbird/thunderbird-android
File: core/logging/api/src/commonMain/kotlin/net/thunderbird/core/logging/LogSink.kt (Kotlin)

Code before:
     * The minimum log level this sink will process.
     * Log events with a lower priority than this level will be ignored.
     */
    val level: LogLevel

    /**
     * Checks whether the sink is enabled for the given log level.
     *
     * @param level The log level to check.
     * @return `true` if this sink will process log events at this level or higher.
     */
    fun canLog(level: LogLevel): Boolean {
        return this.level <= level
    }

    /**
     * Logs a [LogEvent].
     *
     * @param event The [LogEvent] to log.
     */
    fun log(
        event: LogEvent,
    )

    enum class Qualifier {
        DebuggingLogsEnabledByUser,
        SyncDebug,
    }
}


Review comment (bug):
What about renaming it to `DebugLogsEnabledByUser`?

Code after:
package net.thunderbird.core.logging

/**
 * A sink that receives and processes log events.
 *
 * A `LogSink` determines whether to handle a log event based on its log level.
 * Log events with a level lower than the sink's configured level will be ignored.
 */
interface LogSink {

    /**
     * The minimum log level this sink will process.
     * Log events with a lower priority than this level will be ignored.
     */
    val level: LogLevel

    /**
     * Checks whether the sink is enabled for the given log level.
     *
     * @param level The log level to check.
     * @return `true` if this sink will process log events at this level or higher.
     */
    fun canLog(level: LogLevel): Boolean {
        return this.level <= level
    }

    /**
     * Logs a [LogEvent].
     *
     * @param event The [LogEvent] to log.
     */
    fun log(
        event: LogEvent,
    )
}
Repository: thunderbird/thunderbird-android
File: core/logging/impl-composite/src/commonMain/kotlin/net/thunderbird/core/logging/composite/DefaultCompositeLogSink.kt (Kotlin)

Code before:
package net.thunderbird.core.logging.composite

import net.thunderbird.core.logging.LogEvent
import net.thunderbird.core.logging.LogLevel
import net.thunderbird.core.logging.LogSink

internal class DefaultCompositeLogSink(
    override val level: LogLevel,
    override val manager: CompositeLogSinkManager = DefaultLogSinkManager(),
    sinks: List<LogSink> = emptyList(),
) : CompositeLogSink {

    init {
        manager.addAll(sinks)
    }

    override fun canLog(level: LogLevel): Boolean {
        return manager.getAll().any { it.canLog(level) }
    }

    override fun log(event: LogEvent) {
        if (canLog(event.level)) {
            manager.getAll().forEach { sink ->
                if (sink.canLog(event.level)) {
                    sink.log(event)
                }
            }
        }
    }
}


Review comment (suggestion):
These changes will ignore the composite own log level and so the globally configured max level.

The tests need to be updated to cover the new cases.

Code after:
package net.thunderbird.core.logging.composite

import net.thunderbird.core.logging.LogEvent
import net.thunderbird.core.logging.LogLevel
import net.thunderbird.core.logging.LogLevelProvider
import net.thunderbird.core.logging.LogSink

internal class DefaultCompositeLogSink(
    private val logLevelProvider: LogLevelProvider,
    override val manager: CompositeLogSinkManager = DefaultLogSinkManager(),
    sinks: List<LogSink> = emptyList(),
) : CompositeLogSink {
    override val level: LogLevel get() = logLevelProvider.current()

    init {
        manager.addAll(sinks)
    }

    override fun log(event: LogEvent) {
        if (canLog(event.level)) {
            manager.getAll().forEach { sink ->
                if (sink.canLog(event.level)) {
                    sink.log(event)
                }
            }
        }
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/main/java/com/fsck/k9/autocrypt/AutocryptHeader.kt (Kotlin)

Code before:

    override fun hashCode(): Int {
        var result = keyData.contentHashCode()
        result = 31 * result + addr.hashCode()
        result = 31 * result + parameters.hashCode()
        result = 31 * result + if (isPreferEncryptMutual) 1 else 0
        return result
    }

    companion object {
        const val AUTOCRYPT_HEADER = "Autocrypt"

        const val AUTOCRYPT_PARAM_ADDR = "addr"
        const val AUTOCRYPT_PARAM_KEY_DATA = "keydata"

        const val AUTOCRYPT_PARAM_TYPE = "type"
        const val AUTOCRYPT_TYPE_1 = "1"

        const val AUTOCRYPT_PARAM_PREFER_ENCRYPT = "prefer-encrypt"
        const val AUTOCRYPT_PREFER_ENCRYPT_MUTUAL = "mutual"

        private const val HEADER_LINE_LENGTH = 76

        @Suppress("SpreadOperator")
        fun createFoldedBase64KeyData(keyData: ByteArray): String {
            val base64KeyData = ByteString.of(*keyData).base64()
            val result = StringBuilder()

            for (i in base64KeyData.indices step HEADER_LINE_LENGTH) {
                result.append("\r\n ")
                if (i + HEADER_LINE_LENGTH <= base64KeyData.length) {
                    result.append(base64KeyData, i, i + HEADER_LINE_LENGTH)
                } else {
                    result.append(base64KeyData, i, base64KeyData.length)
                }
            }

            return result.toString()
        }
    }
}


Review comment (suggestion):
Please avoid using spread operator when possible. You could use the following approach instead:

```suggestion
        fun createFoldedBase64KeyData(keyData: ByteArray): String {
            val base64KeyData = ByteString.of(data = keyData).base64()
```

Code after:

    override fun hashCode(): Int {
        var result = keyData.contentHashCode()
        result = 31 * result + addr.hashCode()
        result = 31 * result + parameters.hashCode()
        result = 31 * result + if (isPreferEncryptMutual) 1 else 0
        return result
    }

    companion object {
        const val AUTOCRYPT_HEADER = "Autocrypt"

        const val AUTOCRYPT_PARAM_ADDR = "addr"
        const val AUTOCRYPT_PARAM_KEY_DATA = "keydata"

        const val AUTOCRYPT_PARAM_TYPE = "type"
        const val AUTOCRYPT_TYPE_1 = "1"

        const val AUTOCRYPT_PARAM_PREFER_ENCRYPT = "prefer-encrypt"
        const val AUTOCRYPT_PREFER_ENCRYPT_MUTUAL = "mutual"

        private const val HEADER_LINE_LENGTH = 76

        fun createFoldedBase64KeyData(keyData: ByteArray): String {
            val base64KeyData = ByteString.of(data = keyData).base64()
            val result = StringBuilder()

            for (i in base64KeyData.indices step HEADER_LINE_LENGTH) {
                result.append("\r\n ")
                if (i + HEADER_LINE_LENGTH <= base64KeyData.length) {
                    result.append(base64KeyData, i, i + HEADER_LINE_LENGTH)
                } else {
                    result.append(base64KeyData, i, base64KeyData.length)
                }
            }

            return result.toString()
        }
    }
}
Repository: thunderbird/thunderbird-android
File: core/logging/config/build.gradle.kts (Kotlin)

Code before:
plugins {
    id(ThunderbirdPlugins.Library.kmp)
}

android {
    namespace = "net.thunderbird.core.logging.config"
}

kotlin {
    sourceSets {
        androidMain.dependencies {
            implementation(libs.timber)
            implementation(projects.core.logging.implComposite)
            implementation(projects.core.logging.implFile)
        }

        commonMain.dependencies {
            api(projects.core.logging.api)
        }
    }
}


Review comment (suggestion):
These need to be moved to `commonMain` and the android configuration is not needed.

Code after:
plugins {
    id(ThunderbirdPlugins.Library.kmp)
}

android {
    namespace = "net.thunderbird.core.logging.config"
}

kotlin {
    sourceSets {
        commonMain.dependencies {
            api(projects.core.logging.api)
            implementation(projects.core.logging.implComposite)
            implementation(projects.core.logging.implFile)
        }
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/main/java/com/fsck/k9/autocrypt/AutocryptGossipHeaderParser.kt (Kotlin)

Code before:

            hasCriticalParameters(parameters) -> {
                null
            }

            else -> {
                val byteString = base64KeyData.decodeBase64()!! // safe after null-check
                AutocryptGossipHeader(addr, byteString.toByteArray())
            }
        }
    }

    private fun hasCriticalParameters(parameters: Map<String, String>): Boolean {
        return parameters.keys.any { !it.startsWith("_") }
    }

    private fun parseAllAutocryptGossipHeaders(headers: Array<String>): List<AutocryptGossipHeader> {
        return headers.mapNotNull { header ->
            parseAutocryptGossipHeader(header) ?: run {
                Log.e("Encountered malformed autocrypt-gossip header - skipping!")
                null
            }
        }
    }

    companion object {
        val instance: AutocryptGossipHeaderParser by lazy { AutocryptGossipHeaderParser() }
    }
}


Review comment (suggestion):
suggestion: You could declare the class as an `object` instead. Object declarations in Kotlin are singleton's by default. See [Object declarations](https://kotlinlang.org/docs/object-declarations.html#object-declarations-overview) for more reference.

You can access it on Java code via `AutocryptGossipHeaderParser.INSTANCE`.

Code after:
                null
            }

            hasCriticalParameters(parameters) -> {
                null
            }

            else -> {
                AutocryptGossipHeader(addr, decodedKey.toByteArray())
            }
        }
    }

    private fun hasCriticalParameters(parameters: Map<String, String>): Boolean {
        return parameters.keys.any { !it.startsWith("_") }
    }

    private fun parseAllAutocryptGossipHeaders(headers: Array<String>): List<AutocryptGossipHeader> {
        return headers.mapNotNull { header ->
            parseAutocryptGossipHeader(header) ?: run {
                Log.e("Encountered malformed autocrypt-gossip header - skipping!")
                null
            }
        }
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/main/java/com/fsck/k9/autocrypt/AutocryptGossipHeaderParser.kt (Kotlin)

Code before:
            type != null && type != AutocryptHeader.AUTOCRYPT_TYPE_1 -> {
                Log.e("autocrypt: unsupported type parameter %s", type)
                null
            }

            base64KeyData == null -> {
                Log.e("autocrypt: missing key parameter")
                null
            }

            base64KeyData.decodeBase64() == null -> {
                Log.e("autocrypt: error parsing base64 data")
                null
            }

            addr == null -> {
                Log.e("autocrypt: no to header!")
                null
            }

            hasCriticalParameters(parameters) -> {
                null
            }

            else -> {
                val byteString = base64KeyData.decodeBase64()!! // safe after null-check
                AutocryptGossipHeader(addr, byteString.toByteArray())
            }
        }
    }

    private fun hasCriticalParameters(parameters: Map<String, String>): Boolean {
        return parameters.keys.any { !it.startsWith("_") }
    }

    private fun parseAllAutocryptGossipHeaders(headers: Array<String>): List<AutocryptGossipHeader> {
        return headers.mapNotNull { header ->
            parseAutocryptGossipHeader(header) ?: run {
                Log.e("Encountered malformed autocrypt-gossip header - skipping!")
                null
            }
        }
    }

    companion object {
        val instance: AutocryptGossipHeaderParser by lazy { AutocryptGossipHeaderParser() }
    }
}


Review comment (suggestion):
Please, avoid using the bang operator `!!`. Also, you are decoding the `base64KeyData` twice, which you could have done before the `when` and just use it later.

Code after:

        return when {
            type != null && type != AutocryptHeader.AUTOCRYPT_TYPE_1 -> {
                Log.e("autocrypt: unsupported type parameter %s", type)
                null
            }

            base64KeyData == null -> {
                Log.e("autocrypt: missing key parameter")
                null
            }

            decodedKey == null -> {
                Log.e("autocrypt: error parsing base64 data")
                null
            }

            addr == null -> {
                Log.e("autocrypt: no to header!")
                null
            }

            hasCriticalParameters(parameters) -> {
                null
            }

            else -> {
                AutocryptGossipHeader(addr, decodedKey.toByteArray())
            }
        }
    }

    private fun hasCriticalParameters(parameters: Map<String, String>): Boolean {
        return parameters.keys.any { !it.startsWith("_") }
    }

    private fun parseAllAutocryptGossipHeaders(headers: Array<String>): List<AutocryptGossipHeader> {
        return headers.mapNotNull { header ->
            parseAutocryptGossipHeader(header) ?: run {
                Log.e("Encountered malformed autocrypt-gossip header - skipping!")
                null
            }
        }
    }
}
Repository: thunderbird/thunderbird-android
File: core/architecture/api/build.gradle.kts (Kotlin)

Code before:
plugins {
    id(ThunderbirdPlugins.Library.kmp)
}

kotlin {
    androidLibrary {
        namespace = "net.thunderbird.core.architecture"
        withHostTestBuilder {}
    }
}


Review comment (suggestion):
Any reason why you're not using `withHostTest` instead? Otherwise change it please.

```suggestion
        withHostTest {}
```


Code after:
plugins {
    id(ThunderbirdPlugins.Library.kmp)
}

kotlin {
    androidLibrary {
        namespace = "net.thunderbird.core.architecture"
        withHostTest {}
    }
}
Repository: thunderbird/thunderbird-android
File: feature/notification/impl/build.gradle.kts (Kotlin)

Code before:
plugins {
    id(ThunderbirdPlugins.Library.kmpCompose)
    alias(libs.plugins.dev.mokkery)
}

kotlin {
    androidLibrary {
        namespace = "net.thunderbird.feature.notification"
        withHostTestBuilder {}.configure {
            isIncludeAndroidResources = true
        }
    }
    sourceSets {
        commonMain.dependencies {
            implementation(projects.core.common)
            implementation(projects.core.featureflag)
            implementation(projects.core.outcome)
            implementation(projects.core.logging.api)
            implementation(projects.feature.notification.api)
        }
        commonTest.dependencies {
            implementation(projects.core.testing)
            implementation(projects.core.logging.testing)
            implementation(projects.feature.notification.testing)
        }
        androidMain.dependencies {
            // should split feature.launcher into api/impl?
            implementation(projects.feature.launcher)
            implementation(projects.core.ui.theme.api)
        }
        getByName("androidHostTest").dependencies {
            implementation(libs.androidx.test.core)
            implementation(libs.mockito.core)
            implementation(libs.mockito.kotlin)
            implementation(libs.robolectric)
        }

Review comment (suggestion):
Should be:

```suggestion
        withHostTest {
            isIncludeAndroidResources = true
        }
```

Code after:
plugins {
    id(ThunderbirdPlugins.Library.kmpCompose)
    alias(libs.plugins.dev.mokkery)
}

kotlin {
    androidLibrary {
        namespace = "net.thunderbird.feature.notification"
        withHostTest {
            isIncludeAndroidResources = true
        }
    }
    sourceSets {
        commonMain.dependencies {
            implementation(projects.core.common)
            implementation(projects.core.featureflag)
            implementation(projects.core.outcome)
            implementation(projects.core.logging.api)
            implementation(projects.feature.notification.api)
        }
        commonTest.dependencies {
            implementation(projects.core.testing)
            implementation(projects.core.logging.testing)
            implementation(projects.feature.notification.testing)
        }
        androidMain.dependencies {
            // should split feature.launcher into api/impl?
            implementation(projects.feature.launcher)
            implementation(projects.core.ui.theme.api)
        }
        androidHostTest.dependencies {
            implementation(libs.androidx.test.core)
            implementation(libs.mockito.core)
            implementation(libs.mockito.kotlin)
            implementation(libs.robolectric)
        }