ronantakizawa/codereview-bench huggingface dataset:
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/Resource.kt (Kotlin)

Code before:
package org.jetbrains.compose.resources

import androidx.compose.runtime.Immutable

@RequiresOptIn("This API is experimental and is likely to change in the future.")
annotation class ExperimentalResourceApi

@RequiresOptIn("This is internal API of the Compose gradle plugin.")
annotation class InternalResourceApi

/**
 * Represents a resource with an ID and a set of resource items.
 *
 * @property id The ID of the resource.
 * @property items The set of resource items associated with the resource.
 */
@ExperimentalResourceApi
@Immutable
sealed class Resource
@InternalResourceApi constructor(
    internal val id: String,
    internal val items: Set<ResourceItem>
) {
    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (other == null || this::class != other::class) return false

        other as Resource

        return id == other.id
    }

    override fun hashCode(): Int {
        return id.hashCode()
    }
}

/**
 * Represents a resource item with qualifiers and a path.
 *
 * @property qualifiers The qualifiers of the resource item.
 * @property path The path of the resource item.

Review comment (suggestion):
Not for this PR, but because we are removing experimental, we have to support API backward compatibility, and better to apply https://github.com/Kotlin/binary-compatibility-validator plugin for that

Code after:
package org.jetbrains.compose.resources

import androidx.compose.runtime.Immutable

@RequiresOptIn("This API is experimental and is likely to change in the future.")
annotation class ExperimentalResourceApi

@RequiresOptIn("This is internal API of the Compose gradle plugin.")
annotation class InternalResourceApi

/**
 * Represents a resource with an ID and a set of resource items.
 *
 * @property id The ID of the resource.
 * @property items The set of resource items associated with the resource.
 */
@Immutable
sealed class Resource
@InternalResourceApi constructor(
    internal val id: String,
    internal val items: Set<ResourceItem>
) {
    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (other == null || this::class != other::class) return false

        other as Resource

        return id == other.id
    }

    override fun hashCode(): Int {
        return id.hashCode()
    }
}

/**
 * Represents a resource item with qualifiers and a path.
 *
 * @property qualifiers The qualifiers of the resource item.
 * @property path The path of the resource item.
 * @property offset The offset in bytes of the resource in the file. '-1' means the resource is whole file
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/FontResources.kt (Kotlin)

Code before:
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The optional resource environment.
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment = getResourceEnvironment(),
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}

Review comment (suggestion):
Better to make `getResourceEnvironment` public. It is useful for making wrappers around it:
```
suspend fun getThumbnailResourceBytes(
    environment: ResourceEnvironment = getResourceEnvironment(),
    size: IntSize,
    resource: FontResource
): ByteArray {
```

And after making it public, we need a proper name, probably `getSystemResourceEnvironment()`. Or `val ResourceEnvironment.System get()` - we used such approach in `core` in some places, it seems more discoverable.

Code after:
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The resource environment, which can be obtained from [rememberResourceEnvironment] or [getSystemResourceEnvironment].
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment,
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/FontResources.kt (Kotlin)

Code before:
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The optional resource environment.
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment = getResourceEnvironment(),
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}

Review comment (suggestion):
Having `environment` on the first place forces users to use `resource` name explicitly. Was this the intention?

I don't have a strong opinion here. On one hand, it is better for users to make a conscience choice to use the system environment, on the other hand it is verbose.

Code after:
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The resource environment, which can be obtained from [rememberResourceEnvironment] or [getSystemResourceEnvironment].
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment,
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/FontResources.kt (Kotlin)

Code before:
/**
 * Represents a font resource.
 *
 * @param id The identifier of the font resource.
 * @param items The set of resource items associated with the font resource.
 *
 * @see Resource
 */
@Immutable
class FontResource
@InternalResourceApi constructor(id: String, items: Set<ResourceItem>): Resource(id, items)

