Bugzilla – Bug 8871
Implement settings for new alarm functionality
Last modified: 2009-07-31 10:25:28 UTC
The controller needs to support the new alarm functionality, probably similar to the new web interface. Please note that the alarm volume set up may change. At the moment the user can choose a default volume for all alarms and can then override this with a specific volume per alarm. This may become a simple setting per alarm to use the current volume or a specific volume.
heh...Max, been meaning to talk to you about this for days. What would be your interest in doing the code for this? Feel free to say "none", but if you're game, I can give you lots of support-- nobody knows this particular part of SC better than I. The code will be 100% on the SqueezeCenter/Perl side, all delivered from Slim::Control::Jive assigning to myself for now, pending Max's thoughts on writing the code. BTW, love the new alarm code.
the alarm cli command has not been functioning for me, and I had to do the following modification to get it to work: was: addDispatch(['alarm'], [1, 0, 1, \&Slim::Control::Commands::alarmCommand]); now: addDispatch(['alarm', '_cmd'], [1, 0, 1, \&Slim::Control::Commands::alarmCommand]); so now the "command" is sent as a non-tagged parameter (this is the only way, grrr, I could get it to work). Possible values for _cmd are add, update, delete, enableall, and disableall. I've updated the cli-api.html to reflect this change. none of this is checked in yet, but Max wanted to keep you abreast of what's going on.
Michael is a good person to talk to about the alarm CLI stuff. He made the necessary changes in order to implement the web ui. I'm happy to let you make any necessary changes, but thanks for keeping me in the loop. I'll keep reading the bugmail! The web ui for alarms is obviously a good point of reference for how to do some things on the controller. Thanks, Ben!
> addDispatch(['alarm', '_cmd'], [1, 0, 1, > \&Slim::Control::Commands::alarmCommand]); > > so now the "command" is sent as a non-tagged parameter (this is the only way, > grrr, I could get it to work). Possible values for _cmd are add, update, > delete, enableall, and disableall. Ben - how do you set one specific alarm's volume? I wonder whether we could leave the old commands in (for backwards compatibility, emulating enableall etc.), while creating a new command, which better fits the new model where we have to handle x alarms. Could you please attach your patch to this bug, so I can have an idea of what you've been doing?
Looking at the code I'm seeing two issues: - there are some notifications registered. When not using the _cmd thingy, request will check against a list of sub commands. It seems that if notifications are registered, the request would expect one of those as commands too, or fail if no match is found. If you disable those notifier registrations, the command will work - the sanity check for the parameters in alarmCommand() is either broken or I didn't understand what was possible. It wouldn't allow for disabling an alarm with "alarm id:2349 enabled:0". Disabling that check helped. I'm not sure about the notifiers. We might need to register all possible commands like with eg. the playlist command.
Michael, my though on adding the _cmd to the alarm command was that Slim::Control::Commands::alarmCommand would still be the function that would handle this command, just that the first argument to the command would be _cmd, which is mandatory. After that, it still should accept tagged params. in other words, going from this: <playerid> alarm <taggedParameters> to this: <playerid> alarm <add|update|delete|enableall|disableall> <taggedParameters> while it would have been nicer to do it the way it was originally coded, nothing I tried would make the correct subroutine (alarmCommand) fire. IMHO, it may be a bug in the addDispatch() code, particularly with commands with 1, 0, 1 arguments (requires client, not a query, has tags). I have a vague remembrance of having a similar head scratcher when adding some jive-specific favorites commands in 7.0.1. In summary, still one command and one subroutine used to manipulate alarms, but there is a required _cmd parameter before the tagged params. This should still work for all required actions on this command. For example, if you want to set a alarm 4's volume to 60: <playerid> alarm update vol:60 id:4 make sense? Michael, on your last comment, I'm really in the dark as to what request notifiers are all about. Let's try to have a discussion about that before week's end (sorry, was out this a.m., which is prime time to reach my Swiss colleagues)
Created attachment 3711 [details] alarmCommand that has both legacy support for only tagged params, and supports new "_cmd" keyword Michael, this is just an FYI following up on our earlier phone discussion re: legacy support for this command. This seems to work like a charm... Request.pm has one command registered, ['alarm', '_cmd']. alarmCommand() in Commands.pm looks for a colon in the _cmd param and parses it as needed. All params are now stored in a $params hashref, which is better than a scattering of scalars anyway... I'm going to move forward with this unless you have objections. Thanks for the assistance.
Looking good!
Bug 8954 has changed the player and web ui for alarms, I'm afraid. There is no longer a per-alarm volume setting, simply a master volume for all alarms. The only needed changes should be to remove per alarm volume controls and the use default volume setting, and to change the default volume setting to use the strings ALARM_VOLUME and ALARM_VOLUME_DESC. Under the hood the per-alarm volume stuff is still there. This change simply relies on alarms defaulting to use the default alarm volume, which they do.
Thanks for the update Max, I hadn't been tracking that bug... Good timing-- this actually makes my job a bit easier, and I was just getting to this part of it today.
not sure if it matters any more since Boom's been leaked, but Boom branch change 22425 and change 22426 changes the All Alarms item to an on/off choice rather than a checkbox. This change will only work if against a controller running >= r2788, as there was a jive-side change to fix this.
By creating a separate cli command for updating the alarm sound, nextWindow params, 'refresh' for "use current playlist" and 'refreshOrigin' for all others), sent with radio and checkbox items now work fine. "Alarm Sound" menu now has submenus as of boom branch change 22428 with that, I think this is feature complete.
looks like a bunch of fixed 7.2 bugs were erroneously changed to target milestone of '---'. Changing them back, as they should appear in searches as to where they were fixed.
Controller alarm matches Boom new alarms, this looks really nice. 7.2 - 23472
This bug has been fixed in the 7.3.0 release version of SqueezeCenter! Please download the new version from http://www.slimdevices.com/su_downloads.html if you haven't already. If you are still experiencing this problem, feel free to reopen the bug with your new comments and we'll have another look.
Reduce number of active targets for SC