ronantakizawa/codereview-bench huggingface dataset:
Repository: thunderbird/thunderbird-android
File: legacy/preferences/src/main/java/app/k9mail/legacy/preferences/DefaultSettingsChangeBroker.kt (Kotlin)

Code before:
package app.k9mail.legacy.preferences

class DefaultSettingsChangeBroker(
    private val subscribers: MutableSet<SettingsChangeSubscriber> = mutableSetOf(),
) : SettingsChangeBroker, SettingsChangePublisher {

    private val lock = Any()

    override fun subscribe(subscriber: SettingsChangeSubscriber) {
        synchronized(lock) {
            subscribers.add(subscriber)
        }
    }

    override fun unsubscribe(subscriber: SettingsChangeSubscriber) {
        synchronized(lock) {
            subscribers.remove(subscriber)
        }
    }

    override fun publish() {
        for (subscriber in subscribers) {
            subscriber.receive()
        }
    }
}


Review comment (bug):
Suggestion: I'd avoid making what should be internal state available via the constructor. I think it's better if tests don't check implementation details (was this subscriber added to the internal subscribers list), but the observable behavior of the class (will publishing notify the subscriber after it has been added, and will publishing after the subscriber has been removed stop notifications to the subscriber; also edge cases like subscribing twice, unsubscribing subscribers that haven't been added before).

Code after:
package app.k9mail.legacy.preferences

class DefaultSettingsChangeBroker(
    private val subscribers: MutableSet<SettingsChangeSubscriber> = mutableSetOf(),
) : SettingsChangeBroker, SettingsChangePublisher {

    private val lock = Any()

    override fun subscribe(subscriber: SettingsChangeSubscriber) {
        synchronized(lock) {
            subscribers.add(subscriber)
        }
    }

    override fun unsubscribe(subscriber: SettingsChangeSubscriber) {
        synchronized(lock) {
            subscribers.remove(subscriber)
        }
    }

    override fun publish() {
        val currentSubscribers = synchronized(lock) { HashSet(subscribers) }

        for (subscriber in currentSubscribers) {
            subscriber.receive()
        }
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/test/java/com/fsck/k9/helper/FileHelperTest.kt (Kotlin)

Code before:
package com.fsck.k9.helper

import assertk.assertThat
import assertk.assertions.isFalse
import assertk.assertions.isNotEqualTo
import assertk.assertions.isTrue
import java.io.File
import kotlin.test.Test
import org.junit.Rule
import org.junit.rules.TemporaryFolder

class FileHelperTest {

    @Rule
    @JvmField
    val tempFolder = TemporaryFolder()

    @Test
    fun `touchFile should create file`() {
        val parent = tempFolder.newFolder("parent")

        FileHelper.touchFile(parent, "FileName")

        assertThat(File(parent, "FileName").exists()).isTrue()
    }

    @Test
    fun `touchFile should change file modification time`() {
        val parent = tempFolder.newFolder("parent")
        val lastModified = 12345L
        val file = File(parent, "FileName")
        file.createNewFile()
        file.setLastModified(lastModified)

        FileHelper.touchFile(parent, "FileName")

        assertThat(file.lastModified()).isNotEqualTo(lastModified)
    }

    @Test
    fun `renameOrMoveByCopying should move file`() {
        val source = tempFolder.newFile("source")
        val destination = tempFolder.newFile("destination")

        FileHelper.renameOrMoveByCopying(source, destination)

        assertThat(source.exists()).isFalse()
        assertThat(destination.exists()).isTrue()
    }
}


Review comment (refactor):
This test would pass if the implementation of `FileHelper.renameOrMoveByCopying()` would simply delete the source file. The test passing doesn't give much confidence that `FileHelper.renameOrMoveByCopying()` actually does what it says it does.
The test could write something to the source file and then read the destination file after the move operation to see if the file contains the expected content. 

The common case for our app is that the destination file doesn't exist. So if we could write only one test, it should be one where the destination file doesn't exist. Ideally, of course, both cases would be tested.

Code after:

    @Test
    fun `renameOrMoveByCopying should move file when destination does not exist`() {
        val source = tempFolder.newFile("source")
        val destination = File(tempFolder.root, "destination")
        val sourceContent = "content"
        source.writeText(sourceContent)

        FileHelper.renameOrMoveByCopying(source, destination)

        assertThat(source.exists()).isFalse()
        assertThat(destination.exists()).isTrue()
        assertThat(destination.readText()).isEqualTo(sourceContent)
    }

    @Test
    fun `renameOrMoveByCopying should move file when destination exists`() {
        val source = tempFolder.newFile("source")
        val destination = tempFolder.newFile("destination")
        val sourceContent = "content"
        source.writeText(sourceContent)

        FileHelper.renameOrMoveByCopying(source, destination)

        assertThat(source.exists()).isFalse()
        assertThat(destination.exists()).isTrue()
        assertThat(destination.readText()).isEqualTo(sourceContent)
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/core/src/test/java/com/fsck/k9/TestApp.kt (Kotlin)

Code before:
            modules = coreModules + storageModule + telemetryModule + testModule,
            allowOverride = true,
        )

        K9.init(this)
        Core.init(this)
    }
}

