PDA

View Full Version : Patch to addModeDefaultMapping



Philip Meyer
2007-12-15, 06:24
I've had a go at patching 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.

Can someone check that this code is okay? I've not got a lot of experience in this area (or Perl!).


Index: IR.pm
================================================== =================
--- IR.pm (revision 15331)
+++ IR.pm (working copy)
@@ -254,13 +254,21 @@
my ($mode, $mapRef) = @_;

if (exists $irMap{$defaultMapFile}{$mode}) {
-
- # don't overwrite existing mappings
+ my $key;
+ my $value;
+ while(($key, $value) = each(%$mapRef)) {
+ if (exists($irMap{$defaultMapFile}{$mode}->{$key})) {
+ # future enhancement - make a menu of options if a key action is duplicated
+ $log->warn("ignoring $key => $value");
+ }
+ else {
+ $irMap{$defaultMapFile}{$mode}->{$key} = $value;
+ }
+ }
return;
}

if (ref($mapRef) eq 'HASH') {
-
$irMap{$defaultMapFile}{$mode} = $mapRef;
}
}

Philip Meyer
2007-12-18, 16:52
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).

Due to this logging, I think I've discovered a SqueezeCenter bug.

I have the SavePlaylist plugin disabled. When I start my server, I can see that it still runs the initPlugin function, which tries to register 'play.hold' => 'save' within the [playlist] menu. If a plugin is disabled, should it run this code?

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 woukd at least warn if a key mapping is ignored.

Please can someone commit the patch for me (I don't think I'm allowed to do that).


Index: IR.pm
================================================== =================
--- IR.pm (revision 15439)
+++ IR.pm (working copy)
@@ -254,13 +254,27 @@
my ($mode, $mapRef) = @_;

if (exists $irMap{$defaultMapFile}{$mode}) {
-
- # don't overwrite existing mappings
+ my $key;
+ my $value;
+ while(($key, $value) = each(%$mapRef)) {
+ if (exists($irMap{$defaultMapFile}{$mode}->{$key})) {
+ # future enhancement - make a menu of options if a key action is duplicated
+ $log->warn("ignoring [$mode] $key => $value");
+ }
+ else {
+ $log->info("mapping [$mode] $key => $value");
+ $irMap{$defaultMapFile}{$mode}->{$key} = $value;
+ }
+ }
return;
}

if (ref($mapRef) eq 'HASH') {
-
+ my $key;
+ my $value;
+ while(($key, $value) = each(%$mapRef)) {
+ $log->info("mapping [$mode] $key => $value");
+ }
$irMap{$defaultMapFile}{$mode} = $mapRef;
}
}

Philip Meyer
2007-12-20, 13:07
Okay, so what happened with the term "patches welcome" ;-)

Is this that developers are busy preparing for SC7 release?

Should I file a bug report (there may be one already), and attach the patch to that, so it can be applied post SC7 release?

I'm concerned though that plugins don't gell together too well without this patch. eg. it is not possible to use PlaylistMan and TrackStat keybindings together with SC7, or users will need to override all keybindings in a custom map file. This is a change since 6.5 (I think TrackStat used to run first, and now PlaylistMan does, which means all of TrackStat key bindings are ignored).

Phil

andyg
2007-12-20, 13:20
On Dec 20, 2007, at 3:07 PM, Phil Meyer wrote:

> Okay, so what happened with the term "patches welcome" ;-)
>
> Is this that developers are busy preparing for SC7 release?
>
> Should I file a bug report (there may be one already), and attach
> the patch to that, so it can be applied post SC7 release?
>
> I'm concerned though that plugins don't gell together too well
> without this patch. eg. it is not possible to use PlaylistMan and
> TrackStat keybindings together with SC7, or users will need to
> override all keybindings in a custom map file. This is a change
> since 6.5 (I think TrackStat used to run first, and now PlaylistMan
> does, which means all of TrackStat key bindings are ignored).

Attaching the patch to a bug is the best bet for ensuring it doesn't
get lost.

erland
2007-12-20, 15:10
It should be mentioned here that the main problem is in the 'playlist' mode that is used for the "Now Playing" menu. The result of the current implementation is that only a single plugin can be accessed from the "Now Playing" menu (in the context of currently playing song) without requesting the user to mess around with custom.map files. It doesn't matter if the plugins use separate shortcut keys, with the current implementation only ONE plugin will be able to hook into the "Now Playing" menu.

IMHO, the real problem here is actually that each plugin today is required to assigned their own hotkey if they want to be accessed from "Now Playing". The result is that the user needs to remember which remote key shortcut that is connected with each plugin and we will also soon run out of remote keys to assign things to. This might be easy as long as we only talk about TrackStat and PlayListMan, but in reality there are a lot of plugins that would get easier to access if they would be possible to access from the "Now Playing" mode. Just to mentioned a few: Biography, iTunes Update, TrackStat, PlayListMan, AlbumReview and many more.

The real solution would be to implement some kind of context menu that could be launched from the "Now Playing" mode which the plugins could hook into, similar to the "mixer" functionality available in the browse menus.

This is what "step 2" described in this problem report is all about:
http://bugs.slimdevices.com/show_bug.cgi?id=5112
This is also what I requested for the Jive interface in:
http://bugs.slimdevices.com/show_bug.cgi?id=6021

I think Phil's patch (or the one I attached earlier to 5112 for the same thing) would be a great temporary improvement that should be safe to include in 7.0 or 7.1, but for the future we really need to think about some kind of context menu solution to make it easier for the end user.

Philip Meyer
2007-12-20, 17:07
>I think Phil's patch (or the one I attached earlier to 5112 for the
>same thing) would be a great temporary improvement that should be safe
>to include in 7.0 or 7.1, but for the future we really need to think
>about some kind of context menu solution to make it easier for the end
>user.

I've added my patch to your bug #5112.

I agree about a context menu, especially for Now Playing menu. It's really too much bother to navigate to the plugins menu to find a plugin that relates to the currently playing track.

Take KDF's scanner plugin, for example. This is a really good alternative for FF/REW skipping through a track. But there is no button assignment by default, so people have to navigate through the menus to find it (slow, cumbersome, not practical), or know how to enter a custom keymap to define a key to invoke it.

I envision a context menu, that would work like the mixer behaviour (press play.hold on the context of an artist, album, etc). Plugins could register action on that context menu, almost like a "Favourites" menu options.

However, this is a simple fix to the problem. The context menu can be written initially as a separate plugin (although I could see a lot of benefit with it being a facility in SC). I have always planned to write a plugin to do this, so that I can quickly access Scanner, Lyrics, Biography plugins, etc. However bug #5112 prevents it from happening!

Phil