Enqueue fixes: 1) option to enqueue after currently playing 2) respect download start order

[orionlee]

Closes #2448, #2006 (I think they are the same thing)

I think #2652 can be closed as well - it has 2 separate use cases. One is addressed here, the other one is part of feed priority use cases in #2437.

closes #2283

[orionlee]

Review guide: this PR contains 2 separate but related issues. They are grouped together as they same many common codes. Main commits are:

• fc6aa31 : refactoring to make enqueue policy unit-testable
• 5dd2318 and afa89a6 : for Allow for a priority queue #2652 use case "keep in progress episode at front"
• 00d2251 : Podcast insertion order into Queue is broken (In my opinion) #2448 respect start download order in enqueueing

[orionlee]

rebased with the latest develop (v1.7.1 release candidate).

[orionlee]

The fix for respecting download start order (issue #2006) also has an unintended side effect. It fixes #1250 (Resume incomplete download) in many cases.

Specifically, for episodes that can be auto-downloaded (the default unless users change the podcast preference), with this PR, AntennaPod will automatically resume their downloads (if they have been incomplete).

Details: with this PR (fixing #2006),

1. an episode is added to the queue as soon as the download starts.
2. if the download is incomplete, the episode remains in the queue.
3. when AntennaPod's auto-download kicks-in, it will automatically continues the download of the episode, provided the episode is marked to be included in auto-download (the default). AntennaPod existing auto-download logic downloads those in the queue.

Catch:

1. The download report notification becomes a bit confusing in such cases, as the notification would show "1 downloads succeeded, 1 failed": the "1 failed" refers to the incomplete download, while the "1 succeeded" refers to the successful resume.

Users can understand what's happening if they look at the downloads log:

[orionlee]

The PR has been rebased against develop.

[orionlee]

@ByteHamster Thanks for taking the time to review the PR.

[orionlee]

Review feedback all incorporated.

[ByteHamster]

This also closes #2283

[ByteHamster]

Not sure if this is worth changing it but there is currently a difference to the version before this PR. When cancelling a download in progress, it does not get to the queue. With this PR, it stays in the queue, I think.

[orionlee]

You're correct: With this PR, an episode does stay in the queue if its download is cancelled. So is the case if the download fails.

1. Cancelled download should probably be no longer in the queue, unless the episode is in the queue to begin with before the download starts.
2. For cases download fails, staying the queue is beneficial when users dealing with unreliable internet connection: the episodes will get downloaded again automatically when auto media download kicks in.

Thoughts?

[ByteHamster]

For cases download fails, staying the queue is beneficial when users dealing with unreliable internet connection: the episodes will get downloaded again automatically when auto media download kicks in.

Good argument. Would it be possible (without too much additional code) that cancelling the download manually does not keep the item in the queue? I sometimes accidentally download the wrong episode and then cancel the download after a second.

[orionlee]

On undoing the enqueuing when user cancels a download: the change will be a tad messy, as it involves propagates the needed states across multiple layers.

Outline:

• with this PR we first enqueue the items, before doing any actual download at DBTasks layer.
• to undo the enqueuing upon cancel, the code needs to somehow remember that the item was first enqueued.
• states of downloads are not kept at DBTasks layer: they are kept at the (lower-level) DownloadService layer.
• thus, to support undo enqueuing upon cancel, we need to pass the state "the item is enqueued as part of the download" from entry point codes in DBTasks to the DownloadService

[orionlee]

(Regression fixed) Also, I realize there is an regression that first needs to be fixed: The PR as-is ignores the user settings "Enqueue Downloaded": the changed codes always enqueue them. (I totally forget there is such settings!)

[orionlee]

I believe the only open issue is when an user cancels a download, whether the item should be removed from the queue (if it's enqueued by download).

The implementation option I outlined in #2714 (comment) would be somewhat messy.

Here is a cleaner alternative, but it requires tracking of download states at the DBTasks layer, currently stateless.

• with this PR, the items are first enqueued (in a batch) at DBTasks layer before doing any actual download.
• the change to support download canceling would be:

1. DBTasks remembers the items it enqueued for download (no longer stateless)
2. DBTasks provide new entry point methods to cancel download, which will undo the enquue if needed.
3. DBTasks layer listens to the FeedItem events posted by DownloadService, to clean up its internal states (e.g., when a download finishes).

• This approach is cleaner because the enqueue / undo enqueue logic is maintained by the same layer. The downside is the stateless DBTasks will start carry some states.

Thoughts?

[ByteHamster]

What about storing the state in the downloadRequest? I did not look into this a lot but maybe the request can have a flag to remove from queue if it's cancelled.

[orionlee]

What about storing the state in the downloadRequest? I did not look into this a lot but maybe the request can have a flag to remove from queue if it's cancelled.

Storing the state in the downloadRequest is essentially the first approach in #2714 (comment)

It has its own messiness:

1. In order to pass the state to downloadRequest, the APIs of the relevant methods from the higher DBTasks layer to the lower level DownloadService all have to be changed to pass along the state.
   E.g., starting from DBTasks.downloadFeedItems():
   

   AntennaPod/core/src/main/java/de/danoeh/antennapod/core/storage/DBTasks.java

   Line 349 in 048f484

   requester.downloadMedia(context, item.getMedia());

   
   It needs to be changed to requester.downloadMedia(context, item.getMedia(), undoEnqueueIfCanceled) . Similar changes need to be done at lower levels.

2. the initial enqueue is done at the higher level DBTasks layer, but the undo will be done at the lower level DownloadService layer. It is harder to maintain as the related logic are carried out at two different layers.

[ByteHamster]

Hmm okay, you are right, that's quite a nasty change. Maybe we can just keep the episodes for now and see if users complain...

[orionlee]

Hmm okay, you are right, that's quite a nasty change. Maybe we can just keep the episodes for now and see if users complain...

I have used the feature for 1+ year, and have not really bothered by it. I did recall I had encounter it a few times (that I have to remove the canceled episode from the queue) but it didn't really register in my mind until you pointed it out. It might be my own blind spot though.

---

Another idea: at code level, the problem is that download logic is split between DBTasks and DownloadService, making the change difficult. Another approach is to move all the download logic to DownloadService, which entail:

• DownloadService exposes a batch download API (not just a single download)
• move DBTasks.downloadFeedItems() logic to DownloadService
• DownloadSevice will be able to maintain the state and undo the enqueue when needed.
• At UI layer, instead of calling DBTasks.downloadFeedItems(), it calls a new method DownloadRequester.downloadMedia(Context, FeedMedia...) , analogous to how UI logic calls DownloadRequester.cancelDownload(Context context, FeedFile) to cancel at present.

[ByteHamster]

Another idea: at code level, the problem is that download logic is split between DBTasks and DownloadService, making the change difficult. Another approach is to move all the download logic to DownloadService

That actually sounds like a good refactoring. Also makes DBTasks a bit smaller.

Just noticed another requirement that we need to keep in mind: when canceling a download but the episode was in queue already, it should not be removed.

[orionlee]

The latest commits:

• incorporate review feedback
• include some minor cleanup
• the version history is simplified (except today's commits).

From my side, the PR is ready to be merged.

[ByteHamster]

Thanks for this huge PR. Let's hope that this does not break something. It would be great if you could make a PR for removing cancelled downloads (as mentioned in #2714 (comment)) before the next release (take your time, the next release will probably not be too soon).

[orionlee]

@ByteHamster Thanks for taking the time to review / merge.

The PR for handling cancel download is there.

I also added a couple more PR / issue that are related to the use cases for this PR.

[ByteHamster]

I noticed another behavior change caused by this PR. When downloading manually from the episodes » new screen, AntennaPod previously displayed the download status and the episode went away after downloading. Now, pressing download instantly removes the episode from the new screen. In my opinion, this is quite confusing because you don't see if the download was actually started. Technically, I know that the download is started when the episode vanishes but it's still confusing to me. So it's probably even more confusing to users who have not read this PR.

[Matth7878]

I think what matters is what make an episode not new anymore and what triggers new flag removal.
From what I understood and experience :

• removing new flag manually
• adding episode to queue when settings to add to queue after downloading is on (this PR change setting to add to queue when starting to download)
  IMHO I agree with you ByteHamster. In fact new flag should be removed when a download is successful. So when starting to download, the episode is both new and in the queue.

Still what adding to queue manually should do? Right now it's considered as "add to queue and remove new flag".
I think it's fine because it's probably to stream or to manually download later. In both cases episodes are not new anymore for this use case. But should it be a setting so adding manually to queue won't remove new flag? (I guess someone would already have asked for it if it was really useful)

[ByteHamster]

AntennaPod previously displayed the download status and the episode went away after downloading. Now, pressing download instantly removes the episode from the new screen.

This behavior is fixed now.

Still what adding to queue manually should do? Right now it's considered as "add to queue and remove new flag".

I also think this is fine. The way I interpret the "new" flag is like the "unread" flag for emails. If you interact with the episode (stream, download, add to queue, etc), the flag should be removed.