val testModule = module {
    single { AppConfig(emptyList()) }
    single { mock<CoreResourceProvider>() }
    single { mock<EncryptionExtractor>() }
    single<StoragePersister> { InMemoryStoragePersister() }
    single { mock<BackendManager>() }
    single { mock<NotificationResourceProvider>() }
    single { mock<NotificationActionCreator>() }
    single { mock<NotificationStrategy>() }
    single(named("controllerExtensions")) { emptyList<ControllerExtension>() }
    single { mock<WorkManager>() }
    single<FeatureFlagProvider> {
        InMemoryFeatureFlagProvider(
            featureFlagFactory = {
                listOf(
                    FeatureFlag(key = FeatureFlagKey("feature1"), enabled = true),
                )
            },
        )
    }
}


Review comment (suggestion):
I think an emptyList should be enough.

```suggestion
                emptyList<FeatureFlag>()
```

Code after:
            allowOverride = true,
        )

        K9.init(this)
        Core.init(this)
    }
}

val testModule = module {
    single { AppConfig(emptyList()) }
    single { mock<CoreResourceProvider>() }
    single { mock<EncryptionExtractor>() }
    single<StoragePersister> { InMemoryStoragePersister() }
    single { mock<BackendManager>() }
    single { mock<NotificationResourceProvider>() }
    single { mock<NotificationActionCreator>() }
    single { mock<NotificationStrategy>() }
    single(named("controllerExtensions")) { emptyList<ControllerExtension>() }
    single { mock<WorkManager>() }
    single<FeatureFlagProvider> {
        InMemoryFeatureFlagProvider(
            featureFlagFactory = {
                emptyList<FeatureFlag>()
            },
        )
    }
}
Repository: thunderbird/thunderbird-android
File: core/ui/compose/designsystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/molecule/input/TextInput.kt (Kotlin)

Code before:
@Composable
fun TextInput(
    onTextChange: (String) -> Unit,
    modifier: Modifier = Modifier,
    text: String = "",
    label: String? = null,
    isRequired: Boolean = false,
    errorMessage: String? = null,
    contentPadding: PaddingValues = inputContentPadding(),
    isSingleLine: Boolean = true,
    isEnabled: Boolean = true,
    keyboardOptions: KeyboardOptions = KeyboardOptions.Default,
) {
    InputLayout(
        contentPadding = contentPadding,
        errorMessage = errorMessage,
    ) {
        TextFieldOutlined(
            value = text,
            onValueChange = onTextChange,
            label = label,
            isEnabled = isEnabled,
            isRequired = isRequired,
            hasError = errorMessage != null,
            isSingleLine = isSingleLine,
            modifier = modifier.fillMaxWidth(),
            keyboardOptions = keyboardOptions,
        )
    }
}


Review comment (suggestion):
Instead of changing the modifier, the `TextInput` should have a contentType property that is applied here.

```suggestion
            modifier = textFieldModifier.fillMaxWidth(),
```

The textFieldModifier:

```kotlin
        val textFieldModifier = if (contentType != null) {
            Modifier.semantics { this.contentType = contentType }
        } else {
            Modifier
        }
```

and contentType is nullable: `contentType: ContentType? = null,`

Code after:
import app.k9mail.core.ui.compose.designsystem.atom.textfield.TextFieldOutlined

@Suppress("LongParameterList")
@Composable
fun TextInput(
    onTextChange: (String) -> Unit,
    modifier: Modifier = Modifier,
    text: String = "",
    label: String? = null,
    isRequired: Boolean = false,
    errorMessage: String? = null,
    contentPadding: PaddingValues = inputContentPadding(),
    isSingleLine: Boolean = true,
    isEnabled: Boolean = true,
    keyboardOptions: KeyboardOptions = KeyboardOptions.Default,
    contentType: ContentType? = null,
) {
    InputLayout(
        modifier = modifier,
        contentPadding = contentPadding,
        errorMessage = errorMessage,
    ) {
        val textFieldModifier = if (contentType != null) {
            Modifier.semantics { this.contentType = contentType }
        } else {
            Modifier
        }

        TextFieldOutlined(
            value = text,
            onValueChange = onTextChange,
            label = label,
            isEnabled = isEnabled,
            isRequired = isRequired,
            hasError = errorMessage != null,
            isSingleLine = isSingleLine,
            modifier = textFieldModifier.fillMaxWidth(),
            keyboardOptions = keyboardOptions,
        )
    }
}
Repository: thunderbird/thunderbird-android
File: legacy/ui/legacy/src/main/java/com/fsck/k9/ui/messagelist/MessageListConfig.kt (Kotlin)

Code before:
package com.fsck.k9.ui.messagelist

import app.k9mail.legacy.account.SortType
import app.k9mail.legacy.message.controller.MessageReference
import net.thunderbird.feature.search.LocalSearch

data class MessageListConfig(
    val search: net.thunderbird.feature.search.LocalSearch,
    val showingThreadedList: Boolean,
    val sortType: SortType,
    val sortAscending: Boolean,
    val sortDateAscending: Boolean,
    val activeMessage: MessageReference?,
    val sortOverrides: Map<MessageReference, MessageSortOverride>,
)

data class MessageSortOverride(
    val isRead: Boolean,
    val isStarred: Boolean,
)


Review comment (refactor):
This syntax usage is not familiar to me, is there some reason it wouldn't be `LocalSearch` instead of `net.thunderbird...`? If its a duplicate name issue the `as` keyword could be used

Code after:
package com.fsck.k9.ui.messagelist

import app.k9mail.legacy.account.SortType
import app.k9mail.legacy.message.controller.MessageReference
import net.thunderbird.feature.search.LocalSearch

data class MessageListConfig(
    val search: LocalSearch,
    val showingThreadedList: Boolean,
    val sortType: SortType,
    val sortAscending: Boolean,
    val sortDateAscending: Boolean,
    val activeMessage: MessageReference?,
    val sortOverrides: Map<MessageReference, MessageSortOverride>,
)

data class MessageSortOverride(
    val isRead: Boolean,
    val isStarred: Boolean,
)
Repository: thunderbird/thunderbird-android
File: feature/navigation/drawer/dropdown/src/main/kotlin/net/thunderbird/feature/navigation/drawer/dropdown/domain/usecase/GetDisplayTreeFolder.kt (Kotlin)

