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

Support serial chrono order. #39

Conversation

drmikecrowe
Copy link
Contributor

@elelay -- Have a look at this. I think this is what I want, but wanted it reviewed. Naturally, you can ignore if you're not interested...

It seems that the podcast settings are cached (i.e. download_strategy), and the only way I could get the following to work was to add the sorting to podcast.update():

subscribe http://feeds.serialpodcast.org/serialpodcast
serial http://feeds.serialpodcast.org/serialpodcast
episodes http://feeds.serialpodcast.org/serialpodcast

NOTE: If you close/reopen gpo between the 2nd and third step, the episodes command works fine. Once I added the sorting to the update method, the episode order properly changes with the episodic or serial command.

Fixes #35

@Keeper-of-the-Keys
Copy link
Contributor

Wouldn't it make more sense to just allow the display of the episodes to be ordered one way or the other then to add a whole new way of importing a podcast?

@drmikecrowe
Copy link
Contributor Author

drmikecrowe commented Jun 21, 2020 via email

@Keeper-of-the-Keys
Copy link
Contributor

I'm not sure I follow, if I look at the database entries for episodes each has a publish date so it should be trivial to order the episodes that way, the only place where that would mess up is when a podcaster decides to add a episode 0 before another eposide (which in the podcasts I use seems to be very rare).

@drmikecrowe
Copy link
Contributor Author

drmikecrowe commented Jun 21, 2020 via email

@Keeper-of-the-Keys
Copy link
Contributor

Keeper-of-the-Keys commented Jun 21, 2020

But if your client is reading your local db doesn't it make more sense to just order according to the publish date field in asc/desc order as you prefer then to be?
As it stands podcasts don't necessarily go into the db in order (if for instance you raise the limit after the fact) and they are sorted during display so the only question is what do you sort by and how do you sort....

@drmikecrowe
Copy link
Contributor Author

Currently, I'm reading the DB using gpodder-core (hence the PR).

If you look at the key changes in model.py, you'll see the this is really a download-strategy change, with an additional sort happening in the update method.

Finally, I have limited my limit.expisodes to 50 (for this embedded device). When you do this with t he serial-type podcast, model.py doesn't even pull in the first episode if they have over 50 episodes...

@Keeper-of-the-Keys
Copy link
Contributor

But in that mode you will also never move beyond the first 50 episodes.

You would need to raise the download limit in all cases eventually whether from the get go to back all the way to the first episode or as you listen to more of the episodes when they are ordered in reverse.

Which is why I tend to say that this is an adding a level of complication that should be handled on the displaying end (view) and not during download.

@drmikecrowe
Copy link
Contributor Author

drmikecrowe commented Jun 22, 2020 via email

@Keeper-of-the-Keys
Copy link
Contributor

Sorry about the delayed reply, I thought I had already replied.

I see this as a display issue and not an issue that belongs in the logic for downloading episodes for 2 reasons:

  1. This is something that can be resolved strictly by setting a sort order when retrieving the episodes from the internal database.
  2. Should the download logic be inverted as you propose and the user set a limit on downloads that is smaller then the total amount of episodes in the podcast (or the podcast passes this number at some later stage) the user will never see these newer episodes unless they actively raise the download limit.

@elelay @thp your inputs on the subject are very welcome as you have been doing this much longer then me.

@Keeper-of-the-Keys
Copy link
Contributor

Keeper-of-the-Keys commented Jun 24, 2020

(While if the download order is "newest first" and the user realizes the limit is too low what they need to do is 100% clear, the scenario you propose to create creates a potential for silent and unnoticed failure)

@drmikecrowe
Copy link
Contributor Author

@Keeper-of-the-Keys, thanks for pushing back and forcing me to defend this. Actually, this PR should be changed. Here's why:

By supporting this, removes the need for my gpo changes manually configuring this. Instead, the new podcastType would:

  • Map the episodic and serial types to my current STRATEGY_CHRONO changes

I'm happy to update this PR here and submit a new one in podcastparser if you are interested.

As to why this is important, consider Up and Vanished (and pretend they have 301 episodes instead of 101):

  • Has <itunes:type>serial</itunes:type> in their RSS feed
  • Feed lists episodes [1, 2, 3, ... 301]
  • Most relevant episode for new subscribers: 1
  • gpodder-core should return episodes [1, 2, ... 200] instead of [301, 300, ... 101]. Given new users expect to start at season 1 episode 1, they could not start listening to this podcast without changing limit.expisodes. With this change, they would listen to the first 200 episodes, then have to change limit.expisodes.

@Keeper-of-the-Keys
Copy link
Contributor

Hey just a minor heads up, I'm in exam period right now so won't have time to deal with this until the end of the month.

@drmikecrowe
Copy link
Contributor Author

Closing PR (but submitting /pull/40)

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.

Support serial podcasts in chronological order
2 participants