Marc: was this connected to SN or SC?
if SC, which version?

SC 7.3 - just reproduced it with Version: 7.3.1 - 24288 @ Fri Dec 12 03:16:19 PST 2008

Also, the tracks are long enough that the track I'm trying to zap is the same one that was playing when I sync'd.  I have no idea if this matters.

Does the same thing happen when with non-synced players?

Good call!

If I simply go to a music folder, select a track, press play, wait for screensaver to kick in I see the same failure.  Nothing to do with sync.

Also, its not just the first try!  Several tries in a row with the screensaver running all failed.

Sorry for the poor initial diagnosis.

This is happening when the 'screen saver' is active.  Holding + to initiate a zap does nothing while the screen saver is active.

Change the screen saver "while playing" to something other then 'Now Playing' and the same behavior is present.

The first key event will get you out of the screen saver, the second key event will initiate the 'zap' behavior.

Because of the nature of the Now Playing screen saver, this will cause a false failure in the expected behavior of 'zap'.

We need to fix this by allowing ZAP during a Screen Saver event. Which we currently do not.

KDF: Dean would like you to look at fixing this for the Now Playing screen saver.  Make it an 8.0 release fix.

ZAP should work when Now Playing SS is active.

I suspect this is another side-effect of the new code lookup path via screensavers and INPUT.List. I have neither time, nor motivation to trace through the new IR handling so pointers are welcome.

on the subject of screensavers, you'll find a lot of things don't work.

I don't think its anything to do with the input list change - add.hold is explictly mapped to "done" in the Default map for each screensaver - this was added in change 23008.

One of the bugs associated with those changes were to stop "play" causing a play action during a screensaver.  But I can't see a reference in the public viewcv to a reason for doing it for zap.

Suggest chaning the IR/Default.map for add.hold to "done_passback" for the screensavers if you want this function.

done_passback doesn't work.  The lookup finds 'zap' in common mode, but the getFunction doesn't seem to do the same search pattern, fails to find the 'zap' function from playlist mode.  In some cases, it's looking under "home" mode.  If in playlist mode before the screensaver, it fails to even find zap as a function at all.  

Ideally, done_passback is the correct mapping.  It's the lookup that is not consistently matched through the modestack.

changing add.hold in Default.map under [screensaver] to done_passsback makes no difference for me.

changing it to zap doesn't either.

Sounds like it needs more work ... I'll try to look at it sometime.

OK I understand why this is happening - its because a screensaver is not playlist mode.  At present zap is in the default mapping, but is only defined as a function for playlist mode.  So passback for a screensaver goes to the previous mode and not playlist mode.  Also the mode matched in the irlookup is not retained for the getFunction lookup - in short its all a mess...

Personal view is if we want zap to work for all modes then it should be defined in S:B:Common - this would fix this case.  Is this what is wanted?

Kdf can you describe other cases where "screensavers don't work"

