see bug 3134.

Subject: Re:  now playing screensaver doesn't kick in if you are on another song in the current playlist

Thanks, KDF, but I'm going to overrule.   Now Playing screensaver  
should jump to the Now Playing song when it kicks in.

There's some code in Screensaver.pm at line 110:

		# we only go into screensaver mode if we've timed out
		# and we're not off or blocked
		if ($saver eq 'playlist') {
			if ($mode eq 'playlist') {
				Slim::Buttons::Playlist::jump($client);
			} else {
				Slim::Buttons::Common::pushMode($client,'playlist');
			}

That jump is supposed to make it jump to the currently playing song,  
but it's not because a few lines before we check to see if $saver  
equals $mode and we ignore if it does.  That's the broken logic.

it wasn't my design, just me bringing up the backstory. however, we do have to avoid recursion.  This is why that logic exists.  Ideally, it would be a different mode.  This would make screensaver detection FAR simpler, at the cost of complicating the detection of "showingNowPlaying" (which really does need to be looked at as far as deciding if we really do still need it).  Yes, the code sucks. But, it is all there because of requests along the way. Another possible plan is to have only the jump back on wake version of 'now playing' screensaver, though that was specifically blocked previously - determined to be redundant and confusing

would you mind having it continually jump, assuming the animation code can now make that work without restarting animation (this is really why that logic exists, bug 477)

OR do we want to detect and prevent sucessive jump() calls another way?  detecting the showingNowPlaying would work. or comparing browse index vs curent song index. (which part of what the former does)  

The same issue possibly exists for jump back on wake as it now uses playlist lines rather than client->nowPlayingModeLines [so it gets the 2 screen display] - do we need a jump in the setMode?

Is there any reason why we can't give the nowplaying screensaver a new mode name and push it on top of playlist.  We can then make the lines function only return the nowplaying screen if it is in that mode.  This would seem to avoid too much special logic in for switching into screensavers at the expense of a new screensaver mode.

the jump back on wake is 'screensaver' mode, while non-jump is 'playlist'. If you make the non-jump into a new mode, you have to make sure that you can differentiate from popping up the modestack to where you were before (jump back) and popping as if you were simply in 'playlist' mode.  Current function is that non-jump would move left into "now playing" a tthe top level no matter where you were before, and up/down lets you navigate right away.

Perhaps jump back should be the only option.  If they want playlist mode, the user can always press 'now playing'.  If navigating the current playlist, the jump back screensaver can be made to still work, and it would jump back to 'playlist' mode where the user left off.

and this plan would probably need a jump() call in ScreenSaver::setMode.

OK so using my logic, switching to non jump back on wake saver should empty the Mode stack, push 'playlist' and then push 'screensaver'.  This means the logic for leaving any screensaver is the same for all?  The lines function for playlist can then be modified to show the now playing song if in mode 'screensaver' so avoiding loosing the position in the playlist when you go into the screensaver.

I think there are two cases to the wakeup logic covered by bug 2293.  If you are on the current song and go into, lets say the news ticker as a screensaver, it shoule go back to the current song on wakeup, whatever it has become at the time.  If you are just browsing a playlist item, then it should go back to that song index. If this can be the same logic with your idea, then I think we have a winner on that point.

The rest sounds ok to me.

Dean, is that in line with your requirements?

Subject: Re:  now playing screensaver doesn't kick in if you are on another song in the current playlist

I didn't test the "jump back on wake" saver, but my expectation is  
that it jumps you to the currently playing song after the timeout.   
If you were already in the Now Playing playlist when this happened,  
when it wakes, it doesn't do anything. Otherwise it jumps you back to  
the screen you were on before.

The "Now Playing" screensaver should jump you to the current song  
when it kicks in and do nothing when it wakes.

The other screensavers should all jump you back when they wake up.

Created attachment 1519 [details]
patch for suggested logic

I've not considered all the cases, but the attached patch is the basis of my suggestion.  It may need some slightly different logic for bug 2293, also may want to define what is passed back from screensaver mode better.

can't test this right off.  HOwever, I think we'll probably still need a jump() somewhere to ensure locating on the current song.

also, showingNowPlaying already includes mode() eq 'screensaver'

Use of jump or not realy depends if we want to save the current position in a playlist when in playlist mode prior to pushing a screen saver.  I fear we are overloading too many requirement on one piece of state - the current playlist index.

Created attachment 1521 [details]
special check for 'playlist' screensaver

on top of the above patch, this adds a special case to check if the saver IS playlist, and the browsed song is NOT the current one.  This way, the screensaver handler will issue the jump() call.

It's a seriously ugly check, but it should solve this issue for now.  for 7.0 we really need to look at a "now playing" screensaver that handles it's own simple exit or jump back. seems to work for me.

actually, I'm not entirely following the specifics of the initial patch.  I think my added change may be all that is needed to solve this specific report, but if the rest is good for overall behaviour I'm into it.  it is, unfortunately, a very bad week for me to be able to focus on this stuff.  don't let me hold it back.

Agreed - I was reading it showingNowPlaying wrongly - it returns true whenever it is in 'screensaver' mode.  [didn't parse the brackets correctly!]

So the only change necessary is:
-                       $mode ne $saver &&
+                       (($mode ne $saver) || ($mode eq 'playlist' && !Slim::Buttons::Playlist::showingNowPlaying($client))) &&

So if this is agreed, I think it is a reasonably safe addition for 6.5 at this stage.

Subject: Re:  now playing screensaver doesn't kick in if you are on another song in the current playlist

sweet, go for it.

Committed to 9671

sorry, triode, this is still happening as of last night's 7.0 nightly.

does it work with 6.5 still?
I think this may have been rebroken with the param changes.
This feature is highly depenant on the browseplaylistindex, which I believe has been affected by the recent changes

Subject: Re:  now playing screensaver doesn't kick in if you are on another song in the current playlist

Alas, it appears to be broken in the 6.5 branch as well.

ah, it seems Buttons::Playlist::jump is also bypassed when the user in browsing the palylist.  once again, this points to deliberate efforst to NOT jump while browsing.  however, this can be changed, but probably should avoid changing jump at risk of buggering something else.

here is my idea:

Index: Slim/Buttons/ScreenSaver.pm
===================================================================
--- Slim/Buttons/ScreenSaver.pm (revision 9804)
+++ Slim/Buttons/ScreenSaver.pm (working copy)
@@ -113,7 +113,7 @@
                # and we're not off or blocked
                if ($saver eq 'playlist') {
                        if ($mode eq 'playlist') {
-                               Slim::Buttons::Playlist::jump($client);
+                               Slim::Buttons::Playlist::browseplaylistindex($client, Slim::Player::Source::playingSongIndex($client));
                        } else {
                                Slim::Buttons::Common::pushMode($client,'playlist');
                        }



this bypasses anything else, and directly alters the current song shown in playlist mode.

On second thought, having tested this to make sure that it works, I've committed to trunk at change 9811 and to 6.5 at change 9810.  This way it will make the nightly builds and less chance of being missed for the 6.5 release build.  Please feel free to test and verify. change it if you feel there is a better way.  I'll leave this open to make sure it is noted for verification.