Bug 5214 - Nokia770 skin doesn't respect available browse menus
: Nokia770 skin doesn't respect available browse menus
Status: CLOSED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: Skins
: 7.0
: PC All
: P2 normal (vote)
: Future
Assigned To: Squeezebox QA Team email alias
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-02 11:02 UTC by Erland Isaksson
Modified: 2007-09-23 05:57 UTC (History)
1 user (show)

See Also:
Category: ---


Attachments
Patch that hides unregistered icons (4.26 KB, patch)
2007-09-18 21:43 UTC, Erland Isaksson
Details | Diff
Simple test plugin that can disable some different browse menus (4.15 KB, application/octet-stream)
2007-09-18 21:45 UTC, Erland Isaksson
Details
Screenshot with artists, genre and new music disabled (53.94 KB, image/png)
2007-09-18 22:13 UTC, Erland Isaksson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Erland Isaksson 2007-08-02 11:02:42 UTC
I just realised that the Nokia770 and Touch skins presumes that all standard browse methods exist. 

The Custom Browse plugin does an ugly trick and calls things like this to remove standard menus and replace them with a Custom Browse implementation.

Slim::Web::Pages->addPageLinks("browse", {'BROWSE_BY_ARTIST' => undef });
Slim::Web::Pages->addPageLinks("browse", { 'ARTISTS' => 'plugins/CustomBrowse/custombrowse_list.html?hierarchy=artists&mainBrowseMenu=1' });

This works great in the Default and Fishbone skins, the standard menu is no longer visible and the new menu is added to the frontpage.

However, on the Nokia770/Touch skins it doesn't remove the original menu, but it still corrupts its url. So the url in the web interface will be:
http://localhost:9000/Nokia770/&player=01%3A02%3A03%3A04%3A05%3A06
Instead of:
http://localhost:9000/Nokia770/browsedb.html?hierarchy=contributor,album,track&level=0&player=01%3A02%3A03%3A04%3A05%3A06

The correct behaviour IMO would be if the Nokia770/Touch skin at least didn't display menus that has been removed. The optimal solution would of course be if it was also possible for a plugin to add new menus to the start page.

The thread in the development forum about this issue can be found here:
http://forums.slimdevices.com/showthread.php?p=218662
Comment 1 KDF 2007-08-03 10:48:33 UTC
This is referring to the graphic home page, as oppsed to the full home page given by the "more" link, correct?

I think this may fall into enhancement territory as it would require a method for changing what it currently hard-coded on the nokia page, or redirecting the urls used for BROWSE_BY_ARTIST instead of undef'ing it and replacing with an ARTISTS url.

is it not possible for a plugin to use the same string token for the custom url?
Comment 2 Erland Isaksson 2007-08-04 01:33:20 UTC
(In reply to comment #1)
> This is referring to the graphic home page, as oppsed to the full home page
> given by the "more" link, correct?
> 
> I think this may fall into enhancement territory as it would require a method
> for changing what it currently hard-coded on the nokia page, or redirecting the
> urls used for BROWSE_BY_ARTIST instead of undef'ing it and replacing with an
> ARTISTS url.
> 
> is it not possible for a plugin to use the same string token for the custom
> url?
> 

Yes it's the graphical page, I just realised that it requires some more work to make it work perfect. But a simple solution for now would be to just insert an IF around each icon in the home.html file. 

For example like this:
======================
[% IF additionalLinks.browse.BROWSE_BY_ARTIST %]
	[% INCLUDE link_head %]
		<a href="[% webroot %][% additionalLinks.browse.BROWSE_BY_ARTIST %]&player=[% playerURI %]" style="text-decoration:none">
		<img src="html/images/smaller/artist.gif" width="120" height="101" name="bild3" border="0" alt = 'artist'></a>
	[% INCLUDE link_foot title = 'ARTIST' %]
[% END %]
====================

By doing this, the non working icons will be removed if Custom Browse removes them. 
If Custom Browse likes to replace an menu url with its own, it can as kdf suggests use the same key as the original menu, so instead of doing:
Slim::Web::Pages->addPageLinks("browse", { 'ARTISTS' => .... });
Custom Browse can do:
Slim::Web::Pages->addPageLinks("browse", { 'BROSE_BY_ARTISTS' => .... });

To make it perfect there needs to be some way to also read the icon and title from the additionalLinks hash and there need to be a way to specify these in Slim::Web::Pages->addPageLinks. But this part is not that important so if there is other more important things to do this could be done later.

I think the title would actually work if Nokia770/Touch used "BROWSE_BY_ARTIST" as title instead of "ARTIST", this is what the Default and Fishbone skin does, so I'm not sure why Nokia770 and Touch uses "BROWSE" as title. The situation is the same for the other menus, "BROWSE_BY_ARTISTS" was just an example.

The optimal solution would of course be if the user had some way to configure which functions of those available in the additionalLinks hash that he likes to have on the front page. But this part is definitely an enhancement and not a bug.
Comment 3 Chris Owens 2007-09-18 12:49:48 UTC
Ben should this be included for the 7.0 release?
Comment 4 Ben Klaas 2007-09-18 13:00:59 UTC
I think the answer is no, but let me take a look at this today and see if I can just fix it.
Comment 5 Ben Klaas 2007-09-18 16:04:24 UTC
I experimented with the fix that Erland suggested in comment#2 today and it resulted in all of my icons disappearing, because my additonalLinks hash is empty...thoughts?

Chris, as far as priorities go, I can't see this as gating to 7.0 release.

Comment 6 Erland Isaksson 2007-09-18 21:43:11 UTC
Created attachment 2145 [details]
Patch that hides unregistered icons
Comment 7 Erland Isaksson 2007-09-18 21:45:48 UTC
Created attachment 2146 [details]
Simple test plugin that can disable some different browse menus

This is a test plugin.
By going into the "Browse Menu Remover" in "Server Settings/Plugins" you can select which browse menus to disable. By default all menus are enabled. The purpose of this plugin is just to make it easier to verify the patch.
Comment 8 Ben Klaas 2007-09-18 21:48:25 UTC
Erland, I did just what your patch did earlier today, and it resulted in all of the home icons disappearing because I don't return true for any of the IF statements...

Comment 9 Erland Isaksson 2007-09-18 22:12:27 UTC
(In reply to comment #8)
> Erland, I did just what your patch did earlier today, and it resulted in all of
> the home icons disappearing because I don't return true for any of the IF
> statements...
> 

I'm not sure what the difference is in my setup, because it works perfect here. 
I'm running the latest 7.0, updated from svn 2 hours ago and a Firefox browser if that matters. I'll attach a screenshot so you can see how it looks like. I disabled the menus with the attached plugin, which does it in the same way as Custom Browse does.
Comment 10 Erland Isaksson 2007-09-18 22:13:45 UTC
Created attachment 2147 [details]
Screenshot with artists, genre and new music disabled
Comment 11 Ben Klaas 2007-09-19 08:03:48 UTC
Erland, sorry-- your patch worked fine, which means I probably had a typo in my IF statements yesterday when I tried it myself. I patched and checked it in. Seems to work just fine. Moving to FIXED. Verify and close when you get a chance, thanks.
Comment 12 Chris Owens 2007-09-19 09:24:26 UTC
Well I am trying to be judicious in not targeting any unnecessary bugs to 7.0, but I know a lot of users like the 770 skin :)