Support for Plugin Client Menus

[aesculus]

I notice in Material Skin that my plugin is supported in the Player Settings, but only the service side settings.

How can I get the plugins player client menus to be supported in Material Skin?

https://github.com/aesculus/denonavpcontrol/wiki/How-to-Use#operation-with-the-client-applications

[CDrummond]

Sorry, but I will not add support for such actions under a 'My Music' section - that's just awful UI, IMVHO. Mixing browsing for music along with control actions does not make sense to me. Material also does not change 'My Music' options when the player changes - that's another behaviour that I think makes sense for an on-device UI, but not for a multi-device controller (Material, iPeng, etc).

There's a few ways this could be accomplished though:

1. A user can create their own actions.json file to add custom actions to do this - see https://github.com/CDrummond/lms-material/wiki/07-Customisation
2. You could make the relevant page accessible in the 'Extras' LMS menu - Material will show this
3. You could turn the menu into an 'Apps' plugin - similar to 'Music Walk With Me'
4. I could expand Material to allow plugins to register actions to be shown per-player. This would be similar to 1 (above) but would not require any intervention by the user.

My preference would be for option 4 - and I already have similar code to allow plugins to add actions to list responses. Now this would take some extra code, but (again INVHO) this would lead to a better UX.

[aesculus]

Thanks for the rapid response. I am totally for any implementation that does not require extra user interaction to install.

Today for iPeng and Squeezer the plugins client player menus area assigned to one of their menus via these calls:

Slim::Control::Jive::registerPluginMenu(\@menu, 'settingsPlayer' ,$client); Slim::Control::Jive::registerPluginMenu(\@menu2, 'settings' ,$client);

Perhaps something similar needs to take place for Material once you decide where it would best reside.

Note that these menus are used dynamically during playback, while the server side menu you are currently exposing of the plugin is the server side menu which is done for installation and rare adjustments to the installation of the players implementation of the plugin. So two different menus with distinctly different purposes and interaction.

[CDrummond]

I have no Denon AVP/AVR but I have installed your plugin, and enabled it for a Squeezelite player and restarted LMS. What am I supposed to see? Tried with default skin and SqueezeCtrl. All I can see from your linked page is the "Turn on Family Room" - is this correct? But I see those even for player's where the plugin is not enabled.

If this menu entry is just an on/off toggle - then why no intercept the power on/off JSONRPC commands?

[CDrummond]

OK, I hacked the Plugin.pm file and I now see entries under "Settings" However, this does not appear on the Default skin, only Squeezer/SqueezeCtrl.

To be honest, I'm not sure there is much that can be done. I'm not going to add a "Settings" section to the main page - would not fit. To me this belongs in either 'Extras' or in the LMS player settings.

[aesculus]

OK. I was in the middle of responding: Without a Denon AVR all you would get is a menu line item and no menu with it's items. The plugin dynamically creates/tailors the menus based on state, one of which is if the Denon is on or not.

You are correct that we only have it for the apps Squeezer, SqueezeCtr and iPeng and it does not show up in the default skin.

We are pretty flexible on where it appears. Since it is used during music playback wherever that makes sense in MS is appropriate. We were limited to where it could appear in the apps.

Just for your info to gain context: These controls influence how the amplifier tailors the music during playback. Such things as filters, channel volumes, acoustic enhancements etc. So often people will tweak these settings on a track by track, but certainly album by album basis. So they are certainly not set and forget types of actions. Not really appropriate for Settings, but that is where the apps providers wanted to put them.

[aesculus]

Craig: It sounds like you were able to overcome the plugin not building the menus because you do not have a Denon amp. If you need me to, I can attach a plugin.pm file that bypasses that check and makes the menu and items for you to test with.

[CDrummond]

@aesculus Yeah, that'd great thanks.

[aesculus]

