diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index b62dbb0127..c3e29a2306 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -10,7 +10,7 @@ on: # Enrich gradle.properties for CI/CD env: GRADLE_OPTS: -Dorg.gradle.jvmargs="-Xmx6g -Dfile.encoding=UTF-8 -XX:+HeapDumpOnOutOfMemoryError" -Dkotlin.incremental=false -XX:+UseParallelGC - CI_GRADLE_ARG_PROPERTIES: --stacktrace -PpreDexEnable=false --max-workers 8 --no-daemon --warn + CI_GRADLE_ARG_PROPERTIES: --stacktrace -PpreDexEnable=false --max-workers 8 --no-daemon jobs: checkScript: @@ -33,12 +33,13 @@ jobs: - name: Search for invalid screenshot files run: ./tools/test/checkInvalidScreenshots.py - check: - name: Project Check Suite + # Code checks + konsist: + name: Konsist tests runs-on: ubuntu-latest # Allow all jobs on main and develop. Just one per PR. concurrency: - group: ${{ github.ref == 'refs/heads/main' && format('check-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-develop-{0}', github.sha) || format('check-{0}', github.ref) }} + group: ${{ github.ref == 'refs/heads/main' && format('check-konsist-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-konsist-develop-{0}', github.sha) || format('check-konsist-{0}', github.ref) }} cancel-in-progress: true steps: - uses: actions/checkout@v4 @@ -55,8 +56,40 @@ jobs: uses: gradle/actions/setup-gradle@v3 with: cache-read-only: ${{ github.ref != 'refs/heads/develop' }} - - name: Run code quality check suite - run: ./gradlew runQualityChecks $CI_GRADLE_ARG_PROPERTIES + - name: Run Konsist tests + run: ./gradlew :tests:konsist:testDebugUnitTest $CI_GRADLE_ARG_PROPERTIES --no-daemon + - name: Upload reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: konsist-report + path: | + **/build/reports/**/*.* + + lint: + name: Android lint check + runs-on: ubuntu-latest + # Allow all jobs on main and develop. Just one per PR. + concurrency: + group: ${{ github.ref == 'refs/heads/main' && format('check-lint-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-lint-develop-{0}', github.sha) || format('check-lint-{0}', github.ref) }} + cancel-in-progress: true + steps: + - uses: actions/checkout@v4 + with: + # Ensure we are building the branch and not the branch after being merged on develop + # https://github.com/actions/checkout/issues/881 + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }} + - name: Use JDK 17 + uses: actions/setup-java@v4 + with: + distribution: 'temurin' # See 'Supported distributions' for available options + java-version: '17' + - name: Configure gradle + uses: gradle/actions/setup-gradle@v3 + with: + cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: Run lint + run: ./gradlew :app:lintGplayDebug :app:lintFdroidDebug $CI_GRADLE_ARG_PROPERTIES - name: Upload reports if: always() uses: actions/upload-artifact@v4 @@ -64,6 +97,108 @@ jobs: name: linting-report path: | **/build/reports/**/*.* + + detekt: + name: Detekt checks + runs-on: ubuntu-latest + # Allow all jobs on main and develop. Just one per PR. + concurrency: + group: ${{ github.ref == 'refs/heads/main' && format('check-detekt-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-detekt-develop-{0}', github.sha) || format('check-detekt-{0}', github.ref) }} + cancel-in-progress: true + steps: + - uses: actions/checkout@v4 + with: + # Ensure we are building the branch and not the branch after being merged on develop + # https://github.com/actions/checkout/issues/881 + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }} + - name: Use JDK 17 + uses: actions/setup-java@v4 + with: + distribution: 'temurin' # See 'Supported distributions' for available options + java-version: '17' + - name: Configure gradle + uses: gradle/actions/setup-gradle@v3 + with: + cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: Run Detekt + run: ./gradlew detekt $CI_GRADLE_ARG_PROPERTIES --no-daemon + - name: Upload reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: detekt-report + path: | + **/build/reports/**/*.* + + ktlint: + name: Ktlint checks + runs-on: ubuntu-latest + # Allow all jobs on main and develop. Just one per PR. + concurrency: + group: ${{ github.ref == 'refs/heads/main' && format('check-ktlint-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-ktlint-develop-{0}', github.sha) || format('check-ktlint-{0}', github.ref) }} + cancel-in-progress: true + steps: + - uses: actions/checkout@v4 + with: + # Ensure we are building the branch and not the branch after being merged on develop + # https://github.com/actions/checkout/issues/881 + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }} + - name: Use JDK 17 + uses: actions/setup-java@v4 + with: + distribution: 'temurin' # See 'Supported distributions' for available options + java-version: '17' + - name: Configure gradle + uses: gradle/actions/setup-gradle@v3 + with: + cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: Run Ktlint check + run: ./gradlew ktlintCheck $CI_GRADLE_ARG_PROPERTIES + - name: Upload reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: ktlint-report + path: | + **/build/reports/**/*.* + + knit: + name: Knit checks + runs-on: ubuntu-latest + # Allow all jobs on main and develop. Just one per PR. + concurrency: + group: ${{ github.ref == 'refs/heads/main' && format('check-knit-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('check-knit-develop-{0}', github.sha) || format('check-knit-{0}', github.ref) }} + cancel-in-progress: true + steps: + - uses: actions/checkout@v4 + with: + # Ensure we are building the branch and not the branch after being merged on develop + # https://github.com/actions/checkout/issues/881 + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }} + - name: Use JDK 17 + uses: actions/setup-java@v4 + with: + distribution: 'temurin' # See 'Supported distributions' for available options + java-version: '17' + - name: Configure gradle + uses: gradle/actions/setup-gradle@v3 + with: + cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: Run Knit + run: ./gradlew knitCheck $CI_GRADLE_ARG_PROPERTIES + + upload_reports: + name: Project Check Suite + runs-on: ubuntu-latest + needs: [konsist, lint, ktlint, detekt] + steps: + - uses: actions/checkout@v4 + with: + # Ensure we are building the branch and not the branch after being merged on develop + # https://github.com/actions/checkout/issues/881 + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }} + - name: Download reports from previous jobs + uses: actions/download-artifact@v4 - name: Prepare Danger if: always() run: | diff --git a/changelog.d/2291.bugfix b/changelog.d/2291.bugfix new file mode 100644 index 0000000000..73e84559a7 --- /dev/null +++ b/changelog.d/2291.bugfix @@ -0,0 +1 @@ +Make sure explicit links in messages take priority over links found by linkification (urls, emails, phone numbers, etc.) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt index 2aba9c3669..5c0623b480 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt @@ -268,12 +268,16 @@ class TimelineItemContentMessageFactory @Inject constructor( } // Find and set as URLSpans any links present in the text LinkifyCompat.addLinks(this, Linkify.WEB_URLS or Linkify.PHONE_NUMBERS or Linkify.EMAIL_ADDRESSES) - // Restore old spans if they don't conflict with the new ones + // Restore old spans, remove new ones if there is a conflict for ((urlSpan, location) in oldURLSpans) { val (start, end) = location - if (getSpans(start, end).isEmpty()) { - setSpan(urlSpan, start, end, Spannable.SPAN_EXCLUSIVE_EXCLUSIVE) + val addedSpans = getSpans(start, end).orEmpty() + if (addedSpans.isNotEmpty()) { + for (span in addedSpans) { + removeSpan(span) + } } + setSpan(urlSpan, start, end, Spannable.SPAN_EXCLUSIVE_EXCLUSIVE) } return this } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt index 35de78f65b..6d8fb1ad9a 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt @@ -16,7 +16,9 @@ package io.element.android.features.messages.impl.timeline.factories.event +import android.net.Uri import android.text.SpannableString +import android.text.SpannableStringBuilder import android.text.Spanned import android.text.style.URLSpan import androidx.core.text.buildSpannedString @@ -46,6 +48,7 @@ import io.element.android.libraries.matrix.api.media.ImageInfo import io.element.android.libraries.matrix.api.media.MediaSource import io.element.android.libraries.matrix.api.media.ThumbnailInfo import io.element.android.libraries.matrix.api.media.VideoInfo +import io.element.android.libraries.matrix.api.permalink.PermalinkData import io.element.android.libraries.matrix.api.timeline.item.event.AudioMessageType import io.element.android.libraries.matrix.api.timeline.item.event.EmoteMessageType import io.element.android.libraries.matrix.api.timeline.item.event.FileMessageType @@ -75,6 +78,7 @@ import org.robolectric.RobolectricTestRunner import kotlin.time.Duration import kotlin.time.Duration.Companion.minutes +@Suppress("LargeClass") @RunWith(RobolectricTestRunner::class) class TimelineItemContentMessageFactoryTest { @Test @@ -641,6 +645,31 @@ class TimelineItemContentMessageFactoryTest { assertThat((result as TimelineItemEmoteContent).formattedBody).isEqualTo(SpannableString("* Bob formatted")) } + @Test + fun `a message with existing URLSpans keeps it after linkification`() = runTest { + val expectedSpanned = SpannableStringBuilder().apply { + append("Test ") + inSpans(URLSpan("https://www.example.org")) { + append("me@matrix.org") + } + } + val sut = createTimelineItemContentMessageFactory( + htmlConverterTransform = { expectedSpanned }, + permalinkParser = FakePermalinkParser { PermalinkData.FallbackLink(Uri.EMPTY) } + ) + val result = sut.create( + content = createMessageContent( + type = TextMessageType( + body = "Test [me@matrix.org](https://www.example.org)", + formatted = FormattedBody(MessageFormat.HTML, "Test me@matrix.org") + ) + ), + senderDisambiguatedDisplayName = "Bob", + eventId = AN_EVENT_ID, + ) + assertThat((result as TimelineItemTextContent).formattedBody).isEqualTo(expectedSpanned) + } + private fun createMessageContent( body: String = "Body", inReplyTo: InReplyTo? = null, @@ -660,12 +689,13 @@ class TimelineItemContentMessageFactoryTest { private fun createTimelineItemContentMessageFactory( featureFlagService: FeatureFlagService = FakeFeatureFlagService(), htmlConverterTransform: (String) -> CharSequence = { it }, + permalinkParser: FakePermalinkParser = FakePermalinkParser(), ) = TimelineItemContentMessageFactory( fileSizeFormatter = FakeFileSizeFormatter(), fileExtensionExtractor = FileExtensionExtractorWithoutValidation(), featureFlagService = featureFlagService, htmlConverterProvider = FakeHtmlConverterProvider(htmlConverterTransform), - permalinkParser = FakePermalinkParser(), + permalinkParser = permalinkParser, ) private fun createStickerContent( diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 38f879f28b..5061006ec0 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -136,7 +136,7 @@ test_arch_core = "androidx.arch.core:core-testing:2.2.0" test_junit = "junit:junit:4.13.2" test_runner = "androidx.test:runner:1.5.2" test_junitext = "androidx.test.ext:junit:1.1.5" -test_mockk = "io.mockk:mockk:1.13.10" +test_mockk = "io.mockk:mockk:1.13.11" test_konsist = "com.lemonappdev:konsist:0.13.0" test_turbine = "app.cash.turbine:turbine:1.1.0" test_truth = "com.google.truth:truth:1.4.2" diff --git a/plugins/src/main/kotlin/extension/KoverExtension.kt b/plugins/src/main/kotlin/extension/KoverExtension.kt index 68f89f300f..ff66d1d6c6 100644 --- a/plugins/src/main/kotlin/extension/KoverExtension.kt +++ b/plugins/src/main/kotlin/extension/KoverExtension.kt @@ -122,7 +122,6 @@ fun Project.setupKover() { total { verify { - onCheck = true // General rule: minimum code coverage. rule("Global minimum code coverage.") { groupBy = GroupingEntityType.APPLICATION @@ -140,7 +139,6 @@ fun Project.setupKover() { } variant(KoverVariant.Presenters.variantName) { verify { - onCheck = true // Rule to ensure that coverage of Presenters is sufficient. rule("Check code coverage of presenters") { groupBy = GroupingEntityType.CLASS @@ -171,7 +169,6 @@ fun Project.setupKover() { } variant(KoverVariant.States.variantName) { verify { - onCheck = true // Rule to ensure that coverage of States is sufficient. rule("Check code coverage of states") { groupBy = GroupingEntityType.CLASS @@ -214,7 +211,6 @@ fun Project.setupKover() { } variant(KoverVariant.Views.variantName) { verify { - onCheck = true // Rule to ensure that coverage of Views is sufficient (deactivated for now). rule("Check code coverage of views") { groupBy = GroupingEntityType.CLASS