[Camera Uploads] Sync/Upload for older file, which waits for WiFi too #2679

hannesa2 · 2019-10-04T14:49:58Z

During sync, only newer files are handled, but files which were not uploaded because of missing and required WiFi are postponed currently for ever.

My mobile becomes wet and I missed a lot of pictures from the last year, because of this bug. 👎
I hope this bugfix is working like expected, please give it a try. At least my tests were positive

BUGS & IMPROVEMENTS:

(1) Double notification when uploading a file: [Camera Uploads] Sync/Upload for older file, which waits for WiFi too #2679 (comment) [FIXED WITH LATEST CHANGES]
(2) Does not retry if wifi is recovered with the app closed [Camera Uploads] Sync/Upload for older file, which waits for WiFi too #2679 (comment)

hannesa2 · 2019-10-04T15:03:34Z

Btw, I've seen sometime a lot of

W/BroadcastQueue: Background execution not allowed: receiving Intent ... ConnectivityActionReceiver

In my point of view this complete class can be removed because with Android O implicit Intents are no more working. In my tests I never run into this onReceive() and on real device I see no entry in log

davigonz · 2019-10-07T08:15:28Z

Hi @hannesa2 , thanks for this contribution, we will have a look at it.

with Android O implicit Intents are no more working

Are you sure? Implicit intents are widely used to invoke other apps on the device able to perform actions when our app is not able to perform them, I can still use them even in Android 10, e.g. opening a text editor from a file browser.

Anyway, we will recheck what ConnectivityActionReceiver is currently doing.

hannesa2 · 2019-10-08T05:03:47Z

Anyway, we will recheck what ConnectivityActionReceiver is currently doing.

Switch off/on Wifi and look for ConnectivityActionReceiver in log
I at least got no log entries with this name on my device and Emulator

pequesan · 2019-10-18T12:57:01Z

Sorry. Is there any Play Store beta program I can join to test this fix?

hannesa2 · 2019-10-18T14:19:10Z

I'm not sure if owncloud has something like this. At least I see no branch/tag upload mechanism doing it. That's why I made my own "pre-deployment" with a deterministic build. Owncloud has no such deterministic build.

If you are interested here it is https://github.com/hannesa2/owncloud-android/releases

pequesan · 2019-10-18T14:49:42Z

Thanks for pointing me to the apk. I've downloaded and installed it (I had to uninstall play store apk first), but I can't pass login page... After introducing my owncloud url and credentials, app crashes and any further try to open it crashes directly.

Any idea?

Can I install debug apk and provide you any log?

hannesa2 · 2019-10-19T06:06:00Z

@pequesan hannesa2#3

jesmrec · 2019-10-22T11:45:18Z

Sorry. Is there any Play Store beta program I can join to test this fix?

ownCloud Android app is suscribed to the beta program of App Store. Right now, we do not have an active beta (latest release is an official one). If you are interested in beta testing, please suscribe the beta program of Google Play, and you will be notified in he moment we release a new one.

jesmrec · 2019-10-29T17:19:35Z

I started to handle this, and some questions about the problem come to my head:

You reproduce the problem with camera uploads, am i wrong? since "only wifi" are available only for camera uploads
You lost pics that were taken under mobile connection, and were not uploaded even when you recovered the wifi. Is this correct? that means, the pics are queued in Uploads but not retried.

Does your fix make the pending stuff to be retried? @hannesa2

hannesa2 · 2019-10-29T21:43:45Z

@jesmrec all right !
I fixed a follow up crash with missing forground permission too (Android O)

pequesan · 2019-10-30T07:50:15Z

I can confirm I'm using @hannesa2 release with no further issues. All my files are being uploaded, with only wifi checked.

Well, I must say something weird happens when owncloud is uploading a file and you lose wifi... but I assume it should be treated as a separate issue.

jesmrec · 2019-10-30T08:13:10Z

ok, we will move it forward in terms of code review & QA (CC @abelgardep @davigonz)

Well, I must say something weird happens when owncloud is uploading a file and you lose wifi... but I assume it should be treated as a separate issue.

yes, please. Create a new issue including as many details as posible.

Thanks a lot!!

pequesan · 2019-10-30T08:25:08Z

I will do as soon as I have those details. I must see if I can replicate it each time, or it happens only sometimes.

hannesa2 · 2019-10-30T21:45:05Z

@pequesan you should see the reason here #2440 (comment)

pequesan · 2019-10-31T08:12:28Z

Thanks. But I haven't been able to reproduce the issue... I tried with some files, but rock solid at the moment, which is good news. :)

I'm running Owncloud hannesa2 apk in both my wife and my Google Pixel at home, so I'll be able to catch the problem, if any.