Here is what I would like for you to test if you can. You probably don't have a Denon or Marrantz AVR so we will have to hack the code in order for you to show the menus. • Install V4.5 of the DenonAvpControl plugin via the Plugins tab in the Settings of the LMS • It should prompt you to restart LMS • Go to a player in Settings and choose the DenonAVP/AVRControl pulldown on the player • Enter a fake ip address on your network since it won't find an AVR • Make sure Main zone is selected • Select: Enable audio settings menus • Make sure the Enable Plugin is selected. • Select Apply • Copy the attached Plugin.pm file onto the LMS server to replace the standard file you installed above • Go to where the plugin is installed and edit/view Plugin.pm • Look for line 333 that says 'settingsPlayer'. This is the menu and will be looking for a place in your code to set the menu. We can of course come up with a new one for MS. Note there is also settings since iPeng and Squeezer liked different places • I commented out lines 394-403 so the plugin won’t try to look for an amplifier • Now restart LMS for the new file to take affect • Once LMS is running load MS and turn on the selected player the plugin is assigned to • You should see DenonAVP/AVRControl in the list in Settings or wherever you placed it, or at least now you have something to try to place. You will probably get all sorts of errors from the plugin because there is no response to various requests but I am hoping this will not interfere with the test to load the plugin menu.

Rename this Plugin.pm after download Plugin.pm.txt

[CDrummond]

Hmmm... This uses radio buttons, etc, which Material does not support from LMS's SlimBrowse API. Therefore, Material cannot support this as is.

The only way to support this would be:

1. Add some Javascript to your plugin to implement a Material dialog. This dialog would then have all the controls, etc.
2. Update your plugin to register this dialog with Material
3. Update Material to configure how this dialog is invoked

I'm guessing the dialog could be accessed from the player's context menu in 'Manage players' or via a button in the 'Other settings' section of 'Settings/Player'

I can help with the Javascript for the dialog. But does the above make sense? Do those places seem like reasonable and usable locations?

So, at startup your plugin would call something like:

if ( Slim::Utils::PluginManager->isEnabled('Plugins::MaterialSkin::Plugin') ) {
        my $rc = eval {
            require Plugins::MaterialSkin::Extensions;
            Plugins::MaterialSkin::Extensions::addJavascript("plugins/DenonAvpControl/html/js/denonavpcontrol.js");
            Plugins::MaterialSkin::Extensions::addTranslation("plugins/DenonAvpControl/html/lang/");
            Plugins::MaterialSkin::Extensions::addDialog("denonavpcontrol");

            foreach my $id @denon_player {
                Plugins::MaterialSkin::Extensions::addAction($id, "icon", string(ACTION_TITLE), "javascript:\"bus.$emit('dlg.open', 'denonavpcontrol'\"");
            }
            1;
        };
    }

What this does is:

1. Register 'denonavpcontrol.js' with Material. This is the Javascript implementing the dialog.
2. If you have translations for this dialog they should be in JSON files in DenonAvpControl/html/lang/
3. Tells Material to create a place holder for the dialog
4. Foreach denon player, add an action to open the dalog.

I'll work on this more later. Just my initial thoughts.

[aesculus]

OK. But I am not following how I would link the logic from the plugin to the new UI built under Material. Maybe it's possible but currently out of my sphere. Will need to investigate this.

[CDrummond]

I'll play with this later, perhaps over the weekend. Basically my idea is that a plugin can add "custom actions" to Material as per "actions.json" (described in https://github.com/CDrummond/lms-material/wiki/07-Customisation) - this is what "Plugins::MaterialSkin::Extensions::addAction" is doing.

So your plugin would add a "Denon Controls" (or whateve text you want) action that when clicked by the user would emit a signal to open the "denonavpcontrol" dialog.

denonavpcontrol.js would be a Javascript file containing all the code of the control dialog; reading current state, sending commands to update, etc. I'll create the initial javascript file for you.

However, seeing as I'm not a user of this plugin I wasn't sure if this would be the best way to achieve this or not. As in would your users be OK with "Settings" -> "Player" -> "Denon Controls" -> show dialog. Is that too many steps? If so, where else would be better?

[aesculus]

Craig: Thank you for taking this on. My original intent was for you to be able to simply guide me to where to assign my plugins menu with maybe a bit of icon tweaking like was done with the other apps. But that appears to be not possible. Your plugin is written in HTML, JS and JSON while mine is in LMS CLI, PERL and the UI support coming from exposing this in the native clients. So at first glance the two look completely incompatible.

