Repository navigation
fix(auto-upload): files skipped after re-enabling a folder or saving its settings #17906
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
base: master
Are you sure you want to change the base?
Changes from all commits
e446b12
e937c25
6758178
e209985
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,8 @@ class AutoUploadClickHandler( | |
|
|
||
| private val fileUploadHelper = FileUploadHelper.instance() | ||
|
|
||
| private val folderStateDispatcher = Dispatchers.IO.limitedParallelism(1) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to limit parallelism to 1? Also how come click handler can be triggered from multiple threads at the same time? Additionally, please remove the comment. It makes hard to follow code.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the comment in e209985. The handler is only called on the main thread. The concern is the coroutines it launches: every click starts one on
Claude Code on behalf of @Blackclaws
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The dispatcher is a field on Also Timestamp changes in |
||
|
|
||
| fun onCreateCustomFolderClick(activity: AppCompatActivity, accountName: String) { | ||
| Log_OC.d(TAG, "Show custom folder dialog") | ||
| if (!PermissionUtil.checkStoragePermission(activity)) { | ||
|
|
@@ -60,14 +62,21 @@ class AutoUploadClickHandler( | |
|
|
||
| if (item.isEnabled) { | ||
| Log_OC.d(TAG, "auto-upload configuration sync status is enabled: " + item.remotePath) | ||
| backgroundJobManager.startAutoUpload(item, overridePowerSaving = false) | ||
| fragment.lifecycleScope.launch(folderStateDispatcher) { | ||
| fileUploadHelper.clearAutoUploadScanCache(item.id) | ||
| backgroundJobManager.startAutoUpload(item, overridePowerSaving = false) | ||
| } | ||
| dialogs.showBatteryOptimizationDialogIfNeeded() | ||
| return | ||
| } | ||
|
|
||
| Log_OC.d(TAG, "auto-upload configuration sync status is disabled: " + item.remotePath) | ||
| fragment.lifecycleScope.launch(Dispatchers.IO) { | ||
| fileUploadHelper.removeEntityFromUploadEntities(item.id) | ||
| removeUploadEntities(item.id) | ||
| } | ||
|
|
||
| private fun removeUploadEntities(syncedFolderId: Long) { | ||
| fragment.lifecycleScope.launch(folderStateDispatcher) { | ||
| fileUploadHelper.removeEntityFromUploadEntities(syncedFolderId) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -135,8 +144,23 @@ class AutoUploadClickHandler( | |
| private fun updateExistingFolder(syncedFolder: SyncedFolderParcelable): Boolean { | ||
| val adapter = getAdapter() | ||
| val item = adapter.get(syncedFolder.section) ?: return false | ||
| val wasEnabled = item.isEnabled | ||
| itemFactory.updateAutoUploadFolderItem(item, syncedFolder) | ||
| persistence.saveOrUpdateAutoUploadFolder(item) | ||
|
|
||
| when { | ||
| !wasEnabled && item.isEnabled -> fragment.lifecycleScope.launch(folderStateDispatcher) { | ||
| fileUploadHelper.clearAutoUploadScanCache(item.id) | ||
| persistence.saveOrUpdateAutoUploadFolder(item) | ||
| } | ||
|
|
||
| wasEnabled && !item.isEnabled -> { | ||
| persistence.saveOrUpdateAutoUploadFolder(item) | ||
| removeUploadEntities(item.id) | ||
| } | ||
|
|
||
| else -> persistence.saveOrUpdateAutoUploadFolder(item) | ||
| } | ||
|
|
||
| adapter.notifyItemChanged(adapter.getSectionHeaderIndex(syncedFolder.section)) | ||
| return true | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| /* | ||
| * Nextcloud - Android Client | ||
| * | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
| package com.owncloud.android.ui.fragment.autoUpload.data | ||
|
|
||
| import com.nextcloud.client.core.Clock | ||
| import com.nextcloud.client.jobs.upload.FileUploadWorker | ||
| import com.nextcloud.client.preferences.SubFolderRule | ||
| import com.owncloud.android.datamodel.MediaFolder | ||
| import com.owncloud.android.datamodel.MediaFolderType | ||
| import com.owncloud.android.datamodel.SyncedFolder | ||
| import com.owncloud.android.datamodel.SyncedFolderDisplayItem | ||
| import com.owncloud.android.files.services.NameCollisionPolicy | ||
| import com.owncloud.android.ui.dialog.parcel.SyncedFolderParcelable | ||
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
|
|
||
| @Suppress("MagicNumber") | ||
| class AutoUploadDisplayItemFactoryTest { | ||
|
|
||
| private val enabledAt = 1_000_000_000_000L | ||
| private val now = 2_000_000_000_000L | ||
|
|
||
| private val clock: Clock = mockk() | ||
| private lateinit var factory: AutoUploadDisplayItemFactory | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| every { clock.currentTime } returns now | ||
| factory = AutoUploadDisplayItemFactory(clock, 4, "/InstantUpload") | ||
| } | ||
|
|
||
| private fun createItem(enabled: Boolean, timestampMs: Long = enabledAt) = SyncedFolderDisplayItem( | ||
| 1L, | ||
| "/storage/emulated/0/DCIM/Camera", | ||
| "/InstantUpload/Camera", | ||
| true, | ||
| false, | ||
| false, | ||
| false, | ||
| "account", | ||
| FileUploadWorker.LOCAL_BEHAVIOUR_FORGET, | ||
| NameCollisionPolicy.ASK_USER.serialize(), | ||
| enabled, | ||
| timestampMs, | ||
| "Camera", | ||
| MediaFolderType.CUSTOM, | ||
| false, | ||
| SubFolderRule.YEAR_MONTH, | ||
| false, | ||
| SyncedFolder.NOT_SCANNED_YET | ||
| ) | ||
|
|
||
| @Test | ||
| fun `saving settings of an enabled folder keeps its enabled timestamp`() { | ||
| val item = createItem(enabled = true) | ||
| val parcelable = SyncedFolderParcelable(item, 0).apply { isWifiOnly = false } | ||
|
|
||
| factory.updateAutoUploadFolderItem(item, parcelable) | ||
|
|
||
| assertEquals(enabledAt, item.enabledTimestampMs) | ||
| } | ||
|
|
||
| @Test | ||
| fun `enabling a folder through its settings starts a new enabled period`() { | ||
| val item = createItem(enabled = false) | ||
| val parcelable = SyncedFolderParcelable(item, 0).apply { isEnabled = true } | ||
|
|
||
| factory.updateAutoUploadFolderItem(item, parcelable) | ||
|
|
||
| assertEquals(now, item.enabledTimestampMs) | ||
| } | ||
|
|
||
| @Test | ||
| fun `disabling a folder through its settings clears its enabled timestamp`() { | ||
| val item = createItem(enabled = true) | ||
| val parcelable = SyncedFolderParcelable(item, 0).apply { isEnabled = false } | ||
|
|
||
| factory.updateAutoUploadFolderItem(item, parcelable) | ||
|
|
||
| assertEquals(SyncedFolder.EMPTY_ENABLED_TIMESTAMP_MS, item.enabledTimestampMs) | ||
| } | ||
|
|
||
| @Test | ||
| fun `display item of a stored folder keeps the stored enabled timestamp`() { | ||
| val stored = createItem(enabled = true) | ||
| val mediaFolder = MediaFolder().apply { | ||
| folderName = "Camera" | ||
| absolutePath = stored.localPath | ||
| filePaths = emptyList() | ||
| type = MediaFolderType.IMAGE | ||
| } | ||
|
|
||
| val item = factory.createAutoUploadFolder(stored, mediaFolder) | ||
|
|
||
| assertEquals(enabledAt, item.enabledTimestampMs) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6758178.
updateExistingFoldernow does the same as the list toggle: switching a folder off in its settings runs the upload entity cleanup, and switching it on clears the scan cache before saving and starting the scan. I have not added a test for the handler path, it needs the fragment and adapter; the factory part is covered byAutoUploadDisplayItemFactoryTest.Claude Code on behalf of @Blackclaws