pequesan · 2019-10-31T22:07:03Z

Sorry to use this issue to ask this but... Where should I add a feature request? I'm not familiar with Github, sorry.

hannesa2 · 2019-10-31T22:37:49Z

@pequesan What's about to simply create an issue ?

abelgardep

``

abelgardep · 2019-11-04T07:27:26Z

owncloudApp/src/main/java/com/owncloud/android/files/services/FileUploader.java

@@ -290,8 +289,7 @@ public int onStartCommand(Intent intent, int flags, int startId) {
                KEY_REQUESTED_FROM_WIFI_BACK_EVENT, false
        );

-        if ((isCameraUploadFile || isAvailableOfflineFile || isRequestedFromWifiBackEvent) &&
-                Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {


This check was included to fix a problem with notifications here: #2453 (comment) . But if everything is working, we can move this forward @jesmrec

If we'd remove that line, the problem you mention will arise again, causing a regression. We must avoid this. CC @hannesa2

@abelgardep , @jesmrec This if block is just to

startForeground(141, mNotificationBuilder.build());

nothing else. And because of this if condition it was crashing. This startForeground() needs nothing else

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {

@hannesa2

We were checking isCameraUploadFile || isAvailableOfflineFile || isRequestedFromWifiBackEvent conditions to execute startForeground(141, mNotificationBuilder.build()) because it needs to be called within five seconds after calling startForegroundService from TransferRequester for those cases (cameraUploads, available offline and requestdFromWifiBackEvent), otherwise, the app will crash.

But you're also right, we should call startForeground() after calling startService() from TransferRequester to make FileUploader service run in foreground. Otherwise, the app could fail if the system decides to kill FileUploader (it does not usually happen though). Could you tell me in which scenario is the previous code crashing for you?

What really concerns me most is the duplicated notification that appears when trying your solution, and we had to fix it in #2453 (comment) . Besides, we are also able to show a notification even when calling startService() in a normal upload. It seems that many changes have happened here in the Android code itself.

Could you have a look at the problem with duplicated notifications that seems to happen when applying your changes? Thanks

pequesan · 2019-11-05T09:35:00Z

Well, FYI I'm running @hannesa2 apk with this fix, and I would say that I don't have double notifications. But It's true that from Android 9, notification system has changed a lot. I'm using Android 10 in a Pixel 4.

hannesa2 · 2019-11-06T06:44:02Z

Like @pequesan already wrote, I never observed a double notification too.
What's about @jesmrec ?

@davigonz
To much if-conditions for startForeground() is simply wrong. I run into crashes when I do it !
If you ever observed too much notifications (I didn't) you have to rework the notification-channel. This is out of this PR

Summarize:
You have to choose between a crash-free and working synchronization with Wifi-on/off (this is a primary function) or not-oberserved double notification prevention.

@davigonz
#2453 (comment) where is the fix ? Please point me to the commit, not to the release notes.

davigonz · 2019-11-06T08:01:20Z

@hannesa2

Too much if-conditions for startForeground() is simply wrong. I run into crashes when I do it !

Ok, you talked about those crashes before and I asked you the steps to reproduce, could you give me those steps? Thanks

not-observed double notification

I've just tried this PR code and reproduced the double notification in a Google Pixel 2 with Android 10

where is the fix ? Please point me to the commit, not to the release notes.

f09525e

If you ever observed too much notifications (I didn't) you have to rework the notification-channel.

The problem with notifications is something that appears with your solution (this PR), I tried it in master and there's no additional notification. As we usually do with the rest of contributions and even with our own PRs, if something is broken, it should be fixed by the developer who included the new code. Otherwise, all the developers would have to finish the code developed by other people.

Anyway, I can help you if there's any blocker.

davigonz · 2019-11-06T08:20:36Z

[FIXED] Double notification when uploading a file

Upload file
Swipe down the top of the screen to see the notifications bar, two notifications appear:

Notification with upload progress
Empty notification

Current: two notifications for uploads
Expected: show only one notification

Pixel 2 Android 10
Server 10.0.10

pequesan · 2019-11-06T09:07:51Z

Wow. I've just tested to upload a file with @hannesa2 apk, and I must admit there's those two notifications noted by @davigonz. As I'm using owncloud app mainly for camera uploads, I never take a look at notifications behaviour in detail. So I can confirm that bug.

jesmrec · 2019-11-13T07:42:35Z

Since the double notification and the lost uploads are different features, we should keep the checks

if ((isCameraUploadFile || isAvailableOfflineFile || isRequestedFromWifiBackEvent) &&...

in order to avoid regressions, and testing separately both things: this PR for the camera upload stuff, and the other thing, wherever you want (new issue or PR).

Agreed? @abelgardep @davigonz @hannesa2

hannesa2 · 2019-11-13T07:44:52Z

Since the double notification and the lost uploads are different features, we should keep the checks

@jesmrec Sorry, it will crash, when we keep these if conditions #2679 (comment)

jesmrec · 2019-11-13T08:33:54Z

@hannesa2 steps to reproduce the crash? android version?

davigonz · 2019-11-13T10:10:46Z

I've been diving in the uploads and camera uploads code and it seems there was a case missing in FileUploader.java for scheduling automatic retries for uploads that should wait for wifi to be uploaded when the user selects Only wifi. We were only scheduling automatic retries for uploads that were already launched and failed because lack of wifi connection, which is not the same.

I've just pushed a commit with that missing case so we don't need to retry all the failed uploads from CameraUploadsSyncJobService.java as @hannesa2 was doing before. Now is more specific, we just schedule the retry for the camera upload that was waiting for wifi. I also removed the if that was triggering the double notification.

The automatic retries of failed uploads are handled by RetryUploadJobService.java but only work for Lollipop devices or above, the same task is handled by ConnectivityActionReceiver.java for devices below Lollipop. That's why you do not see logs there @hannesa2 , your device will be >= Lollipop for sure. It would be great to reopen the discussion about what to do with < Lollipop support, since 4.4 was released in 2013.

@jesmrec this is ready to QA, can you please recheck if everything work as expected and see if the crashes that @hannesa2 says appear? I'm not able to reproduce any crash with the latest changes I pushed

hannesa2 · 2019-11-13T12:01:45Z

I see, I've to make a complete report for each detected crash.

Reintroduce if ((isCameraUploadFile || isAvailableOfflineFile || isRequestedFromWifiBackEvent)
make a picture without Wifi, after 10 minutes switch on Wifi. After additional 5 minutes it tries to sync.

--> You will see probable a crash because of missing foreground-something

jesmrec · 2019-11-13T13:18:05Z

thanks @hannesa2. This is the same when you go to the doctor... telling him "i feel bad" is not enough. He needs to know where does it hurt ;)

jesmrec · 2019-11-15T11:06:05Z

QA.

Catching events of wifi connection to upload from the camera:

Roaming from mobile connection (app foreground)
Roaming from mobile connection (app closed)
Switching from no connection (app foreground)
Switching from no connection (app closed)
Upload with no roaming (regression)

jesmrec · 2019-11-15T11:13:24Z

(2)

Look at these steps:

Mobile data plan (no wifi) connection (disable the wifi option in the device)
Camera uploads with "only wifi" enabled
Take some pictures -> Pictures queued in Uploads view, waiting for wifi
Kill the app
Recover wifi (enable it on device settings)
Open the app, now under wifi connection

Current: pending uploads are not retried till you open the uploads view and retry them manually.
Expected: retried since wifi is recovered

Huawei P20Lite & Xiaomi MiA2
Android 9

jesmrec · 2019-11-15T11:53:58Z

I also notice ANR events when camera uploads take some time (not when only one upload is done). This is also reproducible in 2.13.1, so i will open a new issue with this -> #2724

jesmrec · 2019-11-21T09:15:52Z

About report (2): i noticed different behaviours in different devices, so it needs more info and research. Since this problem is not a new one (no regression), i will open in a new issue as soon as i gather more info about it.

In the same path, i will open an issue with #2679 (comment) if i reproduce it.

But this PR fixes at least the retry problem under "only wifi". So, we can move forward.

pequesan · 2019-11-27T16:17:29Z

Does this "merge" means that next official release will address this delayed wifi upload issue? Did you manage to fix it while not reintroducing the double notification glitch?

davigonz · 2019-11-27T17:56:36Z

Does this "merge" means that next official release will address this delayed wifi upload issue? Did you manage to fix it while not reintroducing the double notification glitch?

@pequesan Yes to both questions

pequesan · 2019-11-27T18:07:08Z

That's great! Thx

hannesa2 requested a review from davigonz October 4, 2019 14:50

This was referenced Oct 4, 2019

Sync of offline made pictures does not work #2460

Closed

CameraUpload no longer works #2573

Closed

Kotlinize #2680

Closed

jesmrec added this to the 2.14.0 milestone Oct 8, 2019

hannesa2 force-pushed the AttemptToFixUpload branch from 720ec29 to 221d246 Compare October 29, 2019 21:41

jesmrec requested a review from abelgardep October 30, 2019 08:13

abelgardep approved these changes Nov 4, 2019

View reviewed changes

jesmrec added Camera Uploads Estimation - 2 (S) labels Nov 5, 2019

davigonz removed the Camera Uploads label Nov 6, 2019

davigonz added the sync::camera_uploads label Nov 6, 2019

davigonz changed the title ~~Sync/Upload for older file, which waits for WiFi too~~ [Camera Uploads] Sync/Upload for older file, which waits for WiFi too Nov 6, 2019

davigonz added the Sprint label Nov 7, 2019

jesmrec mentioned this pull request Nov 15, 2019

Automatic picture upload not starting if "only on wifi" unchecked #2237

Open

jesmrec added the Approved by qa label Nov 21, 2019

hannesa2 and others added 3 commits November 21, 2019 12:19

Sync waiting older files too

506176b

start FileUploader in forground without too much if conditions

fb804b7

Add new case in FileUploader to retry uploads delayed for wifi

6b516b3

davigonz force-pushed the AttemptToFixUpload branch from 159ebc4 to 6b516b3 Compare November 21, 2019 11:20

davigonz merged commit 9d123cc into master Nov 21, 2019

delete-merged-branch bot deleted the AttemptToFixUpload branch November 21, 2019 12:43

jesmrec removed the Sprint label Nov 22, 2019

hannesa2 mentioned this pull request Dec 22, 2019

Ability to change sync start date to the past #2772

Closed

[Camera Uploads] Sync/Upload for older file, which waits for WiFi too #2679

[Camera Uploads] Sync/Upload for older file, which waits for WiFi too #2679

Conversation

hannesa2 commented Oct 4, 2019 • edited by jesmrec Loading

BUGS & IMPROVEMENTS:

hannesa2 commented Oct 4, 2019

davigonz commented Oct 7, 2019 • edited Loading

hannesa2 commented Oct 8, 2019

pequesan commented Oct 18, 2019

hannesa2 commented Oct 18, 2019

pequesan commented Oct 18, 2019 • edited Loading

hannesa2 commented Oct 19, 2019

jesmrec commented Oct 22, 2019

jesmrec commented Oct 29, 2019

hannesa2 commented Oct 29, 2019

pequesan commented Oct 30, 2019

jesmrec commented Oct 30, 2019

pequesan commented Oct 30, 2019

hannesa2 commented Oct 30, 2019

pequesan commented Oct 31, 2019

pequesan commented Oct 31, 2019

hannesa2 commented Oct 31, 2019 • edited Loading

abelgardep left a comment

Choose a reason for hiding this comment

abelgardep Nov 4, 2019

Choose a reason for hiding this comment

jesmrec Nov 4, 2019

Choose a reason for hiding this comment

hannesa2 Nov 4, 2019

Choose a reason for hiding this comment

davigonz Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

pequesan commented Nov 5, 2019

hannesa2 commented Nov 6, 2019 • edited Loading

davigonz commented Nov 6, 2019 • edited Loading

davigonz commented Nov 6, 2019 • edited Loading

pequesan commented Nov 6, 2019 • edited Loading

jesmrec commented Nov 13, 2019

hannesa2 commented Nov 13, 2019 • edited Loading

jesmrec commented Nov 13, 2019 • edited Loading

davigonz commented Nov 13, 2019 • edited Loading

hannesa2 commented Nov 13, 2019

jesmrec commented Nov 13, 2019

jesmrec commented Nov 15, 2019 • edited Loading

jesmrec commented Nov 15, 2019 • edited Loading

(2)

jesmrec commented Nov 15, 2019 • edited Loading

jesmrec commented Nov 21, 2019

pequesan commented Nov 27, 2019 • edited Loading

davigonz commented Nov 27, 2019 • edited Loading

pequesan commented Nov 27, 2019

hannesa2 commented Oct 4, 2019 •

edited by jesmrec

Loading

davigonz commented Oct 7, 2019 •

edited

Loading

pequesan commented Oct 18, 2019 •

edited

Loading

hannesa2 commented Oct 31, 2019 •

edited

Loading

davigonz Nov 5, 2019 •

edited

Loading

hannesa2 commented Nov 6, 2019 •

edited

Loading

davigonz commented Nov 6, 2019 •

edited

Loading

davigonz commented Nov 6, 2019 •

edited

Loading

pequesan commented Nov 6, 2019 •

edited

Loading

hannesa2 commented Nov 13, 2019 •

edited

Loading

jesmrec commented Nov 13, 2019 •

edited

Loading

davigonz commented Nov 13, 2019 •

edited

Loading

jesmrec commented Nov 15, 2019 •

edited

Loading

jesmrec commented Nov 15, 2019 •

edited

Loading

jesmrec commented Nov 15, 2019 •

edited

Loading

pequesan commented Nov 27, 2019 •

edited

Loading

davigonz commented Nov 27, 2019 •

edited

Loading