Lets take a pause until I can sort all of this out a bit. I don't want you to kill yourself trying to wedge something incompatible into your excellent offering, for what is probably a handful of cross compatible users.

Once I discover the options or barriers, I'll post back here. Then we can decide the best course of action for both of us.

[michaelherger]

Hi @CDrummond - @aesculus asked me to jump in here. But I think you're pretty much on top of it already: the plugin does register menu items for the Settings menu - which you don't want to implement. I think there's not much I can add. The other controller apps @aesculus mentioned would work "out of the box" because they implement that menu.

I understand that in your POV it doesn't make sense. But it's a fact that there are quite a few plugins out there which do this, because it's the way Squeezeplay (and therefore the original devices) works. So for a developer it's hard (to say the least) when you have to implement specific solutions for each UI. It would therefore be great if you could re-consider your decision. Those other developers are as much volunteers as you are. And they try to minimise supporting the corner case as much as you are.

[CDrummond]

@michaelherger I have no intention of add a "Settings" entry to the main screen, it makes no sense with the rest of the items. I could move these items into "Extras", as that would be a (IMVHO) a reasonable place. Or even add entries to the player settings dialog. However, the main issue is that Material does not implement 100% of SlimBrowse, so does not handle things like radio buttons, number input, etc. Adding this would be a lot of work - for a use case I do not have. I realise others work on this in the free time, and have not asked anyone to do anything - I volunteered to make the changes to Material and then implement the plugin side.

[aesculus]

Craig: Really appreciate your desire to accommodate this fringe case.

If you would like to continue this discussion outside of Git you can email me at

[CDrummond]

@aesculus I have no issue with discussions here. There really is not much to discuss. Material does not implement the parts of SlimBrowse required for this. I have no objection to this being implemented, I just have had no need for it so far so never got round to implementing it. Plus, I don't think it fits very well. Hence my original idea to allow plugins to provide dialogs. But perhaps that is too Material-specific. And I agree its bad when a plugin needs to adapt itself to the UI implementation - which could later be changed, or someone could implement a better version.

[aesculus]

Craig: I am speaking from a technical weak point here, hence my request of @michaelherger to comment technical wise. The two architectures are so different I believe it would require me to completely rewrite my plugin since I know of no way to separate the server/amp interaction logic from the UI portion. Heck I have no clue what you need to do to map my plugins UI to your platform. Clearly it's not just "slipping me in" to a menu, or providing me a variable to park myself in a logical spot.

I am sure we could find a place to park it that would work. I would think Player Settings > Other Settings would be great but even where it's at now (replacing the plugins static settings menu) would be acceptable.

So I think the hard part, which we can wait for, would be the effort for you to support radio controls, if that truly is the only barrier. But again I have no clue what effort you need to go through to support plugins like mine in Material.

[michaelherger]

I understand that some features often used in the Settings section of Squeezeplay are not implemented in Material. Thus it would be difficult to render your menus, even if that section was available.

LMS basically has three UIs: old two-line style player menus, Squeezeplay, and the included web UIs. Many controller apps, including Material at least partially implement the Squeezeplay interface. Your plugin implements a Squeezeplay menu which is not supported in Material. This issue can only resolved by either @CDrummond implementing support for those menus, or @aesculus implementing a different UI, which Material would be using. Eg. implementing another web UI to be hooked into the "Extras" menu.

[CDrummond]

I know its been a while, but I have started work on this in the 2.10 branch. This seems to work OK for the modified Plugin.pm that is attached to this issue. Does this plugin have an text input fields? As these are not currently handled.

If you try to test the 2.10 branch, then this Denon plugin should appear at the end of the Player settings dialog.

[aesculus]

Hi Craig: While we have made some changes to the plugin since opening this issue I will test it against the version posted and also against our current set.

In the player menu's there are only sliders, dropdowns, radios and checkboxes.

Thanks for working on this.

[CDrummond]

Where would the dropdowns and checkboxes be? The Plugin.pm you attached only allowed me to see sliders and radios.

[aesculus]

Woops. Sorry for the confusion. Only sliders and radios for your client.

The menu and radios look great, but we are not seeing the sliders themselves, but just the values:

What do you see when you select the Channel Level menu item?

[SamInPgh]