Code before:
                    if (segments.size == 1) folder else null
                }

                val fullPath = if (parentPath.isBlank()) segment else "$parentPath/$segment"

                val currentFolder = currentFolders.firstOrNull() ?: createPlaceholderFolder(fullPath)

                val children = buildAccountFolderTree(childPaths, fullPath)

                val totalUnread = children.sumOf { it.totalUnreadCount } + currentFolder.unreadMessageCount
                val totalStarred = children.sumOf { it.totalStarredCount } + currentFolder.starredMessageCount

                DisplayTreeFolder(
                    displayFolder = currentFolder,
                    displayName = segment,
                    totalUnreadCount = totalUnread,
                    totalStarredCount = totalStarred,
                    children = children.toImmutableList(),
                )
            }
    }

    private fun createPlaceholderFolder(name: String): DisplayAccountFolder {
        placeholderCounter += 1
        return DisplayAccountFolder(
            accountId = "accountId",
            folder = Folder(
                id = placeholderCounter,
                name = name,
                type = FolderType.REGULAR,
                isLocalOnly = false,
            ),
            isInTopGroup = true,
            unreadMessageCount = 0,
            starredMessageCount = 0,
        )
    }
}


Review comment (suggestion):
Let's change this to `placeholder` if we can't include the real account id.

Code after:
                    if (segments.size == 1) folder else null
                }

                val fullPath = if (parentPath.isBlank()) segment else "$parentPath/$segment"

                val currentFolder = currentFolders.firstOrNull() ?: createPlaceholderFolder(fullPath)

                val children = buildAccountFolderTree(childPaths, fullPath)

                val totalUnread = children.sumOf { it.totalUnreadCount } + currentFolder.unreadMessageCount
                val totalStarred = children.sumOf { it.totalStarredCount } + currentFolder.starredMessageCount

                DisplayTreeFolder(
                    displayFolder = currentFolder,
                    displayName = segment,
                    totalUnreadCount = totalUnread,
                    totalStarredCount = totalStarred,
                    children = children.toImmutableList(),
                )
            }
    }

    private fun createPlaceholderFolder(name: String): DisplayAccountFolder {
        placeholderCounter += 1
        return DisplayAccountFolder(
            accountId = "placeholder",
            folder = Folder(
                id = placeholderCounter,
                name = name,
                type = FolderType.REGULAR,
                isLocalOnly = false,
            ),
            isInTopGroup = true,
            unreadMessageCount = 0,
            starredMessageCount = 0,
        )
    }
}
Repository: thunderbird/thunderbird-android
File: core/ui/compose/preference/src/main/kotlin/net/thunderbird/core/ui/compose/preference/api/Preference.kt (Kotlin)

Code before:
    ) : PreferenceSetting<Int> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = true
    }

    @Parcelize
    data class SingleChoice(
        override val id: String,
        val title: () -> String,
        val description: () -> String? = { null },
        override val value: Choice,
        val options: ImmutableList<Choice>,
    ) : PreferenceSetting<Choice> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = false

        @Parcelize
        data class Choice(
            val id: String,
            val title: () -> String,
        ) : Parcelable
    }

    @Parcelize
    data class Switch(
        override val id: String,
        val enabled: Boolean,
        override val value: Boolean,
    ) : PreferenceSetting<Boolean> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = false
    }
}

/**
 * A preference that does not hold a value. It is used to display a section, a divider or custom UI.
 */
sealed interface PreferenceDisplay : Preference {

    @Parcelize
    data class Custom(
        override val id: String,
        val customUi: @Composable (Modifier) -> Unit,
    ) : PreferenceDisplay
}


Review comment (suggestion):
These preferences are related to render UI elements, in this case it needs also a title and a description to be displayed to the user.

Code after:
    ) : PreferenceSetting<Int> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = true
    }

    @Parcelize
    data class SingleChoice(
        override val id: String,
        val title: () -> String,
        val description: () -> String? = { null },
        override val value: Choice,
        val options: ImmutableList<Choice>,
    ) : PreferenceSetting<Choice> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = false

        @Parcelize
        data class Choice(
            val id: String,
            val title: () -> String,
        ) : Parcelable
    }

    @Parcelize
    data class Switch(
        override val id: String,
        val title: () -> String,
        val description: () -> String? = { null },
        val enabled: Boolean,
        override val value: Boolean,
    ) : PreferenceSetting<Boolean> {
        @IgnoredOnParcel
        override val requiresEditView: Boolean = false
    }
}