/**
 * Creates a font using the specified font resource, weight, and style.
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The optional resource environment.
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment = getResourceEnvironment(),
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}

Review comment (suggestion):
It is late and not for this PR, but `Font` function isn't by Compose guidelines.

If it returns a value, it should start with lower case.

Code after:
/**
 * Represents a font resource.
 *
 * @param id The identifier of the font resource.
 * @param items The set of resource items associated with the font resource.
 *
 * @see Resource
 */
@Immutable
class FontResource
@InternalResourceApi constructor(id: String, items: Set<ResourceItem>): Resource(id, items)

/**
 * Creates a font using the specified font resource, weight, and style.
 *
 * @param resource The font resource to be used.
 * @param weight The weight of the font. Default value is [FontWeight.Normal].
 * @param style The style of the font. Default value is [FontStyle.Normal].
 *
 * @return The created [Font] object.
 *
 * @throws NotFoundException if the specified resource ID is not found.
 */
@Composable
expect fun Font(
    resource: FontResource,
    weight: FontWeight = FontWeight.Normal,
    style: FontStyle = FontStyle.Normal
): Font

/**
 * Retrieves the byte array of the font resource.
 *
 * @param environment The resource environment, which can be obtained from [rememberResourceEnvironment] or [getSystemResourceEnvironment].
 * @param resource The font resource.
 * @return The byte array representing the font resource.
 */
@ExperimentalResourceApi
suspend fun getFontResourceBytes(
    environment: ResourceEnvironment,
    resource: FontResource
): ByteArray {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    return DefaultResourceReader.read(resourceItem.path)
}
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/StringArrayResources.kt (Kotlin)

Code before:
    }
    return array
}

/**
 * Loads a list of strings using the specified string array resource.
 *
 * @param resource The string array resource to be used.
 * @return A list of strings representing the items in the string array.
 *
 * @throws IllegalStateException if the string array with the given ID is not found.
 */
suspend fun getStringArray(resource: StringArrayResource): List<String> =
    loadStringArray(resource, DefaultResourceReader, getResourceEnvironment())

/**
 * Loads a list of strings using the specified string array resource.
 *
 * @param environment The resource environment.
 * @param resource The string array resource to be used.
 * @return A list of strings representing the items in the string array.
 *
 * @throws IllegalStateException if the string array with the given ID is not found.
 */
suspend fun getStringArray(
    environment: ResourceEnvironment,
    resource: StringArrayResource
): List<String> = loadStringArray(resource, DefaultResourceReader, environment)

private suspend fun loadStringArray(
    resource: StringArrayResource,
    resourceReader: ResourceReader,
    environment: ResourceEnvironment
): List<String> {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    val item = getStringItem(resourceItem, resourceReader) as StringItem.Array
    return item.items
}


Review comment (suggestion):
To be consistent with other types, maybe we need `suspend fun vectorResource(ResourceEnvironment`, `suspend fun imageResource(ResourceEnvironment`, `suspend fun font(ResourceEnvironment`

Code after:
    }
    return array
}

/**
 * Loads a list of strings using the specified string array resource.
 *
 * @param resource The string array resource to be used.
 * @return A list of strings representing the items in the string array.
 *
 * @throws IllegalStateException if the string array with the given ID is not found.
 */
suspend fun getStringArray(resource: StringArrayResource): List<String> =
    loadStringArray(resource, DefaultResourceReader, getSystemResourceEnvironment())

/**
 * Loads a list of strings using the specified string array resource.
 *
 * @param environment The resource environment.
 * @param resource The string array resource to be used.
 * @return A list of strings representing the items in the string array.
 *
 * @throws IllegalStateException if the string array with the given ID is not found.
 */
@ExperimentalResourceApi
suspend fun getStringArray(
    environment: ResourceEnvironment,
    resource: StringArrayResource
): List<String> = loadStringArray(resource, DefaultResourceReader, environment)