Hi Craig. I'm the one doing the testing so maybe it's better if I jump in here. When I touch anywhere in the box directly below any of the slider's descriptive text, which is where the slider control should be displayed, the slider variable is set to a value of 0 and the descriptive text is updated accordingly due to the "nextWindow=>'refresh'" attribute for the control in the menu. Here is what the menu looks like in the Squeezer Android app. I hope this is helpful.

[CDrummond]

Please update and try again. I used Squeezer and SqueezeCtrl to see how they handled this. For the only example of sliders that I had, they both used dialogs for this. So I mistakenly assumed that was the way to go. But I've changed the code to draw the sliders in the list.

[SamInPgh]

That did it! The slider controls look great and perform flawlessly except for one anomaly (see below). Maybe you can share what you know with the Squeeze Ctrl developer. We had to disable the Channel Level Adjustment menu when using that app because the sliders are not displayed or handled correctly there at all.

The only problem is a minor one, and I suspect it is in LMS. If, when the menu is first displayed, the user presses the '+' icon to increase the value by 1, an incorrect, seemingly random, value is passed to the handler. The strange thing is that it only happens with '+' and not '-' and it has to be the very first adjustment made to the slider. If, for example, '-' is pressed first and then '+', all is well. As I said, it is strange. Btw, Squeezer doesn't support the '+/-' slider buttons at all, so this is the first time I have been able to use them and they are very useful. I'm not sure if iPeng supports them as I have no Apple devices.

One other problem I observed was that the plugin's menu is not displayed if the "Player settings" menu is reached from the "Manage players" drop down menu. It shows up in the settings but selecting it does nothing. I'm sure it is a minor issue.

Thanks for implementing this, Craig. Now the plugin's AVR Settings menu can be used from a browser menu on any platform for the first time --- something I'm sure our users will be very happy to learn. Great job!

[CDrummond]

Both issues should now be resolved. The "+" issue I think is because LMS sometimes sends an integer value as a string - I have seen "initial":"5","adjust":1

If the above is now working, does that mean I can now close this?

[CDrummond]

Also, if you want to make your icons fit better with Material you can use the in-built icon mapping.

e.g.

• Rename volume.png to MTL_icon_volume_up.png will cause Material to use the "volume_up" icon from the Material font (see Material Icons Font for a list of symbols). The icon PNG file can be named <anything>MTL_icon_<Material icon name>.<extension> - and Material will use the text after MTL_icon_ to get the symbol name).
• Rename room_eq.png to room_eq_svg.png and then also ship room_eq.svg Material will then load room_eq.svg when it sees room_eq_svg.png - and it will recolour to match the other icons. The svg should use #000 for all fill and stroke colours - as this is what Material will replace with the actual colour.

Refer to Plugin-icon-mapping for more info.

---

One other point, the plugin is appearing as "DenonAvpAvrControl" which, IMHO, looks a bit odd. Why not "Denon AVP/AVR Control" - e.g. fix capitalisation and add spaces.

[SamInPgh]

Both issues are indeed resolved, Craig. On behalf of Chris and myself, many thanks for your help and cooperation in taking this on. As I said previously, I feel that it adds a lot of value to our plugin and maybe a little bit to yours too. In fact, I think we should both raise our prices! 😉

Thanks also for the info on MS icon mapping capability. We will be looking into using it in the future.

Regarding the name of the plugin menu, it was changed in the last release at my behest. Here is what it looks like now in the Player settings menu:

Thanks again. As far as I'm concerned, this issue can now be closed.

P.S. Material Skin is my new favorite LMS client.

[SamInPgh]

I forgot to mention one thing. Our plugin uses the showBriefly() client function to display some non-critical informational popup messages in the menu. They work as expected in the Squeezer and Squeeze Ctrl apps but do not show up in iPeng or Material Skin. Here is an example of one:

# Do nothing if plugin is disabled for this client
if ( !defined( $pluginEnabled) || $pluginEnabled == 0 || $audioEnabled == 0 ) {
    $log->debug( "Plugin/menu Not Enabled: skipping menu for " . $client->name() . "\n");
    my $message = $client->string('PLUGIN_DENONAVPCONTROL_MENU_NOT_ACTIVE') . " ..."; 
    $client->showBriefly( { 'jive' => { 'text' => [ $message ], } },{'duration' => 2, 'scroll' => 1, 'block' => 1 } );
    $request->setStatusDone();
    return;
}

