Skip to content
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

Add new episodes to UpNext and trigger download after (not before) sync #3211

Open
1 task done
stic opened this issue Nov 11, 2024 · 7 comments · May be fixed by #3212
Open
1 task done

Add new episodes to UpNext and trigger download after (not before) sync #3211

stic opened this issue Nov 11, 2024 · 7 comments · May be fixed by #3212
Labels
[Area] Up Next [Type] Enhancement Improve an existing feature.

Comments

@stic
Copy link

stic commented Nov 11, 2024

Description

Having two phones (devices probably too?) I have the first is setup to keep adding to UpNext (on top, and last, with 20 episodes limit), whilst I occasionally tick off the list items listening on the second device as well as listening to new episodes as these catch my attention (some are setup for UpNext on the 1st device).
On refresh (background disabled) I see update of podcast happening first, so episodes already listen to on 2nd device get added to UpNext (sometimes removing from the back of the list), manage to download, only to disappear a while later when sync happens.

I'd like to improve behaviour by adding to UpNext after the sync (which might actually bring some elements already)

Screenshots or screen recording

No response

Did you search for existing list?

  • I have searched for existing issues.
@stic stic added the [Type] Enhancement Improve an existing feature. label Nov 11, 2024
stic added a commit to stic/pocket-casts-android that referenced this issue Nov 11, 2024
@ashiagr
Copy link
Contributor

ashiagr commented Nov 12, 2024

@stic 👋

I browsed through your PR and appreciate your approach to prevent adding new episodes (that have already been finished on another device) to up next before the sync process is complete.

Is it also to prevent auto-downloading of such episodes? It appears that episodes are added to auto-download queue only after the sync is complete:

val episodeUuidsAdded = updatePodcasts(result)
val syncRefreshState = sync()
if (!emptyResponse) {
var startTime = SystemClock.elapsedRealtime()
episodeManager.checkForEpisodesToAutoArchive(playbackManager, podcastManager)
LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Refresh - checkForEpisodesToAutoArchive - ${String.format("%d ms", SystemClock.elapsedRealtime() - startTime)}")
startTime = SystemClock.elapsedRealtime()
podcastManager.checkForUnusedPodcasts(playbackManager)
LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Refresh - checkForUnusedPodcasts - ${String.format("%d ms", SystemClock.elapsedRealtime() - startTime)}")
startTime = SystemClock.elapsedRealtime()
playlistManager.checkForEpisodesToDownload(episodeManager, playbackManager)

I'm just trying to weigh in on any side effects, possibly up next not getting refreshed instantly for most users 🤔

@stic
Copy link
Author

stic commented Nov 12, 2024

Thanks for reviewing the PR @ashiagr
The auto-download seems to be internally checked for archived episodes, so as it happens post sync() new updates shouldn't be unnecessarily downloaded.

Indeed, a user needs to wait for sync to finish before items are added to UpNext. Personally, I'd rather have that to happen (small delay) than see episodes come and disappear (after all, the global state of me is that I've listened to these already 😉).

It's a quick fix approach, as I'm new to the project, I'm yet to see previous attempts at rationalising update & sync.
I'd suggest syncing first, building list of episodes that aren't know yet (pre-update). Then update with latest list of podcast (some might have been deleted, some added). And put back episodes where these should be.
The major issue I couldn't resolve quickly with such approach is to fetch these 'unknow' episodes on UpNext, history etc. I've come up with 'isSyncing' state, but gave up after about 7 files of changes trying to reflect new state throughout the app 😬

@CookieyedCodes
Copy link

@stic Do you think it will fix my issue #628

@stic
Copy link
Author

stic commented Nov 12, 2024

@stic Do you think it will fix my issue #628

Nope @CookieyedCodes as that seems to be to order of items synced, if archived sync before starred and there is some dependency down the chain it would need a separate fix. Will have a look if no one picked that issue and it's possible to replicate (personally don't I don't star too much so haven't seen it).

Side note, is there a common approach to complex scenarios testing like this? Looked around but haven't seen anything taking sync for spin?

@CookieyedCodes
Copy link

Ya fair enough, the sync is more so on the iOS side of things anyways,

I think most people don't touch sync because they don't want to upset users 😅

@stic
Copy link
Author

stic commented Nov 12, 2024

Ya fair enough, the sync is more so on the iOS side of things anyways,
I think most people don't touch sync because they don't want to upset users 😅
It's rather that the code is quite complicated (vs. complex) in this area, let me take my findings to appropriate issue ok?

@CookieyedCodes
Copy link

Hmmm I might suggest you put it under a toggle under advanced settings then because if it does make an issue it minimizes the risk to people who do want it 🤔🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Area] Up Next [Type] Enhancement Improve an existing feature.
Projects
None yet
4 participants