All the debug in IR.pm is far too confusing as there are two passes through execute but the same debugs are deplayed each time with different data... :(

exactly.  The mode names are now altered, which means a custom function for a mode has to be defined specifically for the altered mode whenever this type of handling is used. The problem with moving zap to common is that it will now operate as a fallback in ANY mode, when what we want is only when a currently playing or song from the current playlist is shown.  If moved to common, you'll need to check the state before acting and this would prevent other functions from being used for play.hold in "common" mode.

As for other issues, I'd have to list them as I find them again.  I've noticed times where actions don't work initially or causes log warnings due to undefined functions.  Just looking at how this case has been changed due to the fallback path says that there will be other "gotcha" cases.  I suspect most other cases are plugin related (most authors being unable to keep pace with changing APIs).

So my point is that zap would have worked for "Now Playing" screensaver as that was mode playlist, but not for any of the other screensavers such as visualisers as these were not playlist mode.

Do we want something which works for all?  If so giving them a common mode name is ok and we should just define the button mapping in screensaver and playlist and have the function available in common (the function lookup is dumb, but the button look up is mode aware so we are better off using that..)

Let me post a patch so you can see if you agree...

Created attachment 4533 [details]
proposed patch

Proposed patch which allows zapping from a now playing screensaver and playlist.

I must admit that having press and hold do zap, but not on the home menu where a simple press of add on the "Now Playing" entry causes the playlist to be cleared is some confusion - surely the UI was not designed like this?

Tried to install you patch by hand in Version: 7.3.2 - 24443 and got:

[08-12-31 15:04:39.4967] Slim::Control::Request::execute (1889) Error: While trying to run function coderef [Slim::Control::Commands::buttonCommand]: [Undefined subroutine &Slim::Buttons::Common::catfile called at /usr/share/perl5/Slim/Buttons/Common.pm line 1229.
]
[08-12-31 15:04:48.0080] Slim::Control::Request::execute (1889) Error: While trying to run function coderef [Slim::Control::Commands::buttonCommand]: [Undefined subroutine &Slim::Buttons::Common::catfile called at /usr/share/perl5/Slim/Buttons/Common.pm line 1229.
]

and zap does not work at all.

hacked at your Common.pm and got it to work.  My perl is too weak to know if all the use statements I added are needed or not.

42,45d41
< use File::Spec::Functions qw(:ALL);
< use File::Spec::Functions qw(updir);
< use Slim::Control::Request;
< use Slim::Buttons::Playlist;
1230,1246d1225
< 
< 		'zap' => sub {
< 			my $client = shift;
< 			my $zapped = catfile($prefs->get('playlistdir'), $client->string('ZAPPED_SONGS') . '.m3u');
< 
< 			if (Slim::Player::Playlist::count($client) > 0) {
< 
< 				$client->showBriefly( {
< 					'line' => [ $client->string('ZAPPING_FROM_PLAYLIST'), Slim::Music::Info::standardTitle($client,
< 						Slim::Player::Playlist::song($client, Slim::Buttons::Playlist::browseplaylistindex($client))) ]
< 				}, {
< 					'firstline' => 1
< 				}); 
< 
< 				$client->execute(["playlist", "zap", Slim::Buttons::Playlist::browseplaylistindex($client)]);
< 			}
< 		},

All that is needed for this patch to work is to add the following to Common.pm:
use File::Spec::Functions qw(catdir);

The add.hold for zap is intended to only be active while in a "playlist" mode.  The feature on the home menu was removed at one time, but a few users complained that they wanted it back as it was the only way to simply empty the playlist.  

Admittedly, it is counter-intuitive but it's been there a very long time.  So far, no other suggestions have come up for a more sensible way to manage all of these features in a completely natural way (aside from perhaps bringing up the context menu idea again)

As this is targetted for 8.0, is that meaning wait until there is an 8.0 trunk or would it be ok to drop the current patch (with the additional line in Common.pm) to 7.4 trunk?

Don't think it needs the exta use if you apply the patch - it gets rid of the catdir.  It works for me on 7.3 and I was planning to apply it there if you can verify it works ok.

Ah yes, so you have.  Sorry, I was reading via email so only saw Marc's patch in full.  It works for me on 7.4.  I don't have a checkout of 7.3 any more as it became a bit of a nuisance to run multiple copies

committed in change 24460

I can confirm that the fix works for me.  I don't know if its appropriate for me to change to verified, so I didn't.

Thanks.

This bug has been marked as fixed in the 7.4.0 release version of SqueezeBox Server!
    * SqueezeCenter: 28672
    * Squeezebox 2 and 3: 130
    * Transporter: 80
    * Receiver: 65
    * Boom: 50
    * Controller: 7790
    * Radio: 7790  

Please see the Release Notes for all the details: http://wiki.slimdevices.com/index.php/Release_Notes

If you haven't already, please download and install the new version from http://www.logitechsqueezebox.com/support/download-squeezebox-server.html

If you are still experiencing this problem, feel free to reopen the bug with your new comments and we'll have another look.