Any advice you might have for getting popup messages such as this to display in MS would be much appreciated. Thanks.

-Sam

[CDrummond]

Messages should now be displayed.

There is one issue though, and this also affects Squeezer. if your plugin is not enabled for a player it still appears in that player's menu, just when clicked an empty page is shown (Squeezer seems to just spin). If your plugin is not enabled for a player, surely it should not appear in the menu?

The command I use to get the menu items is:

curl 'http://localhost:9000/jsonrpc.js' --data-raw '{"id":0,"method":"slim.request","params":["01:02:03:04:05:06",["menu","items",0,25000,"direct:1"]]}'

I then filter items based upon node=="settingsPlayer"

But for enabled, and disabled, players the node for your plugin is the same:

{"id":"pluginDenonAvpControl","actions":{"go":{"cmd":["avpTop"],"player":0}},"menuIcon":"plugins/DenonAvpControl/html/images/denon_control.png","text":"DenonAvpAvrControl","weight":9,"node":"settingsPlayer"}

[CDrummond]

Not sure how this could be achieved, but it'd be great if the menu listing command above could return some descriptive text. Then I could show this using light-grey text as per "Extra settings", etc.

[SamInPgh]

You have identified an ongoing thorn in our side regarding the plugin menu, Craig, and I believe you may have also identified a solution. The primary reason for using showBriefly() was to inform the user when the plugin menu was invoked for a player not using the plugin. However, I just realized that we might be able to make use of the 'player' menu attribute that is currently set to a value of 0 for all instances of the menu (there is one instance per plugin-controlled player). I would like to try setting it to a value that can be added as an additional filter in your code, such as $client or $client->name(). The doc isn't very clear on the use of the 'player' attribute and all of the examples show "player => 0", but I'd like to give it a try if you think it makes sense. Let me know what you think. I may be missing something here. It's happened before... ;-)

[CDrummond]

Are you stating that you want the client (e.g. Material) to filter on this attribute? Which I can do, just not sure if that'd break other plugins. (Are there others using 'playerSettings' ???)

I thought when ["menu","items",0,25000,"direct:1"] is sent to LMS it returned the menu items for a specific player - hence each player can have a different set of browse modes. Can your plugin not exclude itself here?

[CDrummond]

..also I have merged the 2.10 branch into master - so the next release will contain this code.

[SamInPgh]

I agree it makes sense that LMS would only return menu items for the current player. However, that doesn't seem to the case. The menu is only being registered for players using the plugin. Any others are ignored. I know for sure that this is the case. Here is the statement for players using the plugin:

Slim::Control::Jive::registerPluginMenu(\@menu, 'settingsPlayer' ,$client); 

As far as impacting other plugins, you would have to filter on the 'text' attribute as well in order to be completely safe. A slightly less safe option that would avoid special code for our plugin would be to filter on "player == $client" only if it isn't == 0, since it is extremely likely that the other plugins do set it to 0. It's possible, however, that all of this would just be a workaround for a bug in LMS. I don't know.

[SamInPgh]

Btw, thanks for all your work and cooperation on this integration. Chris and I really appreciate it.

[CDrummond]

Looking at Slim/Control/Jive.pm (e.g. line 285) it would appear that the list of plugin menus is not client specific. Perhaps you should raise an issue on LMS' github page?

I can add filtering on player. e.g. if player is a MAC address then only use if matches current player id. But are you sure you can change this player per player? i.e. have you tried my curl command on different players with your plugin setting player to the MAC address?

[SamInPgh]

No. I haven't tried it and I don't know for sure that whatever value I set 'player' to will be preserved. I was going to use the address of the current client object ($client), which is how the plugin identifies a unique player. Do you have access to that variable at the appropriate point in your code? If not, I can try using the MAC address (once I figure out how to locate it).

[CDrummond]

$client->id is the player's ID - and, usually, this is the player's MAC address. I have access to the player ID, as that is what I send when making the call to get the menus. So, when I was referring to MAC address it was client ID that I really meant.

