search

OpenArchive / Save-app-ios

Secure Mobile Media Preservation
https://open-archive.org/
GNU General Public License v3.0
10 stars 3 forks source link

While uploading , when we click on Edit and Remove the other image/ video, the uploading of the same image / video starts again #202

Closed purvi-ranawat closed 2 years ago

purvi-ranawat commented 2 years ago

Steps to Reproduce :

  1. Select few images / videos and click on upload
  2. While the first image is getting uploaded, click on Edit and Remove some of the other images / videos
  3. Click on Done

Expected Result :
The first image / video which was getting uploaded should continue from where it was left after removing the other images / videos

Actual Result :

The image which was getting uploaded starts uploading from the beginning after removing the other images / videos. This way it removes whatever progress of uploading it has done before we remove the other images / videos

Attach is the video link :

https://user-images.githubusercontent.com/8190141/169581716-5ebc3cd9-6de9-4065-bc6e-f1f8db8ead7f.mp4

tladesignz commented 2 years ago

Yes. Actually, that's not a bug, but a feature.

Upload is stopped, when the "Edit" button is hit, because otherwise reordering is impossible while items continuously drop out as finished.

Stopped uploads unfortunately can not be continued, but will restart from the beginning, except when the specialized Nextcloud chunked uploading is activated.

@johnhess: We could disallow reordering any items which are currently being uploaded. Would you consider that an improvement you'd want?

johnhess commented 2 years ago

On nextcloud, if I am uploading:

and I hit edit, then swap the positions of B and C, I lose the progress I'd had on File A. This is true even when chunking uploads. I don't see why we'd want to lose progress there. I think that particular issue is our bug.

A few other thoughts:

tladesignz commented 2 years ago

Ok, seems I need to clarify a few more things here:

  1. No, you don't want to have fully uploaded items stick around forever. This is the scene which shows you the work which needs to be done. There's one exact moment in time, where things can be removed: When an upload is finished. There's no other clear point. Everything else is either a) never b) some arbitrary age threshold which takes an unnecessary amount of logic to implement and may lead to seemingly random behaviour.
  2. Please update your TestFlight to build 60 which is available since more than 24 hours. Issue #196 is fixed, there.
  3. The "Edit" button is for reordering the uploads (and deleting them, but that can also be achieved by a simple swipe. That's standard iOS list UX logic). You can not logically reorder an upload, which is currently ongoing, because it already is ongoing, therefore has position 1 implicitly. So that leads to the choice between two options: a) Stop all uploads, let the user reorder, then resume. b) Only allow reordering for positions 2 and following, while continue uploading position 1. If the user wants to have the currently ongoing upload rescheduled (because e.g. it's the biggest file and they want that to be the last), they will need to explicitly pause it, and can then reorder that, too.

Option (a) was taken, simply because it was easier to implement. I'm happy to implement option (b), if you like that better.

Nextcloud chunked uploading does have resuming, even though it might not be immediately obvious: Every chunk is checked, if it already exists and the size is correct. If so, upload is skipped and the progress indicator is forwarded. You will recognize that behaviour in bigger files on a not-blazing-fast connection.

johnhess commented 2 years ago

I think there's an option c in both lists.

In the first, "option c" could be to let fully uploaded files remain visible while the upload list view remains open and remove them upon closure. That would keep things from moving around unexpectedly and spare us things living there forever.

In point 3, "option c" could be to implicitly pause iff the list is reordered such that the item starting in position 1 is no longer in position 1. I agree "a" is simplest and probably the right call at the outset. What do you think of this option c?

tladesignz commented 2 years ago

Ok, so will be testable in the next build (63) coming up in a few hours.

There's another very good reason why it was like it was: The app crashes due to race conditions on the UITableView updates.

I forgot about that, but this rework brought it vividly back into my memory.

You can experience it with that build, if you try hard enough. The faster the connection and the smaller the images, the more likely you'll experience a crash while reordering.

I'm sorry, but, since everything is done asynchronously from multiple threads, I cannot stop this behaviour.

Unfortunately, Apple decided that it's a good idea to crash, if the UITableView gets into an inconsistent state. We can't even catch that situation and do a full reload or something.

I don't see, how this can be resolved.

@johnhess, have fun playing with it and then let's talk about what's your next best solution.

johnhess commented 2 years ago

Yeah that does throw a wrench in the works.

One option could be to leave the current one uploading (which may finish extremely quickly for small files/fast connections) but to remove others from the queue, re-adding them only when the user is done re-ordering. I'm not sure that's worth the effort though.

I think the best course of action is to put this ticket on ice until we get our UX person onboard. I'd both appreciate another brain on the design side and like to know how important re-ordering is to actual users. It's nice to be able to reorder, but I'd be surprised if it gets use outside of QA. I could imagine a handful of other approaches including simply offering "cancel" for as-yet-unstarted uploads and letting users re-add them to the bottom of the queue. It's worth noting that Dropbox, with an 8B USD market cap doesn't provide a reorder feature for its photo upload feature.

johnhess commented 2 years ago

Are there portions of these commits that make sense to keep @tladesignz or should we revert the crash-causing bit til we resolve the UX questions?

tladesignz commented 2 years ago

One option could be to leave the current one uploading (which may finish extremely quickly for small files/fast connections) but to remove others from the queue, re-adding them only when the user is done re-ordering. I'm not sure that's worth the effort though.

Ha. Yeah, that would mean less updates to the table triggered through an outside thread. Would need to try if this stops crashes. Should not be much work.

I think the best course of action is to put this ticket on ice until we get our UX person onboard. I'd both appreciate another brain on the design side and like to know how important re-ordering is to actual users. It's nice to be able to reorder, but I'd be surprised if it gets use outside of QA. I could imagine a handful of other approaches including simply offering "cancel" for as-yet-unstarted uploads and letting users re-add them to the bottom of the queue. It's worth noting that Dropbox, with an 8B USD market cap doesn't provide a reorder feature for its photo upload feature.

Well, the reordering works perfectly fine, as long as we stop the upload... I do see the use case. Sometimes you might want to get specific assets uploaded as quickly as possible, but there's a huge video in the way. Which might also not survive the 3 minutes background time. So you would be stuck looking at the app to get that out of the way.

I mean, parts of our target audience are citizen journalists under pressure. That's a slightly different group than general file sync.

Are there portions of these commits that make sense to keep @tladesignz or should we revert the crash-causing bit til we resolve the UX questions?

If I revert the continue-upload-while-reordering part, should I also revert the keep-the-uploaded-assets-around part? I don't see the use of that, then, actually.

johnhess commented 2 years ago

Would need to try if this stops crashes. Should not be much work.

Let's go that route and see if we can chalk up a W :-)

tladesignz commented 2 years ago

Would need to try if this stops crashes. Should not be much work.

Let's go that route and see if we can chalk up a W :-)

Oh, yeah. Seems, we finally got this nailed. So, continue uploading the current one seems to work without crashes

Will be contained in next TestFlight (build 64).

johnhess commented 2 years ago

fantastic!

i noticed you assigned me and purvi. is there something you're waiting on me to do?

tladesignz commented 2 years ago

i noticed you assigned me and purvi. is there something you're waiting on me to do?

No. I just thought you'd might want to test a little yourself. It seemed, this one was really close to your heart. 😜

purvi-ranawat commented 2 years ago

@johnhess It works fine now, the first file which is uploading cannot be dragged, so the process does not stop if we edit other files. I did not face any crash issue till now for build 65.