Fix menu items of type “Header” if no action is assigned to them

[Sveninndh]

A header can contain items or an URL (function pointer) or neither. If a header has items or an URL, this indicates that there is more. Material Skin indicates this with the (More) button. If you click on it, the items are displayed or the function in the URL is called up.
If a header has neither items nor a URL, there is nothing else to display. In this case, no button (More) appears and the header line should not respond to a click.
Material Skin always displays the header line as a horizontal line, regardless of whether we are currently in list view or grid view.

[michaelherger]

Would you mind looking into the web and "Button" versions of XMLBrowser, too? In those cases we should probably ignore the headers.

[Sveninndh]

Would you mind looking into the web and "Button" versions of XMLBrowser, too? In those cases we should probably ignore the headers.

I'm afraid I don't know enough about this part. When is this part executed? What devices or apps can I use to test this?

You could insert the following code starting at line 528, but that would be a shot in the dark on my part.

elsif ( $item->{type} eq 'header' ) {
next;
}

[michaelherger]

Under what circumstances would a header item have items and a URL?

[Sveninndh]

I am not aware of any. I suspect you overlooked the ‘!’.
The condition ‘no action’ for ‘header’ is true if there are no items and no url.

[michaelherger]

No, the exclusion is why I'm asking: why add that condition if it is expected to always be true? Seems to be just noise, rendering the condition difficult to read?

[Sveninndh]

A header can contain items or an URL (function pointer) or neither.

[darrell-k]

Would you mind looking into the web and "Button" versions of XMLBrowser, too? In those cases we should probably ignore the headers.

I'm afraid I don't know enough about this part. When is this part executed? What devices or apps can I use to test this?

You could insert the following code starting at line 528, but that would be a shot in the dark on my part.

elsif ( $item->{type} eq 'header' ) { next; }

Test Slim::Web::XMLBrowser with default skin.

Test Slim::Buttons::XMLBrowser with a Squeezebox Classic/2/3/Transporter if you have one, otherwise install https://lyrion.org/players-and-controllers/softsqueeze/

[Sveninndh]

Under what circumstances would a header item have items and a URL?

I am not aware of any. I suspect you overlooked the ‘!’.
The condition ‘no action’ for ‘header’ is true if there are no items and no url.

[Sveninndh]

Test Slim::Buttons::XMLBrowser with a Squeezebox Classic/2/3/Transporter if you have one, otherwise install https://lyrion.org/players-and-controllers/softsqueeze/

OK, I'll test it with Squeezeplay, which corresponds to the Squeezebox Touch.

[Sveninndh]

Would you mind looking into the web and "Button" versions of XMLBrowser, too? In those cases we should probably ignore the headers.

The processing is probably done in gotOPML().

Neither in the code nor when testing with SqueezePlay could I find anything that needed to be changed. Everything can stay as it is and corresponds exactly to the expected logic. An element of type ‘header’ without action runs to the end, is displayed without > and then calls $client->bumpRight();. This corresponds to the behavior of type ‘text’. With action, it is also displayed, but this time with > and the action is executed correctly when selected.
So no adjustment of slim/Button/XMLBrowser.pm is necessary for ‘header’.

[darrell-k]

Test Slim::Buttons::XMLBrowser with a Squeezebox Classic/2/3/Transporter if you have one, otherwise install https://lyrion.org/players-and-controllers/softsqueeze/

OK, I'll test it with Squeezeplay, which corresponds to the Squeezebox Touch.

That wouldn't use Slim::Buttons::XMLBrowser

[Sveninndh]

Test Slim::Buttons::XMLBrowser with a Squeezebox Classic/2/3/Transporter if you have one, otherwise install https://lyrion.org/players-and-controllers/softsqueeze/

OK, I'll test it with Squeezeplay, which corresponds to the Squeezebox Touch.

That wouldn't use Slim::Buttons::XMLBrowser

'Since 2009 SoftSqueeze is no longer included in the LMS distribution and is replaced by SqueezePlay.' ???

How long do you want to support this? The hardware is 20 years old. And then there's the question of how comfortable the support should be.
‘header’ doesn't really cause any problems as it is. The only thing might be to prevent an arrow from appearing on the right for headers without action. But that's just a minor aesthetic issue, nothing more.

Under no circumstances should this slow down the further development of the more modern UI.

I have one more question about pull requests.
If changes are made after the request has been submitted, what is your workflow?
Do you send a new pull request, or how do you usually do it? What happens to the old pull requests, since they are numbered?