[SamInPgh]

Okay. I'll use that. There is also a $client->macaddress.

[CDrummond]

Not sure that will actually work. I hacked your Plugin.pm to set player => $client->id I then have 2 squeezelite instances with different MACs, one has denon enabled, the other not. The curl command posted above returns "player":"<mac of enabled player>" for BOTH players :(

As stated, I think for LMS there is only 1 copy of this menu.

[SamInPgh]

But isn't that the point? Can't you bypass creating the menu for the player whose id(mac) doesn't match the one returned in the curl command? Or am I missing something yet again?

[SamInPgh]

The next question for me is 'what happens when there are two players using the plugin'? My hope is that LMS creates a separate copy of the menu for each of them, based on the separate calls to registerPluginMenu().

[CDrummond]

But if you have 3 players, 2 supporting 1 not - then all three return the same "player":"mac" value. So, this filtering would only work if the plugin was only ever for 1 player. And to me, that's not very generic.

I really think the solution is to have LMS only return this item for players that support it.

[SamInPgh]

I agree on the ultimate solution being in LMS. However, do you know for sure that a second plugin player won't create an additional menu structure with a different "player":"mac" value? Have you tested with two players that use the plugin?

[aesculus]

The issue I think is that a player is a bit subjective. Physical players and I think MS, look like real players to LMS.

We won't even get started talking about how terrible the menus for our plugin are in the SB Touch UI for example. Disaster!

Apps like iPeng, Squeezer and SqueezeCtrl front end the real players, but also display menus via CLI. We don't see these apps when we create the top level menu. We just create a menu for Settings and one for Player Settings (we actually only want one, but some apps like the menu in Player Settings and some like it in Settings, and some can do both).

We can figure out what App ($client) is when we populate the menus, and here we have a lot of control.

So we were trying to see if there was a magic way of only creating menus for apps, but so far that has alluded us.

[SamInPgh]

Okay. I just verified via the curl command that each call to registerPluginMenu() results in overlaying the attributes of the menu created by the previous one (if any), so the MAC address in the plugin menu's 'player' attribute will always be that of the last player to register... UNLESS, that is, we simply append the MAC of each player that registers to the previous ones, if any, so we end up with an array of MAC addresses that can be parsed out with a regex. I know. I know. Just thought I'd bring up the possibility...
It looks like the only sensible solution is a change to LMS.

[SamInPgh]

Just one more thing, Craig. The showBriefly() messages are showing up now. However, in most cases they aren't displayed on the relevant screen but seem to come up at random after navigating to another page. I think the version of the plugin you have displays a message when attempting to invoke the Audio Settings menu for a non-plugin player (ironically enough) so you should be able to observe how the message is displayed in Squeezer and Squeeze Ctrl. Thanks.

[CDrummond]

Having a list of usable MACs would work as a work-around, but would need all other controllers to support this - again the real fix should be in LMS. After all other parts of the menu can be client specific, so why not this? Might need an extra function added so as to not change the existing behaviour - e.g. add a registerClientSpeficicPluginMenu

As to the timing of showBriefly messages, that again is an LMS issue. I use the long-polling libcometd to receive notifications from LMS, but LMS likes to bunch these up, etc, which can lead to delays. Material shows them as soon as they are received, so not much else I can do here.

But if these message are mainly to state that the plugin is not enabled for a client, why not just return that as text to the API call? e.g. instead of returning the actual menu, return a menu with only 1 text item stating plugin not enabled for this client.

[SamInPgh]

I agree about needing a new client-specific plugin menu function in LMS. Regarding the workaround, would you be willing to support the change I proposed involving an array of MAC addresses in the 'player' menu attribute? The vast majority of our users have only one Denon player but there are a few with more. I will also pursue getting Squeezer to support it while we await an LMS change.

With the above change, the showBriefly delay problem will be moot for the case of invoking the menu for a player not using the plugin, and you're absolutely right about displaying the other messages in the context of the menu itself, which I was already thinking of doing due to iPeng not supporting showBriefly at all.

Thanks again for your help, Craig. Btw, I installed the apk from F-Droid and am enjoying the extra features available there. Great job.