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

Podcast Play from last position and skip back trackinfo item not functioning in LMS 8.2 #647

Closed
mw9 opened this issue Aug 8, 2021 · 10 comments · Fixed by #648
Closed

Podcast Play from last position and skip back trackinfo item not functioning in LMS 8.2 #647

mw9 opened this issue Aug 8, 2021 · 10 comments · Fixed by #648

Comments

@mw9
Copy link
Contributor

mw9 commented Aug 8, 2021

I think this may something for @philippe44 to review.

  • Play from last position is not functioning for me.

Removing these two lines from Parser.pm fixes it for me:

$item->{on_select} = 'play';
$item->{play} = Slim::Plugin::Podcast::Plugin::wrapUrl($url);

Refer to this point in the code:
https://github.com/Logitech/slimserver/blob/ecbae3cd089609a014bbfdf99550bab58d1c224e/Slim/Plugin/Podcast/Parser.pm#L84

The nearby comment suggests that image caching may have motivated the inclusion of these two lines. However the effect (on Squeezeplay based devices anyway) is that the "Play from position" sub-menu is simply ignored, because the play item takes precedence.

  • Skip back X minutes track info item is not functioning for me.

I think what's needed is an additional line ($url) = unwrapUrl($url); immediately before testing for a podcast cache item at this point in the code:
https://github.com/Logitech/slimserver/blob/ecbae3cd089609a014bbfdf99550bab58d1c224e/Slim/Plugin/Podcast/Plugin.pm#L223

This fix works for me.

The issue seems to be that podcast urls now have a podcast scheme identifier (podcast://) prefixed to them, but the keys to the podcast cache simply use the ordinary http(s):// url.

My findings stem from LMS 8.2 on macOS with Squeezeplay player. I have not researched other playback devices.

@mw9
Copy link
Contributor Author

mw9 commented Aug 8, 2021

In addition, the ability to define the "Skip back" time has been inadvertently removed from the preference settings interface. It would be good to have that back.

The relevant commit is here:

b27b207#diff-70ebc636db89c6d13868f99a582a585fbb12af3f37db5eea225439474d014953

It has removed this settings section in the process of adding the podcast provider section:

[% WRAPPER setting title="PLUGIN_PODCAST_SKIP_BACK_SECS" desc="PLUGIN_PODCAST_SKIP_BACK_DESC" %]
 		<input type="text" class="stdedit sliderInput_5_90_5" name="pref_skipSecs" id="skipSecs" value="[% prefs.pref_skipSecs %]" size="3">
[% END %]

@mw9
Copy link
Contributor Author

mw9 commented Aug 8, 2021

Minor typos in strings.txt:

PLUGIN_PODCAST_UNSUBSCRIBE
	DE	Von Podcast '%s' abmelden
	EN	Unubscribe from '%s'                  Should be: Unsubscribe from '%s'  
	FR	Se désabonner de '%s'

PLUGIN_PODCAST_DONE
	DE	Abonnement angepasst...
	EN	Subscribed/Unsubscried from show...   Should be: Subscribed/Unsubscribed from show...   
	FR	Abonné/Désabonné ...

Or, perhaps, Subscribed/Unsubscribed from podcast

@philippe44
Copy link
Contributor

philippe44 commented Aug 8, 2021

Thanks for the fixes. The problem with SqueezePlay and submenu is more complicated and might have no solution to work in all cases. If I do what you suggest, AFAIR, then either the WebUI or Material or iPeng stops working properly. Each XMLBrowser has a different behavior and so do clients. So having both the option to play from the top and drill into a submenu is problematic. To get it work on Squeezeplay, you must longpress (long enter on PC) and then you'll see the menu with "play from last position"

In general, that podcast improvement has been an excruciating pain, for little enhancement and I'm about to throw the towel on this one.

@philippe44
Copy link
Contributor

And looking at the TrackInfoMenu I don't even know why the check to cache was there at the first place

@philippe44
Copy link
Contributor

See #648

@mw9
Copy link
Contributor Author

mw9 commented Aug 8, 2021

In general, that podcast improvement has been an excruciating pain, for little enhancement and I'm about to throw the towel on this one.

I won't underestimate how hard it must have been. There's a lot of complexity and lack of clarity in the LMS code !

My initial impression is that it is a great enhancement. The ability to subscribe/unsubscribe, and search aggregators easily will be a real enhancement.

I don't know iPeng, but I'll check Material and the WebUI and report back. I thought the WebUI needed the fix too ! But maybe I was getting confused.

As regards "playing from the top", I guess you mean "playing" a podcast menu and having all the episodes play out in turn. I managed to get that working, once, on the Classic. And ended up with a playlist of about 350 items, which was no use at all ! So I've never thought it was a useful feature in practice, fine if a podcast only has a few episodes, but useless if they number in the hundreds. And many podcasts do have episodes in the low hundreds, for example BBC offerings.

Maybe you have never used the "skip back" item. It's great for when someone knocks on the door and interrupts your listening. :)

@mw9
Copy link
Contributor Author

mw9 commented Aug 8, 2021

And looking at the TrackInfoMenu I don't even know why the check to cache was there at the first place

I misunderstood you.

It was there, I think, to be sure that it was a podcast playing out, and not something else, so would only be offered up on a podcast track.

But useful for any playing track, I guess, not just podcasts.

@philippe44
Copy link
Contributor

philippe44 commented Aug 8, 2021

Sorry " playing from the top" meant "playing from the beginning" ... these ESL folks 😄

With the PR I've pushed, I think we have all we want: was working in classic & material UI, works in SqueezePlay & iPeng. FOr SqueezePlay, you just need a long press for accessing the "play from..."

The real modifications were the typos and the skipback that indeed I never used but the removal from settings was not on purpose: it's not because I don't use something that I deem it useless - I'm not there yet - 😄
But I got confused b/c the purpose of "cache" was for tracking time, a totally different intent and I missed that this menu is global although added for podcast... I always forget that. I'll update my PR then because the cache has now a different purpose than in the original plugin.

@mw9
Copy link
Contributor Author

mw9 commented Aug 8, 2021

FOr SqueezePlay, you just need a long press for accessing the "play from..."

That was what I was missing. I do see it there, now, on a Radio & a Controller. That changes my perspective...

But I got confused b/c the purpose of "cache" was for tracking time, a totally different intent and I missed that this menu is global although added for podcast... I always forget that. I'll update my PR then because the cache has now a different purpose than in the original plugin.

I think an easier way of restricting to podcasts, now, may be just to look for the podcasts:// scheme prefix, now that you've artfully put it in. No need to examine the cache at all, I'm thinking.

@philippe44
Copy link
Contributor

I think an easier way of restricting to podcasts, now, may be just to look for the podcasts:// scheme prefix, now that you've artfully put it in. No need to examine the cache at all, I'm thinking.

Yes, I've just updated the PR that way

@mherger mherger linked a pull request Aug 9, 2021 that will close this issue
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 a pull request may close this issue.

2 participants