Rockbox

Tasklist

FS#10868 - Always display backdrop in the plugin's menu

Attached to Project: Rockbox
Opened by Johannes Schwarz (Ubuntuxer) - Saturday, 19 December 2009, 21:53 GMT
Last edited by Jonathan Gordon (jdgordon) - Saturday, 29 May 2010, 14:29 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Release 3.5
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

I wrote a small patch, which display always backdrop in the plugin's menu.
But I have the sense that on the targets the menu is displayed delayed.
Please test it and give me feedback.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Saturday, 29 May 2010, 14:29 GMT
Reason for closing:  Fixed
Comment by Thomas Martitz (kugel.) - Saturday, 19 December 2009, 22:57 GMT
Can you please separate those whitespace changes out? They make reading the patch really hard.
Comment by Jonathan Gordon (jdgordon) - Sunday, 20 December 2009, 01:52 GMT
that is not the correct way to do it... the backdrop should be linked to the theme manager (so its always on if the sbs/ui vioewport are enabled and off otherwise
Comment by Teruaki Kawashima (teru) - Sunday, 20 December 2009, 01:53 GMT
IMO, loading backdrop from disk when open the menu is the worst way.
What i think is needed is simple function(s) to just only show/hide backdrop.
I also had thought an idea that backdrop is hidden when the theme is disabled.
but probably cause another problem - backdrop is not shown or shown partly.
Comment by Johannes Schwarz (Ubuntuxer) - Sunday, 20 December 2009, 12:34 GMT
@kugel sorry

What do you mean with theme manager ? (viewport.c: viewportmanager_theme_enable())?
The function "backdrop_show(BACKDROP_MAIN) " shows the backdrop if the backdrop was loaded once.
You can hide the backdrop with function "lcd_set_backdrop(NULL)".
> IMO, loading backdrop from disk when open the menu is the worst way.
You're right, so I rewrote the patch and use the function "lcd_set_backdrop(NULL)".
Now it works fine for me, the menu isn't displayed delayed anymore.
Comment by Thomas Martitz (kugel.) - Sunday, 20 December 2009, 13:29 GMT
I don't think it's the right way too. The plugins should be fixed instead.
Comment by Jonathan Gordon (jdgordon) - Sunday, 20 December 2009, 17:45 GMT
no, it should be linked to the viewportmanager_theme_enable/undo() functions... but it needs to be carefully thought out... we are getting to the point where there is really no reasont he sbs cant have its own background image, as well as the wps and fm screens..
Comment by Thomas Martitz (kugel.) - Sunday, 20 December 2009, 17:46 GMT
Why should a plugin get the backdrop anywhere if it sets it to NULL explicitely at the start? that doesn't make sense. It should set the backdrop around do_menu() calls.
Comment by Jonathan Gordon (jdgordon) - Sunday, 20 December 2009, 17:57 GMT
must you always argue? its two seperate issues..
1) its not only plugins are show this problem, the only reason it works in the wps now is because its handled where it shouldnt be
2) plugins arnt core and have a different set of assumptions, this might be bad, but there are how many plugins that all need to be fixed.
Comment by Thomas Martitz (kugel.) - Sunday, 20 December 2009, 18:11 GMT
I'm fine with having a do_menu parameter for the backdrop, but showing the main one unconditionally is bad imo.
Comment by Thomas Martitz (kugel.) - Wednesday, 06 January 2010, 17:29 GMT
Ok, I thought about it. The approach of this patch isn't that bad. My initial concern is still valid (imo), so I decided to go another route.

As do_menu() is made to have a simple way of creating a list, particularly for plugins, I changed to meaning of hide_bars to hide_theme. So, passing false will show both the SBS and the backdrop and passing true will hide both.
In sbs times, hiding the bars only isn't really appropriate. Turning it to hide_theme keeps it simple, and still allows to override (with a custom viewport and backdorp for example).

This way we can commit it IMO.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 06 January 2010, 17:56 GMT
I'm in agreement all the way up to the last sentence!
changing to hide_theme is the logical way to go, but the backdrop itself is part of the theme NOT the list, therefore the showing/hiding of it needs to happen inside viewportmanager_theme_enable().
Comment by Thomas Martitz (kugel.) - Wednesday, 06 January 2010, 18:06 GMT
I can agree with that. But I'd like to sort that out later.

Changing it now to happen in viewportmanager_enable() is a bit more work since it's called more often, and I'm not very familiar yet with how viewport.c works today.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 January 2010, 01:42 GMT
bloody hell... OK the real problem is that backup_unload() actually disables the damn backdrop, not just sets the lcd driver to show nothing. (so the backdrop needs to be reloaded from disk!, unless im missing something)

I have a cleaner way to do this without manginglg backdrop.c which ill work on this evening if i dont do anything else.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 January 2010, 02:49 GMT
This is the bare minimum that needs to be done for this to work. What this does is unconditionally set the backdrop to the main one when the theme is enabled, and hides it when its disabled.
If we go this route, it would make things slightly simpler for skined screens because they would need to manually toggle their backdrop, but as soon as they disable the theme it will revert to the main.
Comment by Thomas Martitz (kugel.) - Wednesday, 20 January 2010, 07:08 GMT
lcd_set_backdrop(NULL) makes the lcd driver to show nothing.

skins don't necessarily disable the theme; the wps enables or disables it depending on %we/%wd.
Comment by Thomas Martitz (kugel.) - Wednesday, 20 January 2010, 07:14 GMT
I thought about storing the backdrop pointer per theme_stack_item, and set it if it changed in toggle_theme(), would that work?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 January 2010, 07:24 GMT
If a skin doesnt disable the theme and they still want to show a backdrop then this will still work fine.
If a skin does load a backdrop, and then doesnt call theme_undo() before going into any menus, this wont nessecarily show the write thing.
Comment by Thomas Martitz (kugel.) - Wednesday, 20 January 2010, 07:26 GMT
and why would a skin not call theme_undo? isn't that required anyway?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 January 2010, 08:24 GMT
exactly, which is why this change is pretty safe (i think :D )

Comment by Thomas Martitz (kugel.) - Wednesday, 20 January 2010, 08:33 GMT
which change are you refering to now? your latest patch or my idea with the backdrop pointer?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 20 January 2010, 08:37 GMT
my patch, response to "and why would a skin not call theme_undo? isn't that required anyway?" to "this wont nessecarily show the write thing."

the only background which the theme should be able to set is the "main" one, skins need to manage their own (which will happen without extra work anyway). my plan is to simply load the backdrop into the skin buffer, save a pointer on the wps_data struct, backdrop_show() when the skin needs to do a full refresh (which the screen will manage accordingly, ah la wps_enter_screen() or whatver the heck its called... this will probbaly all happen in 10922
Comment by Jonathan Gordon (jdgordon) - Friday, 22 January 2010, 04:40 GMT
feel free to commit the above patch if you want it fixed in 3.5, otherwise it will be fixed very shortly afterwards.

Loading...