[darrell-k]

'Since 2009 SoftSqueeze is no longer included in the LMS distribution and is replaced by SqueezePlay.' ???

You can still download it and simply run the jar file (Java JRE required).

How long do you want to support this? The hardware is 20 years old. And then there's the question of how comfortable the support should be. ‘header’ doesn't really cause any problems as it is. The only thing might be to prevent an arrow from appearing on the right for headers without action. But that's just a minor aesthetic issue, nothing more.

Under no circumstances should this slow down the further development of the more modern UI.

There are still SB2/3/Classic/Transporter users out there and we continue to support those devices. And looking at the change to Slim::Control::XMLBrowser here it doesn't look like corresponding changes to Slim::Web::XMLBrower and Slim::Buttons::XMLBrowser would be difficult, if needed.

I have one more question about pull requests. If changes are made after the request has been submitted, what is your workflow? Do you send a new pull request, or how do you usually do it? What happens to the old pull requests, since they are numbered?

Just commit and push further changes to your Header-bar-fix branch on your Github fork. It will then be added to the PR automatically if it's still open. Looks like you already did this, as there are two commits already in this PR.

[Sveninndh]

There are still SB2/3/Classic/Transporter users out there and we continue to support those devices. And looking at the change to Slim::Control::XMLBrowser here it doesn't look like corresponding changes to Slim::Web::XMLBrower and Slim::Buttons::XMLBrowser would be difficult, if needed.

You do realize that these headers have been around for two years. Has a single user of SB2/3/Classic/Transporter complained so far?

In such cases, it might make sense to wait and see if these users actually make any demands.
It works, after all.

I have now been able to disable the right arrow if there is no action available.
Of course, you could still make headers recognizable in some way and, if an action is available, add a (More) before the right arrow. As I said, this is cosmetic.
If it's that easy, then please, don't hesitate. :-)

[Sveninndh]

Just commit and push further changes to your Header-bar-fix branch on your Github fork. It will then be added to the PR automatically if it's still open. Looks like you already did this, as there are two commits already in this PR.

Cool, it's fully automatic. I just tried it with my last commit. :-)

[michaelherger]

Just commit and push further changes to your Header-bar-fix branch on your Github fork. It will then be added to the PR automatically if it's still open. Looks like you already did this, as there are two commits already in this PR.

Cool, it's fully automatic. I just tried it with my last commit. :-)

The beauty of working with Github 😉

[michaelherger]

Would a header have items? I guess that's the same comment as before: you said a header would never have items. Why keep the option to show the arrow in that not to be expected situation?

[Sveninndh]

A header can contain items or an URL (function pointer) or neither.

[Sveninndh]

Would a header have items? I guess that's the same comment as before: you said a header would never have items. Why keep the option to show the arrow in that not to be expected situation?

Yes, I think that's where the misunderstanding lies. A header can contain items or an URL (function pointer) or neither. If a header has items or an URL, this indicates that there is more. Craig indicates this with the (More) button. If you click on it, the items are displayed or the function in the URL is called up.
If a header has neither items nor a URL, there is nothing else to display. In this case, no button (More) appears and the header line should not respond to a click.
In Material, this is implemented even more elegantly (which is difficult to achieve in SB2, etc.) by always displaying the header line as a horizontal line, regardless of whether we are currently in list view or grid view.
This mechanism makes it very easy to program views from plugins, such as the one Craig implemented for global search.
There, for example, the artists are displayed first. The first 10 are displayed directly below the header. If there are more results, the (More) button appears and you can also display these if required. If there are fewer than 10 results, no button appears and the header line is just a heading without any function.
The new API functions from Qobuz are just crying out for such a possibility.

[michaelherger]

Ah, got it. That makes much more sense to me now.

[Sveninndh]

Something is broken, but I don't know where at the moment. Wait until I give the all-clear. Unfortunately, I have to leave the house now.

[Sveninndh]

Something is broken, but I don't know where at the moment. Wait until I give the all-clear. Unfortunately, I have to leave the house now.

As suspected, it was a false alarm, but yesterday the HEADER suddenly stopped working for me during searches, and I couldn't check whether it was due to the changes in this pull request, so I wanted to play it safe.
However, it had nothing to do with that; it was a nasty bug that had crept in when switching my Qobuz plugins to the latest Material version 6.1.7 and, annoyingly, had not generated a syntax error.
So the code here is fine and safe from my point of view.