Allow shares from desktop to be canceled
ClosedPublic

Authored by eduisters on Oct 28 2018, 7:08 PM.

Details

Summary

Allow in progress file transfers to be canceled

BUG: 349956

Test Plan

Send a large file from desktop to android
Press cancel in the progress notification

Result: the file transfer is cancelled and the cancelled file is deleted from storage

Diff Detail

Repository
R225 KDE Connect - Android application
Branch
arcpatch-D16491
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9343
Build 9361: arc lint + arc unit
eduisters created this revision.Oct 28 2018, 7:08 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptOct 28 2018, 7:08 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
eduisters requested review of this revision.Oct 28 2018, 7:08 PM
eduisters updated this revision to Diff 44452.Oct 29 2018, 5:19 PM
  • Removed debug logging
  • Intercept Device.SendPacketStatusCallback callbacks to prevent "Failed to send file to" notification from being shown when cancelling locally
  • Remove cancel Action when done
eduisters edited the summary of this revision. (Show Details)Oct 29 2018, 5:25 PM
nicolasfella requested changes to this revision.Oct 29 2018, 7:33 PM
nicolasfella added a subscriber: nicolasfella.

The patch doesn't apply on master (caused by D16473). Please rebase

This revision now requires changes to proceed.Oct 29 2018, 7:33 PM

BackgroundJob looks line https://developer.android.com/reference/android/os/AsyncTask to me. Can we use that instead of creating our own thing?

eduisters updated this revision to Diff 44483.Oct 30 2018, 10:09 AM

Rebased onto master

BackgroundJob looks line https://developer.android.com/reference/android/os/AsyncTask to me. Can we use that instead of creating our own thing?

You could but because there are so many issues with AsyncTask I would not recommend it (its even not recommended for running tasks that take longer than a few seconds).
Using BackgroundJob and BackgroundJobHandler also allows to start jobs from non Activities. Regarding orientation changes the same problems remain, we have to make sure that updates to the UI are done on the newly created activity/fragment not the old ones (The 2 I use only update a notification so no problems there).

Current best practices are using ViewModels that are retained and LiveData that deliver data to the activity/fragment only when the lifecycle permits it. So ViewModel starts a BackgroundJob and is guaranteed to receive the result updates the LiveData with the result and then when the Activity observing the LiveData becomes active it will receive the result.

nicolasfella requested changes to this revision.Nov 30 2018, 12:22 AM

Needs rebasing, but seems to work fine otherwise.

I don't think the desktop side would delete the partially transferred file, but that's something for another patch

This revision now requires changes to proceed.Nov 30 2018, 12:22 AM
eduisters planned changes to this revision.Nov 30 2018, 1:49 PM

This needs a bit of work because of all the changes to master

eduisters updated this revision to Diff 47681.Dec 16 2018, 4:43 PM
  • Rebased onto simplify_receiving_files
eduisters retitled this revision from Allow shares to be canceled to Allow shares from desktop to be canceled.
eduisters edited the test plan for this revision. (Show Details)
eduisters edited the test plan for this revision. (Show Details)Dec 16 2018, 4:48 PM
eduisters updated this revision to Diff 47711.Dec 17 2018, 9:44 AM
  • Removed debug log statement
eduisters planned changes to this revision.Jan 6 2019, 1:08 PM

Needs rebasing after D17627 lands

eduisters updated this revision to Diff 50384.Jan 27 2019, 3:22 PM
  • Removed debug logging
  • Intercept Device.SendPacketStatusCallback callbacks to prevent "Failed to send file to" notification from being shown when cancelling locally
  • Remove cancel Action when done
  • Rebased onto master
  • Rebased onto master
  • Delete ReceiveFileRunnable.java erroneously brought back by rebase
  • Rebased onto simplify_receiving_files
  • Removed debug log statement
  • Rebased onto master
  • Rebased onto master

ping -c2 -s3G

albertvaka accepted this revision.Mar 8 2019, 5:32 PM
albertvaka added a subscriber: albertvaka.

Tested on my OnePlus and it works well.

+1 on not using AsyncTask.

Building our own Job management system seems a bit like an overkill just for this, though, but hopefully we can use it in more places.

src/org/kde/kdeconnect/Plugins/SharePlugin/ShareNotification.java
77–93

Rename to addCancelButton or something like that? It does more than setting an id.

nicolasfella resigned from this revision.Mar 8 2019, 5:38 PM
This revision is now accepted and ready to land.Mar 8 2019, 5:38 PM
eduisters closed this revision.Mar 8 2019, 6:08 PM