-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[FIX] Image autoupload uploads images twice under certain conditions #4571
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
Conversation
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.
LGTM! Good job @JuancaG05 💯 Let's see if it works.
NOTE: Don't forget to add calens file and release note if this PR is going to be merged into master
branch.
(1) [FIXED]
Current: No folder is chosen, therefore no pictures are uploaded Expected: Folder chosen is set as subtitle and uploads work Pixel 7 |
(2) [FIXED]
Current: The list of spaces is the one of the first account (the one selected in step 3.) Expected: The list of spaces is the one of the second account (the one selected in step 4.) Pixel 7 |
87e6bce
to
c9139c8
Compare
@jesmrec (1) and (2) should be fixed now |
(3)After some tests, i got some duplications... this is the log if it helps somehow: owncloud.2025-04-16_08.41.04.log Samsung A8 |
@jesmrec regarding (3), those duplicates seen in logs refer to 0B files... let's see at least if worker flow is correct now (timestamp taken is correct, as seen in those logs) and maybe we can address the 0B problem in a separate issue |
d565f45
to
b20a9f3
Compare
Some conclusions:
NOTE: In my device (Samsung Galaxy A51), automatic uploads are working well and I don't have duplicated files. |
Logs over all the tests done are available, just in case |
@joragua Did the logging improvements already make it into a playstore release? I have the problems that nothing get's uploaded (well with a two days delay or so…) and I'd love to look at the logs with more information. |
9a510d8
to
b20a9f3
Compare
Hi @apollo13! No, unfortunately the verbosity added to logs is not in Play Store, and there are no plans to upload it since it's just some internal testing. But since you are interested on it, I'll provide an APK with the new verbosity. You can download it here: https://infinite.owncloud.com/s/OeffTWmcFNXtmxi (password: $xP$r58W-,PB) We would be thankful if you tell us if there is any new finding from your side! 🍻 |
@JuancaG05
Looking at the code, it is not clear to me where the worker could sleep between those timestamps. My owncloud is empty since I am currently evaluating it, there is no way it takes 6 minutes to go from updating the timestamp to redisplaying it in getFilesReadyToUpload I hope this helps, I am running a Pixel 6a with the latest Android. Happy to chat on matrix or whatever if you prefer realtime comms at some point. |
And at 13:30 still no uploads, the last try was:
Given that the next logline should be "Last Sync", this code appears to hang in android/owncloudApp/src/main/java/com/owncloud/android/workers/AutomaticUploadsWorker.kt Line 262 in b20a9f3
|
@jesmrec As mentioned two comments up, it is a Pixel 6a running latest Android (15). Anything more specific you are looking for? |
@apollo13 i have the same behaviour with Pixel 7, just to know whether the device has something to do. Because with other brands, it's working fine. |
You can reach us in https://talk.owncloud.com/channel/mobile |
Looking at some "debugging" options I think it would be a good idea to implement I'll try to get some further information via https://developer.android.com/develop/background-work/background-tasks/testing/persistent/debug |
Hi @apollo13! Thanks for your ideas! Seems that since we're using Let's try this for the moment, and if we don't get further information, we can try the custom initialization mentioned in https://developer.android.com/develop/background-work/background-tasks/testing/persistent/debug#enable-logging, which will add much more verbosity to the current logs. The new APK can be downloaded here: https://infinite.owncloud.com/s/cXkqstdShbaMevX (Password: Fja_'O188N,c) Tell us if you have any new finding, and thanks a lot for your collaboration! 🤜 🤛 |
69d4826
to
a3a846c
Compare
302b8fe
to
3f82bf9
Compare
owncloudApp/src/main/java/com/owncloud/android/workers/AutomaticUploadsWorker.kt
Show resolved
Hide resolved
10da175
to
5383f85
Compare
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.
Overall the changes make sense, left a few comments inline where I think there is room for further improvements.
One thing that still stands out though: We appear to no longer have duplicate runners because it looks as if they finish fast enough now. But it could still happen due to other factors that we cannot control. In that sense I'd prefer some defensive coding by utilizing a lock in doWork
(or whatever makes sense for the android platform) and immediately exit the worker if the lock cannot be taken (ie another worker is still running) -- plus some logging on warn or so that one could see that there is a problem early on. Exiting a work run early wouldn't be that bad I guess since the next run would pick up the files anyways (assuming you exit before updating the timestamp).
I also think/feel that my original code using the content providers still performs better. I cannot tell you why and maybe there is some magic going on behind the scenes so it performs better or maybe it was just luck during my tests…
@@ -248,10 +247,13 @@ class AutomaticUploadsWorker( | |||
val arrayOfLocalFiles = documentTree?.listFiles() ?: arrayOf() | |||
|
|||
val filteredList: List<DocumentFile> = arrayOfLocalFiles | |||
.asSequence() | |||
.filter { | |||
it.lastModified() in lastSyncTimestamp..<currentTimestamp && |
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.
I wonder if it would make more sense to move the MimetypeIconUtil
check before the lastModified
check. The rationale being that the mime type lookup is hopefully just a hash table lookup whereas the modification check has to touch the filesystem. This would save quite a bit of I/O for cases where not many files match (let's say automatic video upload and the folder containing photos & videos consists of only 1% of videos, in such a case you'd have 99% useless I/O calls -- I actually think that such a case is probably not uncommon, even if one has a 50:50 ratio of pictures vs photos you'd check half the file unnecessarily).
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.
That's right, but there are a lot of use cases... 🤔 As you said, you can have a folder in which the number of videos and photos is only a 1% and, with this filter, you will have 99% unnecessary calls. Also you can have a folder with a high number of photos and videos (99%) so, if we change the order there will be a lot of unnecessary calls anyway. In any case, @jesmrec is doing some tests so we will see what happens... But we will take into account your comments if something goes wrong. Thanks a lot for your opinion! 😄
.filter { | ||
it.lastModified() in lastSyncTimestamp..<currentTimestamp && | ||
MimetypeIconUtil.getBestMimeTypeByFilename(it.name).startsWith(syncType.prefixForType) | ||
} | ||
.sortedBy { it.lastModified() } |
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.
Any reason for sorting? Assuming lastModified
is not cached on the file object this would again result in I/O (granted only a small amount since one usually doesn't have many files in 15 minutes). Since the updated timestamp is updated independent of the actual uploads, the order of files does not matter at all.
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.
I'm not sure if it works as you comment here but it could be... 🤔 The reason for sorting is: upload the files in the same order that were taken, so we can have an "history" in the uploads section...
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.
Ah okay, if one sees that as important then ordering makes sense.
.filter { it.lastModified() >= lastSyncTimestamp } | ||
.filter { it.lastModified() < currentTimestamp } | ||
.filter { MimetypeIconUtil.getBestMimeTypeByFilename(it.name).startsWith(syncType.prefixForType) } | ||
.toList() |
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.
Not knowing kotlin at all I am curious: What is the usecase for asSequence/toList
here as opposed to the previous code that does without ?
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.
The sequence is used to load and evaluate the files individually. The list loads all the files together and then the necessary checks are performed. The final .toList()
is used to convert the sequence to a list (taking into account that the size of the list which contains the filtered files is smaller than the list of root folder)
I've done some tests with the current approach. Some conclusions:
Results were the same with @apollo13's approach. So, here it's a matter to decide which filtering algorithm fits better to our needs. |
A couple of improvements to add, let me know your ideas:
When the feature is enabled in
(the number of pictures is just an example) When the worker runs, this is the status: There are 5 uploads, because the 2 removed pictures are moved to kind of trashbin but anyway accesible with a different name, that starts with Should we prevent this kind of automatic uploads? i mean, if the file name starts with That effect was reproducible in Pixel and Xiaomi devices, but not in Samsung. |
Seems sensible, can't decide whether to filter out all hidden files or only the ones starting with |
it'd be only preventing the hidden files coming from automatic uploads, not from other types of uploads... we can maybe assume that the "regular" pictures/videos taken to upload automatically will not be hidden files, but i did not handle every existing version of every customization layer of every vendor 😄 |
yeah, hence my comment :) I would also assume that photos by default are probably not hidden. |
ok, then refusing the ones stating by It's ok to get them out if they are hidden |
Yeah, we can exclude all hidden files from the filter of the worker in this case 👍🏻 |
The fix works 👍 |
c9113ae
to
d5cd00d
Compare
Let's move this approach forward. For sure, if new improvements arise, we are open to include them after validation. Thanks everyone for your engagement, hoping the feature boosts with the news! 💯 🚀 |
Lovely thanks, is there any ETA on the 4.6 release? (Guess this has to affect quite a few users) |
not yet, but it should not go beyond the first half of July. |
Related Issues
App: #3983
ReleaseNotesViewModel.kt
creating a newReleaseNote()
with String resources (if required)QA