/**
 * A preference that does not hold a value. It is used to display a section, a divider or custom UI.
 */
sealed interface PreferenceDisplay : Preference {

    @Parcelize
    data class Custom(
        override val id: String,
        val customUi: @Composable (Modifier) -> Unit,
    ) : PreferenceDisplay
}
Repository: thunderbird/thunderbird-android
File: core/ui/compose/designsystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/atom/RadioGroup.kt (Kotlin)

Code before:
package app.k9mail.core.ui.compose.designsystem.atom

import androidx.compose.foundation.layout.Row
import androidx.compose.material3.RadioButton
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import app.k9mail.core.ui.compose.designsystem.atom.text.TextLabelLarge
import kotlinx.collections.immutable.ImmutableList

@Composable
fun <T> RadioGroup(
    onClick: (T) -> Unit,
    options: ImmutableList<T>,
    optionTitle: (T) -> String,
    modifier: Modifier = Modifier,
    selectedOption: T? = null,
) {
    if (options.isEmpty()) {
        return
    }

    options.forEach { option ->
        Row(
            modifier = modifier,
            verticalAlignment = Alignment.CenterVertically,
        ) {
            RadioButton(
                onClick = {
                    onClick(option)
                },
                selected = option == selectedOption,
            )

            TextLabelLarge(
                text = optionTitle(option),
            )
        }

Review comment (suggestion):
There should be a preview to investigate the RadioGroup in the design system. 

Also the component should be added to the Catalog app in the atom section.

Code after:
package app.k9mail.core.ui.compose.designsystem.atom

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.material3.RadioButton
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import app.k9mail.core.ui.compose.designsystem.atom.text.TextLabelLarge
import kotlinx.collections.immutable.ImmutableList

@Composable
fun <T> RadioGroup(
    onClick: (T) -> Unit,
    options: ImmutableList<T>,
    optionTitle: (T) -> String,
    modifier: Modifier = Modifier,
    selectedOption: T? = null,
) {
    if (options.isEmpty()) {
        return
    }

    Column(modifier = modifier) {
        options.forEach { option ->
            Row(
                verticalAlignment = Alignment.CenterVertically,
            ) {
                RadioButton(
                    onClick = {
                        onClick(option)
                    },
                    selected = option == selectedOption,
                )

                TextLabelLarge(
                    text = optionTitle(option),
                )
Repository: thunderbird/thunderbird-android
File: core/ui/compose/designsystem/src/main/kotlin/app/k9mail/core/ui/compose/designsystem/atom/RadioGroup.kt (Kotlin)

Code before:
    modifier: Modifier = Modifier,
    selectedOption: T? = null,
) {
    if (options.isEmpty()) {
        return
    }

    options.forEach { option ->
        Row(
            modifier = modifier,
            verticalAlignment = Alignment.CenterVertically,
        ) {
            RadioButton(
                onClick = {
                    onClick(option)
                },
                selected = option == selectedOption,
            )

            TextLabelLarge(
                text = optionTitle(option),
            )
        }
    }
}


Review comment (suggestion):
I think this should be wrapped by a `Column` and the modifier should be applied to it. At the moment the modifier applies padding to each `Row` which results in a lot of spacing.

Code after:
    optionTitle: (T) -> String,
    modifier: Modifier = Modifier,
    selectedOption: T? = null,
) {
    if (options.isEmpty()) {
        return
    }

    Column(modifier = modifier) {
        options.forEach { option ->
            Row(
                verticalAlignment = Alignment.CenterVertically,
            ) {
                RadioButton(
                    onClick = {
                        onClick(option)
                    },
                    selected = option == selectedOption,
                )

                TextLabelLarge(
                    text = optionTitle(option),
                )
            }
        }
    }
}