From d4a39bd60954443e7446f2ea7d1462c40de6d480 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 11 Jan 2026 17:48:02 +0100 Subject: [PATCH 1/5] Fixed GalleryAdapter#getFilesCount() The method actually returned the number of rows, but each row contains multiple files (currently: 2 or 5, depending on the display rotation). In the unit test, the expected file count was changed from the original and correct value 4 to 2 in the commit 66d8756b, but the correct change is in the implementation. Hence, reinstating the old expected value and the test does pass. Signed-off-by: Philipp Hasper --- .../main/java/com/owncloud/android/ui/adapter/GalleryAdapter.kt | 2 +- .../java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/ui/adapter/GalleryAdapter.kt b/app/src/main/java/com/owncloud/android/ui/adapter/GalleryAdapter.kt index f7f0bc2ef76b..edbbe0d4a19d 100644 --- a/app/src/main/java/com/owncloud/android/ui/adapter/GalleryAdapter.kt +++ b/app/src/main/java/com/owncloud/android/ui/adapter/GalleryAdapter.kt @@ -106,7 +106,7 @@ class GalleryAdapter( } private fun updateFilesCount() { - cachedFilesCount = files.fold(0) { acc, item -> acc + item.rows.size } + cachedFilesCount = files.sumOf { it.rows.sumOf { it.files.size } } } private fun rebuildFilePositionMap() { diff --git a/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt b/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt index eca3acfef8ed..8f1446c330e3 100644 --- a/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt +++ b/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt @@ -95,6 +95,6 @@ class GalleryAdapterTest { sut.addFiles(list) - assertEquals(2, sut.getFilesCount()) + assertEquals(4, sut.getFilesCount()) } } From 4293d2096d49facd660f92c7457c23f84592b075 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Sun, 11 Jan 2026 20:42:48 +0100 Subject: [PATCH 2/5] Test bugfix of #15918 and #16194: Gallery multiselect could crash The crash came from a collision of the improperly coded hash function GalleryRow#calculateHashCode(). Simply summing the row's file hashes can easily cause a collision as the same sum can also be achieved by two other file hashes. Take two rows with two files each. All having the same parentId=0. Row 1: 263512 and 148830 Row 2: 279897 and 132445 Summing the hashes given by calculateHashCode() will return in the same value for Row1 and Row2. PR #15918 fixed this by migrating away from this faulty hash function. This commit tests the bugfix first with a known collision and then some random fileIds for some additional explorative testing. This commit also removes the problematic calculateHashCode(), as it is now unused. Signed-off-by: Philipp Hasper --- .../owncloud/android/datamodel/GalleryRow.kt | 1 - .../android/ui/adapter/GalleryAdapterTest.kt | 129 +++++++++++++++++- 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/datamodel/GalleryRow.kt b/app/src/main/java/com/owncloud/android/datamodel/GalleryRow.kt index febd3aaf19a4..6e99de382fae 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/GalleryRow.kt +++ b/app/src/main/java/com/owncloud/android/datamodel/GalleryRow.kt @@ -14,5 +14,4 @@ data class GalleryRow(val files: List, val defaultHeight: Int, val defau fun getMaxHeight(): Float = files.maxOfOrNull { OCFileUtils.getImageSize(it, defaultHeight.toFloat()).second.toFloat() } ?: 0f - fun calculateHashCode(): Long = files.sumOf { it.hashCode() }.toLong() } diff --git a/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt b/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt index 8f1446c330e3..2b7bb193a56e 100644 --- a/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt +++ b/app/src/test/java/com/owncloud/android/ui/adapter/GalleryAdapterTest.kt @@ -8,6 +8,7 @@ package com.owncloud.android.ui.adapter import android.content.Context +import android.text.TextUtils import com.nextcloud.client.account.User import com.nextcloud.client.jobs.upload.FileUploadHelper import com.nextcloud.client.preferences.AppPreferences @@ -18,14 +19,18 @@ import com.owncloud.android.datamodel.OCFile import com.owncloud.android.ui.activity.ComponentsGetter import com.owncloud.android.ui.interfaces.OCFileListFragmentInterface import com.owncloud.android.utils.theme.ViewThemeUtils -import junit.framework.Assert.assertEquals +import io.mockk.every +import io.mockk.mockkStatic import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Before import org.junit.Test import org.mockito.Mock import org.mockito.MockitoAnnotations import org.mockito.kotlin.doReturn import org.mockito.kotlin.whenever +import kotlin.random.Random class GalleryAdapterTest { @Mock @@ -55,20 +60,25 @@ class GalleryAdapterTest { private lateinit var mocks: AutoCloseable @Before - fun setUp() { + fun setUpMocks() { mocks = MockitoAnnotations.openMocks(this) + + whenever(transferServiceGetter.storageManager) doReturn storageManager + whenever(transferServiceGetter.fileUploaderHelper) doReturn fileUploadHelper + + // Mocking TextUtils so OCFile#existsOnDevice() doesn't fail due to Android not being available + // This is needed so OCFile#toString() works for logging errors + mockkStatic(TextUtils::class) + every { TextUtils.isEmpty(any()) } answers { arg(0)?.isEmpty() ?: true } } @After - fun tearDown() { + fun tearDownMocks() { mocks.close() } @Test fun testItemCount() { - whenever(transferServiceGetter.storageManager) doReturn storageManager - whenever(transferServiceGetter.fileUploaderHelper) doReturn fileUploadHelper - val thumbnailSize = 50 val sut = GalleryAdapter( @@ -97,4 +107,111 @@ class GalleryAdapterTest { assertEquals(4, sut.getFilesCount()) } + + @Test + @Suppress("LongMethod") + fun testIdUniqueness() { + val thumbnailSize = 50 + + val sut = GalleryAdapter( + context, + user, + ocFileListFragmentInterface, + preferences, + transferServiceGetter, + viewThemeUtils, + 5, + thumbnailSize + ) + val rows = mutableListOf() + + // Test a known (former) hash collision + val row1File1 = 263512L + val row1File2 = 148830L + val row2File1 = 279897L + val row2File2 = 132445L + rows.add( + GalleryRow( + listOf( + OCFile("/$row1File1.md").apply { + fileId = row1File1 + parentId = 0 + }, + OCFile("/$row1File2.md").apply { + fileId = row1File2 + parentId = 0 + } + ), + thumbnailSize, + thumbnailSize + ) + ) + rows.add( + GalleryRow( + listOf( + OCFile("/$row2File1.md").apply { + fileId = row2File1 + parentId = 0 + }, + OCFile("/$row2File2.md").apply { + fileId = row2File2 + parentId = 0 + } + ), + thumbnailSize, + thumbnailSize + ) + ) + val alreadyUsedFileIds = listOf(row1File1, row1File2, row2File1, row2File2) + + // Generate some random Ids for some explorative testing + val randomFileIds = uniquePositiveRandomLongs(10000, 1000000) + for (i in 0..() + for (i in 0.. { + require(count >= 0) { "count must be non-negative" } + if (count == 0) return emptyList() + + val set = HashSet(count) + while (set.size < count) { + // produce positive (> 0) values + set.add(Random.nextLong(1, max)) + } + return set.toList() + } } From bd06be7b0974d4a5425509f8ee995475df6d8b5f Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Fri, 2 Jan 2026 18:12:20 +0100 Subject: [PATCH 3/5] UI test for multi selection in media gallery This was originally meant to find the cause of crashes caused by non-unique IDs in the ViewHolder (#15918 and #16194). But the UI test still succeeded because it didn't carefully craft the fileIds to cause the issue. Once the actual reason, a collision of an improper hash function, was found, a unit test for that scenario was added instead - see prior commit. Committing this UI test anyways, as it might be able to catch other, future regressions. Signed-off-by: Philipp Hasper --- .../android/ui/fragment/GalleryFragmentIT.kt | 105 +++++++++++++++++- 1 file changed, 102 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/java/com/owncloud/android/ui/fragment/GalleryFragmentIT.kt b/app/src/androidTest/java/com/owncloud/android/ui/fragment/GalleryFragmentIT.kt index 0bdf133e055f..3b4ede5f274b 100644 --- a/app/src/androidTest/java/com/owncloud/android/ui/fragment/GalleryFragmentIT.kt +++ b/app/src/androidTest/java/com/owncloud/android/ui/fragment/GalleryFragmentIT.kt @@ -1,6 +1,7 @@ /* * Nextcloud - Android Client * + * SPDX-FileCopyrightText: 2026 Philipp Hasper * SPDX-FileCopyrightText: 2025 Alper Ozturk * SPDX-FileCopyrightText: 2022 Tobias Kaminsky * SPDX-FileCopyrightText: 2022 Nextcloud GmbH @@ -12,24 +13,37 @@ import android.graphics.Bitmap import android.graphics.Canvas import android.graphics.Color import android.graphics.Paint +import android.view.View +import android.view.ViewGroup +import android.widget.FrameLayout +import androidx.recyclerview.widget.RecyclerView import androidx.test.core.app.launchActivity import androidx.test.espresso.Espresso.onView +import androidx.test.espresso.UiController +import androidx.test.espresso.ViewAction import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.contrib.RecyclerViewActions +import androidx.test.espresso.contrib.RecyclerViewActions.actionOnItemAtPosition import androidx.test.espresso.matcher.ViewMatchers.isDisplayed import androidx.test.espresso.matcher.ViewMatchers.isRoot +import androidx.test.espresso.matcher.ViewMatchers.withId import com.nextcloud.test.TestActivity import com.owncloud.android.AbstractIT +import com.owncloud.android.R import com.owncloud.android.datamodel.OCFile import com.owncloud.android.datamodel.ThumbnailsCacheManager import com.owncloud.android.datamodel.ThumbnailsCacheManager.PREFIX_RESIZED_IMAGE import com.owncloud.android.lib.common.utils.Log_OC import com.owncloud.android.lib.resources.files.model.ImageDimension +import com.owncloud.android.ui.adapter.GalleryRowHolder import com.owncloud.android.utils.ScreenshotTest import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Before import org.junit.Test import java.util.Random +import org.hamcrest.Matchers.`is` as isSameView class GalleryFragmentIT : AbstractIT() { private val testClassName = "com.owncloud.android.ui.fragment.GalleryFragmentIT" @@ -85,14 +99,99 @@ class GalleryFragmentIT : AbstractIT() { } } - private fun createImage(id: Int, width: Int? = null, height: Int? = null) { + @Test + fun multiSelect() { + val imageCount = 100 + for (num in 1..imageCount) { + // Spread the files over multiple days to also get multiple sections + val secondsPerDay = 1L * 24 * 60 * 60 + createImage(10000000 + num * 7 * secondsPerDay, 700, 300) + } + + // Test that scrolling through the whole list is possible without a crash + launchActivity().use { scenario -> + lateinit var galleryFragment: GalleryFragment + scenario.onActivity { testActivity -> + galleryFragment = GalleryFragment() + testActivity.addFragment(galleryFragment) + } + onView(isRoot()).check(matches(isDisplayed())) + + onView(withId(R.id.list_root)) + .perform(RecyclerViewActions.scrollToLastPosition()) + .perform(RecyclerViewActions.scrollToPosition(0)) + } + + // Test selection of all entries + launchActivity().use { scenario -> + lateinit var galleryFragment: GalleryFragment + scenario.onActivity { testActivity -> + galleryFragment = GalleryFragment() + testActivity.addFragment(galleryFragment) + } + onView(isRoot()).check(matches(isDisplayed())) + + // get the RecyclerView and itemCount on the UI thread + val recyclerView = findRecyclerViewRecursively(galleryFragment.view) + ?: throw AssertionError("RecyclerView not found") + val adapterCount = recyclerView.adapter?.itemCount ?: 0 + + // Perform the view action on each adapter position (row) + for (pos in 0 until adapterCount) { + onView(isSameView(recyclerView)) + .perform(actionOnItemAtPosition(pos, longClickAllThumbnailsInRow())) + } + + val checked = galleryFragment.commonAdapter.getCheckedItems() + assertEquals(imageCount, checked.size) + } + } + + /** Recursively walk view tree to find the first RecyclerView. Runs on the same thread that calls it. */ + @Suppress("ReturnCount") + private fun findRecyclerViewRecursively(root: View?): RecyclerView? { + if (root == null) return null + if (root is RecyclerView) return root + if (root !is ViewGroup) return null + for (i in 0 until root.childCount) { + val child = root.getChildAt(i) + val found = findRecyclerViewRecursively(child) + if (found != null) return found + } + return null + } + + /** For the given row view, long-click each thumbnail inside its FrameLayouts */ + @Suppress("NestedBlockDepth") + fun longClickAllThumbnailsInRow() = object : ViewAction { + override fun getConstraints() = isDisplayed() + + override fun getDescription() = "Long-click all thumbnail ImageViews inside a GalleryRowHolder" + + override fun perform(uiController: UiController, view: View) { + if (view is ViewGroup) { + // each child of the row is a FrameLayout representing one gallery cell + for (i in 0 until view.childCount) { + val cell = view.getChildAt(i) + if (cell is FrameLayout) { + // GalleryRowHolder builds FrameLayout with children: + // 0 = shimmer, 1 = thumbnail ImageView, 2 = checkbox + val thumbnail = if (cell.childCount > 1) cell.getChildAt(1) else cell + thumbnail.performLongClick() + } + } + } + } + } + + private fun createImage(id: Long, width: Int? = null, height: Int? = null) { val defaultSize = ThumbnailsCacheManager.getThumbnailDimension().toFloat() val file = OCFile("/$id.png").apply { - fileId = id.toLong() + fileId = id remoteId = "$id" mimeType = "image/png" isPreviewAvailable = true - modificationTimestamp = (1658475504 + id.toLong()) * 1000 + modificationTimestamp = (1658475504 + id) * 1000 imageDimension = ImageDimension(width?.toFloat() ?: defaultSize, height?.toFloat() ?: defaultSize) storageManager.saveFile(this) } From b5e940d248324d6849f779c411b48213de5987b0 Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Thu, 15 Jan 2026 07:48:13 +0100 Subject: [PATCH 4/5] ThumbnailsCacheManager: fixed mutex and volatile ThumbnailsCacheManager#clearCache() needs to mutex the access, otherwise there is a race condition with initDiskCacheAsync(), as observed in a small fraction of GalleryFragmentIT tests Signed-off-by: Philipp Hasper --- .../android/datamodel/ThumbnailsCacheManager.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/owncloud/android/datamodel/ThumbnailsCacheManager.java b/app/src/main/java/com/owncloud/android/datamodel/ThumbnailsCacheManager.java index 08250553fd6c..2213a77b7439 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/ThumbnailsCacheManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/ThumbnailsCacheManager.java @@ -100,8 +100,8 @@ public final class ThumbnailsCacheManager { private static final String ETAG = "ETag"; private static final Object mThumbnailsDiskCacheLock = new Object(); - private static DiskLruImageCache mThumbnailCache; - private static boolean mThumbnailCacheStarting = true; + private static volatile DiskLruImageCache mThumbnailCache; + private static volatile boolean mThumbnailCacheStarting = true; private static final int DISK_CACHE_SIZE = 1024 * 1024 * 200; // 200MB private static final CompressFormat mCompressFormat = CompressFormat.JPEG; @@ -1243,8 +1243,12 @@ public static void generateThumbnailFromOCFile(OCFile file, User user, Context c @VisibleForTesting public static void clearCache() { - mThumbnailCache.clearCache(); - mThumbnailCache = null; + synchronized (mThumbnailsDiskCacheLock) { + if (mThumbnailCache != null) { + mThumbnailCache.clearCache(); + mThumbnailCache = null; + } + } } public static void setClient(OwnCloudClient client) { From c1f393d0b32387f48f92941de582e21a4f7aa5ec Mon Sep 17 00:00:00 2001 From: Philipp Hasper Date: Thu, 15 Jan 2026 07:50:00 +0100 Subject: [PATCH 5/5] Added LongLine as linter warning The CI will fail the build in case of long lines, so at least warn the user before. As the CI sees this as error, we might also set this as error in the IDE, but for now let's be conservative and only show a warning Signed-off-by: Philipp Hasper --- .idea/inspectionProfiles/ktlint.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/.idea/inspectionProfiles/ktlint.xml b/.idea/inspectionProfiles/ktlint.xml index 4aab2f7215ef..631c09bc2163 100644 --- a/.idea/inspectionProfiles/ktlint.xml +++ b/.idea/inspectionProfiles/ktlint.xml @@ -33,6 +33,7 @@