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

Refresh the podcast on Podcast Details page creation #51

Merged
merged 3 commits into from Jul 18, 2021
Merged

Refresh the podcast on Podcast Details page creation #51

merged 3 commits into from Jul 18, 2021

Conversation

ademar111190
Copy link
Contributor

Add a minor convenience to the user, whenever the user opens the podcast detail it silently updates the list of episodes.

Based on this suggestion: breez/breezmobile#456

How it looks like

auto-refresh.mp4

@amugofjava
Copy link
Owner

Hi @ademar111190,

Thanks for the pull request. Background/auto refresh is something I am looking into. There are a couple of issues with the podcast load routine which has stopped me calling _handleRefresh() during page load.

  1. If you try to perform a load when you are offline or have no connection an error message is displayed. Calling refresh on page load when offline causes the error message to be displayed at the start of the episode list which looks a little odd (see screenshot). When performing a silent load it also needs to silently fail.
  2. The load routine cannot be cancelled once started. This means that if you go into one podcast and the load starts, then you go to another podcast before the first has finished loading the first set of episodes returned will be displayed in the second podcast until the second load has completed (see video).

Both issues shouldn't be too hard to resolve, but as it stands I feel calling refresh on load may give a poorer user experience than having to refresh manually in the short term.

refresh.mp4

@ademar111190
Copy link
Contributor Author

Thank you for the feedback @amugofjava

I'll think in something that updates without those undesirables side effects.

@kingonly
Copy link

@amugofjava this is something Breez users keep straggling with. I get feedback on this issue on a daily basis.
Both issues you've raised are valid, but if we could fix #2 and keep the manual refresh in can of no connectivity, I think it's (by far) a better UX than what we have today.

@roeierez
Copy link
Contributor

@ademar111190 @amugofjava
AFAICT the podcast_bloc is responsible for loading the podcast and updating the UI via the streams here: https://github.com/amugofjava/anytime_podcast_player/blob/master/lib/bloc/podcast/podcast_bloc.dart#L103
By the time the podcastService.loadPodcast is finished the new episodes are saved safely in the database without affective the UI. The following lines are responsible for updating the streams with the podcast and new episodes so in order to prevent wrong UI update we can check if the current podcast (in the _podcastStream) is the one that we are refreshing, if not just skip updating the streams.
will that solve the seconds issue?

For the first issue we can pass a flag (background: true) to the Feed entity when calling handleRefresh. The podcast_bloc will fail silently in case of errors (network error, etc...). I think it will solve the first issue.

LMK your thoughts.

@amugofjava
Copy link
Owner

Hi @roeierez,

Yes, that is exactly my thinking for both points. This seems to be the simplest solution and shouldn't be difficult to implement. In addition, I am also thinking of adding settings to set the background refresh to be Off, On (always), On (Wifi-Only). I will also add a cache time - i.e. don't bother refreshing again if you did so only 5 minutes ago; podcasts won't necessarily update that frequently. If I make all these options configurable settings then you can use or ignore them how to feel best suits Breez.

@ademar111190
Copy link
Contributor Author

Hi, thanks for the feedback and the insights.

I think I did solve both issues with my last changes. May you guys can check it one more time?

I was thinking about some settings too, it doesn't seem to be difficult, but I don't have much clue about how to design it properly and keep a good experience for the user.

@ademar111190 ademar111190 changed the title Call the refresh method when Podcast Details is created Refresh the podcast on Podcast Details page creation Jun 28, 2021
@roeierez
Copy link
Contributor

I think I did solve both issues with my last changes. May you guys can check it one more time?

Look good to me.

@amugofjava
Copy link
Owner

Thanks @ademar111190, this looks good to me too. I will add the settings around your changes and push the update to master. Thanks for your PR.

@amugofjava
Copy link
Owner

I have found one small issue @ademar111190 in that because the load process is now being called twice in the initState, if you load a podcast by URL (i.e. not one already saved in the database) it results in the RSS feed being downloaded and processed twice. I'll work on resolving that.

@amugofjava
Copy link
Owner

This is the current state of the auto-refresh. It needs finalising and thorough testing, but I think this solution will work. The stuttery-ness of the video is not a reflection on Anytime, just the poor recording ability of the emulator running a debug build.

auto_refresh.mp4

@kingonly
Copy link

kingonly commented Jul 9, 2021

@amugofjava great UI. One suggestion: add a spinner on top of the podcast image while new episodes are loading so people understand something is going on.

@amugofjava amugofjava merged commit 7fba07e into amugofjava:master Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants