Bugzilla – Bug 5112
Allow multiple plugins to register buttons in same mode
Last modified: 2008-12-18 11:12:53 UTC
It would be a good idea to allow several plugins to register buttons for the same mode. As an example several plugins wants to register buttons in the "Now Playing" menu (playlist mode), TrackStat wants to register a button for rating tracks and Playlist Mangager plugin wants to register buttons for adding tracks to playlists. An example of how a plugin register a button in the "Now Playing" mode: ---------------------- my %mapping = ( '0.hold' => 'saveRating_0', '1.hold' => 'saveRating_1', '2.hold' => 'saveRating_2', '3.hold' => 'saveRating_3', '4.hold' => 'saveRating_4', '5.hold' => 'saveRating_5', ); Slim::Hardware::IR::addModeDefaultMapping('playlist',\%mapping); my $functref = Slim::Buttons::Playlist::getFunctions(); $functref->{'saveRating'} = \&saveRatingsForCurrentlyPlaying; ---------------------- The problem is that if one plugin registers "1.hold" and another plugin registers "play.hold" for the same mode, the first one that calls addModeDefaultMapping will win and the second plugin that makes the call will be ignored. The implementation can be done in several steps where the first Step 1: Make it possible for plugins to register actions on free buttons in the same mode Step 2: Make it possible for plugins to register action on non-free buttons in the same mode, resulting in a menu when the user pressed the button. The menus show all actions registered for the particular button in the current mode. Step 3: Make it possible for the user to specify the order of the items in the menu initiated in Step 3 above. In the version version it would be enough if this was done by editing a text file, but in a future version it would be a good idea to provide some sort of user interface for this also. There has also been a discussion related to this in the following thread: http://forums.slimdevices.com/showthread.php?p=186298#post186298
Created attachment 2040 [details] Patch for step 1 Patch for step 1. The patch has been tested against 7.0, it might work against 6.5 but I haven't tested this. If a 6.5 version is desired I would be happy to make one. The patch is implemented by keeping the mapping for each mode in an array, this means that each caller of addModeDefaultMapping with the same mode will get a unique entry in this array. When a button should be looked up it searches the array from the first position and stops as soon as it finds an entry for the button. This means that if two plugins calls addModeDefault mapping for the same button in the same mode, the first caller will win.
Erland, if there is already a discussion in the developer forum regarding this, would you please post a link? Regardless, it would move this along as well if you could interest some people in voting for it. QA concerns make me reluctant to move forward on this if you're the only one interested. :)
The discussion in the Developers forum regarding this can be found found here: http://forums.slimdevices.com/showthread.php?p=186298#post186298 I've also posted a link to this bug report in that thread As far as I know, the major interest for this is users that use several plugins that registers buttons in the "Now Playing" mode. Today this happens in at least TrackStat, Playlist Manager and iTunes Update but there are probably more. The workaround today is that users using more than one of these plugins needs to create a custom.map file and the user has to know the format of this file and exactly which buttons to register for each of the plugins.
custom.map is there for this very purpose. if we want that changed, we should very seriously consider how custom.map will be handled in light of the changes. It's getting unmanageable to have so many features that have multiple points of access (multiple points of failure)
(In reply to comment #4) > custom.map is there for this very purpose. if we want that changed, we should > very seriously consider how custom.map will be handled in light of the changes. > It's getting unmanageable to have so many features that have multiple points > of access (multiple points of failure) > The main issue today as I see it is the "Now Playing" menu. It is not that unusual that a plugin likes to do something based on the currently playing song. TrackStat makes it possible to rate currently playing song, Playlist Manager makes it possible to add currently playing song to different playlists, iTunes Update also makes it possible to rate currently playing song. There are other example besides this also, some that have implemented some kind of way and some plugins that hasn't solved the problem. Due to this a plugin needs to be be able to hook in a access function in the "Now Playing" menu. Without this patch the following possibilities exist: - Use the addModeDefaultMapping function and hope that no one else has called it - Instruct the user that he needs to create a custom.map file The provided patch for Step 1 will besides the above add this: - Use the addModeDefaultMapping function and hope that no one else has registered the SAME accesss button The idea for Step 2 in the enhancement request (which has no patch yet) would be to: - Make it possible to a plugin to add an acccess method and SlimServer would provide a menu if several plugins has choosen the same access button. I have a feeling that custom.map is still needed, because it also solves a completely other issue. It makes it possible for the end user to add new access functions in various places which the plugin developer hasn't thought of. The problem with custom.map is that in the simple situation where the end user just want to install two plugin it is too hard to manually edit the custom.map. It works for advanced users, but not for users which aren't used to this kind of stuff. The addModeDefaultMapping seemed like the best way to improve the situation one step without doing a major change, but I have a feeling it would be better to make a bigger redesign if we like to take care of the whole problem. As an example the plugin today needs to call Slim::Buttons::Playlist::getFunctions() to get the function hash and add its own function there, this feels a bit dirty to me. One solution for the end user would be to have a graphical user interface for the custom.map file, where the plugins register which functions that are available and the user selects which access buttons to use for each function. On the other hand I have a feeling that most users will be happy with the default choice of access button which a plugin developer has choosen, so although a graphical user interface will solve the problem it will also make the situation more complex for most users. I'm not sure if it would be better to discuss all this in the developer forum instead ?
I too believe that SqueezeCenter code is incorrect in the handling of addModeDefaultMapping. The function exists so that key mappings can be added for menu modes, but it goes wrong (silently ignores) if the function is called more than once for the same menu mode. I have created my own patch to fix this problem, which I think is more simplistic than Erlands. I'll add it as an attachment so someone can judge what fix is the best approach. My approach merges undefined key mappings, thus still keeping the one hash of all keys for each menu mode. I've also added additional logging to indicate when requested keymappings have been registered, and when they are being ignored. Custom keymaps are additional to internal and plugin code keybindings. This gives the end-user a means to override keys, which would still be required if two plugins clash with the definition of keys for the same mode, and a user may want to assign to different keys anyway.
Created attachment 2556 [details] Patch addModeDefaultMapping prevent keybindings being ignored patches addModeDefaultMapping in IR.pm, to merge keymap items into the mode when they are not assigned, instead of throwing the whole map away if one has already been set. I've enhanced the logging a bit to report when key mapping are ignored (with warn logging level), and when key mappings are applied (with info logging level). Without my patch, the call to register the key mappings would either be ignored, or would stop any other plugin from registering any keys in the [playlist] mode. With my patch, many plugins could register different key mappings without a problem, or would at least warn if a key mapping is ignored.
Andy: can you review this patch?
Do both of these patches address the same issue? Erland, what do you think of Phil's patch?
(In reply to comment #9) > Do both of these patches address the same issue? Erland, what do you think of > Phil's patch? > Short story: ============ Both patches will solve the urgent problem. Phil's patch is simpler and should work, but isn't prepared for the future in the same way as my patch. Long story: =========== The main issue solved by both patches are: - They make it possible for two plugins to call addModeDefaultMapping with the same mode but with different buttons. This makes it possible for one plugin to register a function on the number buttons in the "Now Playing" mode and for another plugin to register an "add.hold" action in the "Now Playing" mode. The difference are: - In Phil's patch a plugin can't register a button if the user has setup a another button in a custom.map file for the same mode, this is possible with my patch. The button registered in custom.map will have priority in my plugin, so a plugin can't override a button specified in custom.map even in my patch. - The plugin really sends a reference to a hash to the addModeDefaultMapping function, so the plugin could change the hash at a later time without calling addModeDefaultMapping again, although this is probably not something you should do as a plugin developer. However, Phil's patch modifies the hash which the first plugin has sent in when the second plugin makes the call, mine doesn't. I don't think there is any real risk with this but thought it was worth mentioning. - Phil's patch doesn't allow several plugin to several plugins to register the same button in the same mode, my patch allows this. However, my patch still only uses the first registered function for the button. The reason I keep all the registrations is to allow the functionality to be extended to show a context menu in the next step. - Phil's patch is obviously the simple one and it solves the most critical problem "to make two plugins register different buttons in the "Now Playing" mode". Mine is a bit more complex so it might be a bit more risky with the advantage that it is prepared for the future. I've only tested my own patch in a real environment and I'm pretty sure Phil has done the same with his patch. The problem solved by both these patches is today a critical problem that causes support headaches for all plugins that likes to provide some function in the "Now Playing" mode, my opinion is that it is important that at least one of the patches is committed in 7.0. Assuming this is for 7.0 my suggestion is: - If you just want to solve the most urgent problem without risking anything, commit Phil's patch and do the preparation for the future in the next release. You could safely commit Phil's patch for 7.0 and investigate mine for 7.1 and later if you like to minimize the risks. - If you like to prepare for the future already now, commit my patch.
Erland has summed it up well. My patch is more simplistic - I was just trying to solve the immediate problem as it was affecting the co-habitation of many plugins. Another thing that my patch provided though was some extra logging, so it is easy to determine when plugins have registered key functions, and when they have been ignored. I think that's quite important too. I had other ideas for enhancements following this patch, which could later be incorporated, or move towards Erlands patch in the future.
I am not that familiar with the button mappings, so I would like to leave it up to you 2 guys to choose which patch gets applied for 7.0, or to work on a new patch that you are both happy with.
(In reply to comment #12) > I am not that familiar with the button mappings, so I would like to leave it up > to you 2 guys to choose which patch gets applied for 7.0, or to work on a new > patch that you are both happy with. > Andy, Phil and I have discussed this a bit and our conclusion is that it would be best to apply Phil's patch in 7.0. I suggest that we look into this further in 7.1 and later and try to make a complete solution that also involves some kind of context menu (step 2 in the description of this bug report).
Great, patch applied as change 16028, with a bit of cleanup. Marking this bug as fixed.
This bug is being closed since it was resolved for a version which is now released! Please download the new version of SqueezeCenter (formerly SlimServer) at http://www.slimdevices.com/su_downloads.html If you are still seeing this bug, please re-open it and we will consider it for a future release.