Rockbox

Can you please separate those whitespace changes out? They make reading the patch really hard.

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

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.

@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.

   menu_backdrop.diff (1.1 KiB)

I don’t think it’s the right way too. The plugins should be fixed instead.

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..

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.

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.

I’m fine with having a do_menu parameter for the backdrop, but showing the main one unconditionally is bad imo.

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.

   menu_backdrop.diff (4.3 KiB)

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().

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.

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.

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.

   backdrops_in_plugins.diff (1.8 KiB)

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.

I thought about storing the backdrop pointer per theme_stack_item, and set it if it changed in toggle_theme(), would that work?

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.

and why would a skin not call theme_undo? isn’t that required anyway?

exactly, which is why this change is pretty safe (i think :D )

which change are you refering to now? your latest patch or my idea with the backdrop pointer?

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

feel free to commit the above patch if you want it fixed in 3.5, otherwise it will be fixed very shortly afterwards.