private suspend fun loadStringArray(
    resource: StringArrayResource,
    resourceReader: ResourceReader,
    environment: ResourceEnvironment
): List<String> {
    val resourceItem = resource.getResourceItemByEnvironment(environment)
    val item = getStringItem(resourceItem, resourceReader) as StringItem.Array
    return item.items
}
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt (Kotlin)

Code before:
package org.jetbrains.compose.resources

import androidx.compose.foundation.isSystemInDarkTheme
import androidx.compose.runtime.*
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.text.intl.Locale

@ExperimentalResourceApi
data class ResourceEnvironment internal constructor(
    internal val language: LanguageQualifier,
    internal val region: RegionQualifier,
    internal val theme: ThemeQualifier,
    internal val density: DensityQualifier
)

internal interface ComposeEnvironment {
    @Composable
    fun rememberEnvironment(): ResourceEnvironment
}

internal val DefaultComposeEnvironment = object : ComposeEnvironment {
    @Composable
    override fun rememberEnvironment(): ResourceEnvironment {
        val composeLocale = Locale.current
        val composeTheme = isSystemInDarkTheme()
        val composeDensity = LocalDensity.current

        //cache ResourceEnvironment unless compose environment is changed
        return remember(composeLocale, composeTheme, composeDensity) {
            ResourceEnvironment(
                LanguageQualifier(composeLocale.language),
                RegionQualifier(composeLocale.region),
                ThemeQualifier.selectByValue(composeTheme),
                DensityQualifier.selectByDensity(composeDensity.density)

Review comment (suggestion):
[Don't use data classes in an API﻿](https://kotlinlang.org/docs/jvm-api-guidelines-backward-compatibility.html#don-t-use-data-classes-in-an-api)

Code after:
package org.jetbrains.compose.resources

import androidx.compose.foundation.isSystemInDarkTheme
import androidx.compose.runtime.*
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.text.intl.Locale

@ExperimentalResourceApi
class ResourceEnvironment internal constructor(
    internal val language: LanguageQualifier,
    internal val region: RegionQualifier,
    internal val theme: ThemeQualifier,
    internal val density: DensityQualifier
) {
    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (other == null || this::class != other::class) return false

        other as ResourceEnvironment

        if (language != other.language) return false
        if (region != other.region) return false
        if (theme != other.theme) return false
        if (density != other.density) return false

        return true
    }

    override fun hashCode(): Int {
        var result = language.hashCode()
        result = 31 * result + region.hashCode()
        result = 31 * result + theme.hashCode()
        result = 31 * result + density.hashCode()
        return result
Repository: JetBrains/compose-multiplatform
File: gradle-plugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/KotlinCompatibilityTest.kt (Kotlin)

Code before:

    private fun testMpp(kotlinVersion: String) = with(
        testProject(
            TestProjects.mpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
        )
    ) {
        val logLine = "Kotlin MPP app is running!"
        gradle("run").checks {
            check.taskSuccessful(":run")
            check.logContains(logLine)
        }
    }

    private fun testJsMpp(kotlinVersion: String) = with(
        testProject(
            TestProjects.jsMpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
        )
    ) {
        gradle(":compileKotlinJs").checks {
            check.taskSuccessful(":compileKotlinJs")
        }
    }

    @Test
    fun testNewCompilerPluginError() {
        val testProject = testProject(
            TestProjects.mpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = newCompilerIsAvailableVersion)
        )
        testProject.gradleFailure("tasks").checks {
            check.logContains(newComposeCompilerError)
        }
    }
}


Review comment (suggestion):
Let's add TODO:
```
// TODO replace by this after Kotlin 2.0 release
// testEnvironment = defaultTestEnvironment.copy(kotlinVersion = "2.0")
```
dev kotlin builds can be removed from maven later

Code after:

    private fun testMpp(kotlinVersion: String) = with(
        testProject(
            TestProjects.mpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
        )
    ) {
        val logLine = "Kotlin MPP app is running!"
        gradle("run").checks {
            check.taskSuccessful(":run")
            check.logContains(logLine)
        }
    }

    private fun testJsMpp(kotlinVersion: String) = with(
        testProject(
            TestProjects.jsMpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
        )
    ) {
        gradle(":compileKotlinJs").checks {
            check.taskSuccessful(":compileKotlinJs")
        }
    }

    @Test
    fun testNewCompilerPluginError() {
        // TODO replace by this after Kotlin 2.0 release
        // testEnvironment = defaultTestEnvironment.copy(kotlinVersion = "2.0")
        val testProject = testProject(
            TestProjects.mpp,
            testEnvironment = defaultTestEnvironment.copy(kotlinVersion = newCompilerIsAvailableVersion)
        )
        testProject.gradleFailure("tasks").checks {
            check.logContains(newComposeCompilerError)
        }
    }
}
Repository: JetBrains/compose-multiplatform
File: gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/JvmApplicationDistributions.kt (Kotlin)

Code before:
)

abstract class JvmApplicationDistributions : AbstractDistributions() {
    var modules = arrayListOf(*DEFAULT_RUNTIME_MODULES)
    fun modules(vararg modules: String) {
        this.modules.addAll(modules.toList())
    }
    var includeAllModules: Boolean = false

    val linux: LinuxPlatformSettings = objects.newInstance(LinuxPlatformSettings::class.java)
    open fun linux(fn: Action<LinuxPlatformSettings>) {
        fn.execute(linux)
    }

    val macOS: JvmMacOSPlatformSettings = objects.newInstance(JvmMacOSPlatformSettings::class.java)
    open fun macOS(fn: Action<JvmMacOSPlatformSettings>) {
        fn.execute(macOS)
    }

    val windows: WindowsPlatformSettings = objects.newInstance(WindowsPlatformSettings::class.java)
    fun windows(fn: Action<WindowsPlatformSettings>) {
        fn.execute(windows)
    }
    
    internal val fileAssociations: MutableSet<FileAssociation> = mutableSetOf()
    fun fileAssociation(mimeType: String, extension: String, description: String) {
        fileAssociations.add(FileAssociation(mimeType, extension, description))
    }
}

Review comment (suggestion):
Let's move it to https://github.com/JetBrains/compose-multiplatform/blob/67da8d12f9b18f6aefe68a449cb2ed8c3eb8646f/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/PlatformSettings.kt#L19

Other common properties are there, and we can specify them separately.

Code after:
    "java.base", "java.desktop", "java.logging", "jdk.crypto.ec"
)

abstract class JvmApplicationDistributions : AbstractDistributions() {
    var modules = arrayListOf(*DEFAULT_RUNTIME_MODULES)
    fun modules(vararg modules: String) {
        this.modules.addAll(modules.toList())
    }
    var includeAllModules: Boolean = false

    val linux: LinuxPlatformSettings = objects.newInstance(LinuxPlatformSettings::class.java)
    open fun linux(fn: Action<LinuxPlatformSettings>) {
        fn.execute(linux)
    }

    val macOS: JvmMacOSPlatformSettings = objects.newInstance(JvmMacOSPlatformSettings::class.java)
    open fun macOS(fn: Action<JvmMacOSPlatformSettings>) {
        fn.execute(macOS)
    }

    val windows: WindowsPlatformSettings = objects.newInstance(WindowsPlatformSettings::class.java)
    fun windows(fn: Action<WindowsPlatformSettings>) {
        fn.execute(windows)
    }
    
    @JvmOverloads
    fun fileAssociation(
        mimeType: String, extension: String, description: String,
        linuxIconFile: File? = null, windowsIconFile: File? = null, macOSIconFile: File? = null,
    ) {
        linux.fileAssociation(mimeType, extension, description, linuxIconFile)
        windows.fileAssociation(mimeType, extension, description, windowsIconFile)
        macOS.fileAssociation(mimeType, extension, description, macOSIconFile)
    }
}