Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve OSS licenses screen #1018

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

fumiya-kume
Copy link
Contributor

@fumiya-kume fumiya-kume commented Aug 29, 2023

Issue

Overview (Required)

  • Add OssLicenseScreen to list up the libraries
  • Add OssLicenseDetailScreen to show the license contents
  • Parsing the generated contents from the Google's licenses plugin.

TODO

  • Replace Screen title handling
  • Remove Licenses position from the domain model
  • Refactoring library grouping logic
  • Align naming rule for the license/library
  • Add implementation for navigation up
  • Improve Card expand state handling
  • Support Large screen devices

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Before After

Movie (Optional)

Before After
licensescreen.mp4

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Test Results

218 tests   218 ✔️  8m 39s ⏱️
  13 suites      0 💤
  13 files        0

Results for commit ecd9b9f.

♻️ This comment has been updated with latest results.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 29, 2023 15:10 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 3, 2023 03:41 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 3, 2023 08:12 Inactive
@fumiya-kume fumiya-kume changed the title [WIP] Improve OSS licenses screen Improve OSS licenses screen Sep 12, 2023
@fumiya-kume fumiya-kume marked this pull request as ready for review September 12, 2023 12:43
@fumiya-kume fumiya-kume requested a review from a team as a code owner September 12, 2023 12:43
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 12, 2023 13:05 Inactive
import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.persistentListOf

data class License(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put sources to appropriate modules?
I think
UI will be feature/about,
Repository's implementation and api will be core/data,
This(License) will be core/model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's make sense.

OssLicenseRepositoryImpl is still in the app module since limitation of Google's license plugin.
Except that, placing to appropriate modules. 🙇

@AppAndroidOssLicenseConfig
fun provideOssLicenseRepositoryProvider(
@ApplicationContext context: Context,
): OssLicenseRepository = OssLicenseRepositoryImpl(context)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using Default〜 prefix for the concrete class 🙏

import okio.source
import javax.inject.Inject

class OssLicenseRepositoryImpl @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving the logic to DefaultOssLicenseDataSource to simplify the app's implementation? I think this is similar to an API, since both handle data parsing. Therefore, I suggest transferring the code to DefaultOssLicenseDataSource.

class DefaultOssLicenseDataSource @Inject constructor(
    private val context: Context,
) : OssLicenseDataSource {

    override suspend fun licenseFlow(): OssLicense {
        return withContext(context = Dispatchers.IO) {
            val details = readLicensesFile().toRowList()
            val metadata = readLicensesMetaFile().toRowList().parseToLibraryItem(
                details = details,
            )
            val groupList = metadata.distinctBy { it.name }.groupByCategory()
                .map {
                    OssLicenseGroup(
                        title = it.key,
                        licenses = it.value,
                    )
                }.toPersistentList()
            OssLicense(groupList)
        }
    }

    private fun List<License>.groupByCategory(): Map<String, List<License>> {
        val categoryList = listOf(
            "Android Support",
            "Android Datastore",
            "Android ",
            "Compose UI",
            "Compose Material3",
            "Compose ",
            "AndroidX lifecycle",
            "AndroidX ",
            "Kotlin",
            "Dagger",
            "Firebase",
            "Ktorfit",
            "okhttp",
            "ktor",
        )
        return groupBy { license ->
            categoryList.firstOrNull {
                license.name.startsWith(
                    prefix = it,
                    ignoreCase = true,
                )
            } ?: "etc"
        }
    }

    private fun readLicensesMetaFile(): BufferedSource {
        return context.resources.openRawResource(raw.third_party_license_metadata)
            .source()
            .buffer()
    }

    private fun readLicensesFile(): BufferedSource {
        return context.resources.openRawResource(raw.third_party_licenses)
            .source()
            .buffer()
    }

    private fun List<String>.parseToLibraryItem(details: List<String>): List<License> {
        return mapIndexed { index, value ->
            val (position, name) = value.split(' ', limit = 2)
            val (offset, length) = position.split(':').map { it.toInt() }
            val id = name.replace(' ', '-')
            License(
                name = name,
                id = id,
                offset = offset,
                length = length,
                detail = details[index],
            )
        }
    }

    private fun BufferedSource.toRowList(): List<String> {
        val list: MutableList<String> = mutableListOf()
        while (true) {
            val line = readUtf8Line() ?: break
            list.add(line)
        }
        return list
    }
}

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 16, 2023 04:31 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 16, 2023 05:18 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 16, 2023 06:14 Inactive
@github-actions
Copy link

Hi @fumiya-kume! Codes seem to be unformatted. To resolve this issue, please run ./gradlew detekt --auto-correct and fix the results of ./gradlew lintDebug.. Thank you for your contribution.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 16, 2023 08:01 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 17, 2023 05:01 Inactive
@takahirom
Copy link
Member

Could you add a screenshot test for this screen? This is because all other screens have one, and this PR causes our test coverage to shrink. 🙇

@github-actions
Copy link

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 23, 2023 10:41 Inactive
@@ -33,7 +33,7 @@ jobs:
workflow: UnitTest.yml
branch: main

- run: ./gradlew compareRoborazziDebug compareRoborazziDevDebug --stacktrace -Pscreenshot
- run: ./gradlew compareRoborazziDebug compareRoborazziDevRelease --stacktrace -Pscreenshot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem are you experiencing with compareRoborazziDevDebug? 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Add title for the license screen
2 participants