This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#10709 - Remove the old statusbar WIP, also other viewport modifications.. also WIP
Attached to Project:
Rockbox
Opened by Jonathan Gordon (jdgordon) - Thursday, 22 October 2009, 07:44 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Thursday, 24 December 2009, 07:41 GMT+2
Opened by Jonathan Gordon (jdgordon) - Thursday, 22 October 2009, 07:44 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Thursday, 24 December 2009, 07:41 GMT+2
|
DetailsThis is the work in progress to remove the old statusbar in favour of the skined user loaded one....
This first version of the patch doesnt work.,.. i removed too much code :p statusbar.c doesnt get compiled for LCD_BITMAP targets and is fixed so the rest does compile... need to start adding code back.... I tihnk this old nonesense about forcing the bars on some screens just doesnt work anymore... so if the user loads a .sbs it will always be on screen (except if another forces it off, i.e WPS... you wont be able to force it on screen if its been disabled (i.e none loaded) To get around that problem we can add back in a second pre-canned sbs which can be loaded by a non skinabble screen when needed (i.e fm/rec) which will be a skin but not a statusbar (the user wont know the difference) SO: 1) remove the old crap (this means charcell statusbar needs some special attention!) 2) get sbs working again 3) add the pre-canned one for fm/rec 4) commit 
remove_old_statusbar.diff
(33.1 KiB)
apps/screen_access.c | 8 -- apps/screens.c | 3 - apps/recorder/radio.c | 10 --- apps/recorder/keyboard.c | 2 apps/recorder/recording.c | 11 --- apps/gui/usb_screen.c | 8 -- apps/gui/statusbar-skinned.c | 53 ++++++++++-------- apps/gui/skin_engine/skin_display.c | 37 ------------ apps/gui/skin_engine/wps_internals.h | 5 - apps/gui/wps.c | 41 -------------- apps/gui/list.c | 2 apps/gui/viewport.c | 101 +---------------------------------- apps/gui/viewport.h | 8 -- apps/menu.c | 3 - apps/settings.h | 7 -- apps/menus/theme_menu.c | 50 ----------------- apps/menus/eq_menu.c | 2 apps/settings_list.c | 19 ------ apps/filetree.c | 8 -- apps/plugin.c | 9 --- apps/SOURCES | 3 - apps/plugin.h | 10 --- apps/debug_menu.c | 2 apps/main.c | 2 24 files changed, 44 insertions(+), 360 deletions(-) |
This task depends upon
Closed by Jonathan Gordon (jdgordon)
Thursday, 24 December 2009, 07:41 GMT+2
Reason for closing: Out of Date
Thursday, 24 December 2009, 07:41 GMT+2
Reason for closing: Out of Date
event GUI_EVENT_REFRESH is always given NULL????
Or we handle the emulated classic statusbar differently. It's always full-width and completely at top or bottom. It wouldn't even need a %Vi necessarily. I'd be in favor of this.
For the reasons you mentioned, and since I don't like the text only statusbar, I would like to keep the classic-as-skinned statusbar always around. It would be shown if somethinig forces the statusbar even if it's turned off globally.
BTW: charcells don't really need special attention. We can complile the custom statusbar code and use it for charcell too. We could simply deactivate loading user defined statusbars. (Correct me if I'm wrong, this is based on the assumption that the statusbar area of charcell is the only area we can treat like a bitmap lcd (i.e. we can decide what to draw into it) - I haven't looked at the code really).
"Or we handle the emulated classic statusbar differently. It's always full-width and completely at top or bottom. It wouldn't even need a %Vi necessarily. I'd be in favor of this." sounds good.
using a text only inbuilt sbs just makes the code much nicer and simpler (and saves space on the skin buffer)... putting images here means keeping them in the core (waste) and we would like to remove all images from it. Also, ideally this would never be displayed anyway. *if* someone really liked the old bar they could redo it as a proper sbs... but I doubt that anyone actually does.
The biggest annoyance I have with the current statusbar is how painful it is to force the bar on or off (and undo the change)..
So what I'm doing is a rediculously simple mechanism where if you dont want the current/default behaviour you force the bar to what you want... then, when you are done you simple undo the call with a simple statusbar_undo(screen)... internally a stack will be kept which will then have the bar shown when its needed... the stack will only be 4 deep which should be plenty (maybe more if really needed, but i doubt it)
so instead of all the current oldbars = viewport_blaa_whatever_uit_is(); and then changing it its just....
on entering your screen - statusbar_set(screen, state);
on exiting your screen - statusbar_undo(screen);
state right now is bool, but it will in all likelyhood end up being on (users if loaded, or hardcoded), on HARDCODED ONLY, off..... actually maybe the middle one isnt needed....
there is still some ui flickering, and cleanup work to fix...
what does work:
* displaying a .sbs if one was loaded (and the statusbar is enabled, menu option not added back in though yet)
* force the bar on/off in screens (check the fm screen as an example)
* updating everywhere (including in the wps)
* display the inbuilt bar if its being forced and none are loaded
I ened up making the state stack 16 deep because 4 caused problems in the menus, 16 should be fine... I'm not sure how it will work if you enable/diable the bar in the menu though (whihc isnt a problem right now because the menu option isnt there :p )
Can you please keep it simple instead of introducing code that possibly fails with a panicf?
Re: change to skin_parser.c, it's ok that the default skin viewport is 0,0,0,0 or whatever. It's completely ignored.
statusbar_set() and _undo() are much simpler than the previous viewport_set_statusbars() stuff
>> "statusbar_set() and _undo() are much simpler than the previous viewport_set_statusbars() stuff"
I largely disagree. Anyway, can that be a separate patch and discussion please?
anyway, current problem is how the %we/d tag is supposed to work? I assume we want them to mean:
no tag - use the user viewport/remainder of the screen when "statusbar" is enabled, no change when nothing there is set
%wd - disable all user viewports... AKA give the WPS the entire real full display.... 0,0,LCD_WIDTH,LCD_HEIGHT....
%we - (force?) a user viewport? what happens here when a list vp is set in the config, and a statusbar is loaded? we need to decide the priority here (or better yet, get rid of %Vi I think...)
anyway, something to think about
The "old api doesn't work"? Do you mean the viewportmanager_set_statusbar one? I don't think it does not work.
There is an ambiguity in viewport.c (I tihnk.. going from memory now) over where the ui viewport comes from (depending on theme load order)
it either comes from the ui viewport config string, or from the %Vi viewport in the statusbar skin. we need to remove this ambiguity...
We have 2 options here...
1) remove the %Vi from the sbs which gets rid of a hack, and always use the ui viewport setting string which means themers have to set it correctly
2) use the setting string if no sbs is loaded, if one is then use the %Vi from it.
There's no ambiguity. The UI viewport *always* overrides %Vi, no matter of the load order. Stop talking about it as if it would be a hack. It is intended to make sbs theme independant (i.e. so that it works with skins that don't have a UI viewport).
I would appreciate if you wouldn't make such assumptions about the code I just wrote. Thanks
ok fine then... %Vi is redundant and should be removed
too much code is removed, and a fair few fixme's...
edit: got the wps working...
There is a specific order needed for everything to work for loading the various theme settings:
1) statusbars
1) ui viewport setting
3) WPS... (yes, 1 is there twice.. order for those two dont make a huge difference)
I'm not entirely sure why viewportmanager_theme_changed() is needed, or where it should be..?
WTF at your option_screen() call change.
I'm not convinced that the size of the default vp in the wps must be known at parsing time (%pb can be easily fixed when it's displayed for the first time). That would remove the need for a specific order.
plugins are always treated as a seperate beast, so if set_int is displaying in a full screen viewport there, big deal.
load order isnt a problem, we just need to be aware of it... and knowing the dimensions at parse time does have its bonuses (like making %pb work :p )
this isnt unrelated and random...
this of course doesnt work because heaven forbid I do more than 3 lines changed in one hit.... but dont worry... a 3rd patch is coming which will fix this by forcing the skins load order... actually this might work without the load_order patch....
apply all three and we are almost ready to remove the statusbar
viewport_get_current_v() shouldnt exist... but if it really has to, then it should be changed to having a static viewport struct and then that gets set by viewport_get_defaults() anyway... the custom viewport struct shold never be used directly.
I said something about the other patches here: http://www.rockbox.org/irc/log-20091030#20:33:59
that temp fix is for the hardcoded statusbar... WHICH IS BEING REMOVED.....have a coffee and wake up a bit first... we can argue in irc...
How about
snprintf(buf, sizeof buf, "%Vi|0|%d|%d|%d|-|-|-|, ....)
viewport_parse_viewport(vp, buf, ....)
to simulate %Vi properly instead of messing with the classic sb in statusbar-skinned.c?
load_order,1,diff if the last of those three to check in... I dont remember what exactly it does so probably wont do it tonight... I'll let things stabalise for tonight i think...
There are some very nice implications of removing the old bar:
1) WPS/skin default viewport dimensions are known at parse time... there is no more statusbar setting (just like there is no wps setting...) either a sbs is loaded or it isnt.
2) I cant tihnk of the other reasons right now :p
Now, some screens want to force the bar on, OK, if a sbs is loaded then it is *always* visible unless a screen disables it (YES I'm changing the API... tough titties... hear me out...), if a sbs is not loaded, but a screen wants it forced then the uber-shite inbuilt one will be displayed. Unless someone can come up with a good reason against this, I'm making the decision that if no sbs is loaded, the internal one will not be displayed (remember, it only going to show the clock and the battery level... ). This does mean a replica has to be shipped, but that was going to happen anyway...
So, that said.. this first version just strips the old code so it works... next step is adding the hardcoded stuff (another hour or so i guess), and finally fixing the api (TBD)
The unfortuanyly biggest thing holding all this back is the lack of a good classic replacement for all screen sizes and rec/rtc abilities....
and hooked up using the current enable API... the internal bar will only ever be displayed if a sbs is not loaded... I dont tihnk it makes sense to allow screens to ever use it if one is loaded
the stupid root menu doesnt update properly if the theme changes though :( it bloody *should* but it doesnt.... any ideas?
it used to be that a statusbar existed (so screens could force it on screen if they wanted) but the user can have it disabled. That doesnt really happen anymore, sure there is a known dimensioned inbuilt bar, but almost no screens should be forcing it on (as they will have to, because if no sbs is loaded its the same as "statusbar: off" in svn. It does make sense that screens will need to force the bar off though.
So, I go back to my proposal of something like statusbar_enable(screen, bool) and statusbar_undo(screen) which basically does exactly what viewportmanager_set_statusbar() does today, but simpler in that a screen just says "I dont care what the bars were before, I want them on/off" followed by "I'm done, put them back" on exit.
Having said that... I want to get this commited so give it a once over and try it out... that API argument can happen after this is in (I've bandaided the problem which was causing the update problem in the previous comment)
edit: ok, this one actually is commitable.. should be green, the only issue realy is the big FIXME in viewport.c in that ACTION_UPDATE callback which isnt needed anymore? (well its not needed if drawing the bars is all it was for)
(side note for discussion later, I'd be OK this going through the viewportmanager instead of the statusbar code, because (I tihnk) it might eventually make sense to en/disable different "bits" of the viewport, right now it doesnt, but if it gets to more than the sbs and ui viewport it will... actually, why does statusbar-skinned.c have anything more than a load and display function? it shouldnt have any logic other than that... it draws on events which viewportmanager configures...)