Rockbox

Tasklist

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, 05:44 GMT
Last edited by Jonathan Gordon (jdgordon) - Thursday, 24 December 2009, 06:41 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This 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
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Thursday, 24 December 2009, 06:41 GMT
Reason for closing:  Out of Date
Comment by Jonathan Gordon (jdgordon) - Thursday, 22 October 2009, 05:54 GMT
event GUI_EVENT_ACTIONUPDATE can be removed after this also (or repurposed....) nothing listens for it

event GUI_EVENT_REFRESH is always given NULL????
Comment by Thomas Martitz (kugel.) - Thursday, 22 October 2009, 10:46 GMT
GUI_EVENT_ACTIONUPDATE is used for the custom statusbar also, filetree passes a redraw function with GUI_EVENT_REFRESH in a few places (the event handler is meant to take a redraw function and call it, since it clears the whole display which may cause a noticeable flicker without redraw function). This event is still rather nasty and deactivated (remember it's only there to remove dead left-over parts from the display) if neither custom statusbar and custom ui viewport are used. It will be always active when the old statusbar code is removed. It could be removed and made into a function call then.

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).
Comment by Jonathan Gordon (jdgordon) - Thursday, 22 October 2009, 16:09 GMT
charcell statusbar isnt bitmap.. its a bunch of seperate LED segment (sort of things) than can be individually turned on/off...

"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.
Comment by Thomas Martitz (kugel.) - Thursday, 22 October 2009, 17:21 GMT
you can't just remove the icons. they're used in other places too. Besides, they're all *tiny* mono bitmaps, they take virtually no space.
Comment by Dominik Riebeling (bluebrother) - Thursday, 22 October 2009, 22:09 GMT
Jonathan: just for the record: I _do_ like the old statusbar, especially on non-color targets. I bet you could find more who actually like it.
Comment by Jonathan Gordon (jdgordon) - Friday, 23 October 2009, 04:37 GMT
brain dump time incase I dont get this done tonight....

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....
Comment by Jonathan Gordon (jdgordon) - Friday, 23 October 2009, 06:27 GMT
implemented the above idea... and got it mostly working... there is a change in skin_parser.c which I dont tihnk is needed, not sure what changed there to need it (without that the default viewport is 0,0,0,0!)
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 )
Comment by Thomas Martitz (kugel.) - Friday, 23 October 2009, 08:39 GMT
What stack? Why aren't you keeping the way of having the screen save the old state? That doesn't make any sense to me. Your way needs 2 calls as well, I can't see how that is less annoying then the current. Also, that's a separate thing, do that in a separate patch please if you want it so bad.
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.
Comment by Jonathan Gordon (jdgordon) - Friday, 23 October 2009, 17:06 GMT
"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." yes, thats fine for the statusbar but it kills the wps which is the problem... I cant see what I removed to break it, an I have no idea why you removed the code to set the correct values?

statusbar_set() and _undo() are much simpler than the previous viewport_set_statusbars() stuff
Comment by Thomas Martitz (kugel.) - Friday, 23 October 2009, 19:53 GMT
The code to fix the default viewport is in statusbar_toggle_handler (then skin_statusbar_changed), that one is also called "manually" on entering the wps. That was to consolidate the code for fixing up the default viewport since the same actions are needed on entering the wps and changing the statusbar. That code path is entirely ommitted for the statusbar and was rather disturbing to have during parsing (redraw problems etc).

>> "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?
Comment by Jonathan Gordon (jdgordon) - Monday, 26 October 2009, 03:37 GMT
arg, so i put a bit of time into makeing the old api work.. and well.. it doesnt....
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
Comment by Thomas Martitz (kugel.) - Monday, 26 October 2009, 10:35 GMT
%we should force whatever statusbar setting is set (top, bottom or custom). If the statusbar setting is off, then it forces top. I'm not sure if we can make that guess any better.

The "old api doesn't work"? Do you mean the viewportmanager_set_statusbar one? I don't think it does not work.
Comment by Jonathan Gordon (jdgordon) - Monday, 26 October 2009, 19:16 GMT
I think you're missing the point re %we... the only reason the skinned statusbar is being called a statusbar is because there isnt a better word for it just yet... nothing about it can be assumed. The only thing that is known about it is that some part of the screen should be left for the rest of the UI.
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.
Comment by Thomas Martitz (kugel.) - Monday, 26 October 2009, 22:02 GMT
I designed it work as a statusbar. Don't tell me I didn't get the point of 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
Comment by Jonathan Gordon (jdgordon) - Monday, 26 October 2009, 22:07 GMT
angry much?

ok fine then... %Vi is redundant and should be removed
Comment by Jonathan Gordon (jdgordon) - Monday, 26 October 2009, 23:43 GMT
right, well im looking at viewport.c and there are no comments explainig viewport prioritites, and the code looks like in part agrees with what you said, and in part doesnt... so YES.. there is ambiguities which need to be adressed
Comment by Thomas Martitz (kugel.) - Tuesday, 27 October 2009, 02:51 GMT
Ok, I'll have a look at fixing the comments. Please. just don't destroy what I've been working on without even asking me.
Comment by Jonathan Gordon (jdgordon) - Thursday, 29 October 2009, 06:30 GMT
ok, here we go again... this is using the old API and incorporates the argu^H^H^H^Hdiscussion from today :) (its more work to get it working while the classic bar is still in the code, so we'll do it in one hit)

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..?
Comment by Thomas Martitz (kugel.) - Thursday, 29 October 2009, 07:28 GMT
The ui viewport could be loaded before skins, couldn't it?

WTF at your option_screen() call change.
Comment by Jonathan Gordon (jdgordon) - Thursday, 29 October 2009, 07:38 GMT
they both have to be loaded before skins.. thats the important bit.. and that gets rid of viewport_get_current_vp() which shouldn't be there
Comment by Thomas Martitz (kugel.) - Thursday, 29 October 2009, 09:12 GMT
it's intentionally there. because plugins override the UI vp and do set_int() which doesn't take a viewport. Ever heard of svn log and svn blame and then asking why a certain thing is done before making assumptions?

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.
Comment by Thomas Martitz (kugel.) - Thursday, 29 October 2009, 09:15 GMT
And please just stop to mix random unrelated changes in!
Comment by Jonathan Gordon (jdgordon) - Thursday, 29 October 2009, 17:47 GMT
do you know the difference between design and actually going? I can easily see who put it there and what it does... I'm saying it shouldnt nessecarily be there.... maybe if some some design docs were done we wouldnt be here... even discussion... which also happens to be why this patch was created so early in its development cycle....

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...
Comment by Jonathan Gordon (jdgordon) - Friday, 30 October 2009, 05:09 GMT
don't take this the wrong way or anything... but you are being a big PITA here.... to split this up to removing statusbar and viewport mods into separate patches I need to add more crap code to handle the inbuilt statusbar...
Comment by Jonathan Gordon (jdgordon) - Friday, 30 October 2009, 05:53 GMT
screw that... ok here we go... part one, do the agreed on changes for how set_defaults and fullscreen work... this *will* break screens if the classic bar is enabled, big woop.. I dont care... setting a ui viewport setting here will fix it
Comment by Jonathan Gordon (jdgordon) - Friday, 30 October 2009, 06:00 GMT
part 2... fix %pb and make the parser know its default viewport from the start.

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
Comment by Thomas Martitz (kugel.) - Friday, 30 October 2009, 10:28 GMT
Looks much better. MAX(0,....) shouldn't be needed since viewport_parse_viewport takes care of that already. I'm not sure if the intersection math is correct (or the big if before). I think the UI viewport should be fixed directly (everytime a statusbar changes or the setting is loaded). It's possible to use it directly as a parent (viewport_get_current_v() )
Comment by Jonathan Gordon (jdgordon) - Friday, 30 October 2009, 17:20 GMT
the math is correct on the if().... I found a site that gave me that formula :D

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.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 November 2009, 05:11 GMT
viewport.1.diff has just been committed... working on doing the other two now. (wake up now if you want to complain :D ) also wps_parser.1.diff
Comment by Thomas Martitz (kugel.) - Wednesday, 04 November 2009, 05:24 GMT
WTF at the patch you committed. What was the statusbar-skinned.c hunk for? I wonder why something that works needs a temp fix???

I said something about the other patches here: http://www.rockbox.org/irc/log-20091030#20:33:59
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 November 2009, 05:29 GMT
chill dude!
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...
Comment by Thomas Martitz (kugel.) - Wednesday, 04 November 2009, 05:39 GMT
No time for chatting, need to go to the uni :)

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?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 November 2009, 05:42 GMT
you want to call that every time set_Defaults() is called? and that still doesnt get you out of having to check for STATUSBAR_TOP/BOTTOM.... I agree that whole files diff is bad, but it can and will be removed easily when we are ready....

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...
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 November 2009, 05:15 GMT
here we go again..... This version rips out all the guts of the old bars, right now its not fixed for screens to be able to disable the bar, or force one if its "disabled"... thats next.

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....
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 November 2009, 05:52 GMT
add the inbuilt one so a sbs is always guaranteed to be available...
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
Comment by Jonathan Gordon (jdgordon) - Monday, 16 November 2009, 06:56 GMT
updated, more complete....
the stupid root menu doesnt update properly if the theme changes though :( it bloody *should* but it doesnt.... any ideas?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 18 November 2009, 07:25 GMT
goody... very close to being committable... the problem is the viewportmanager_set_statusbar() API doesnt make sense anymore...

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...)
Comment by Thomas Martitz (kugel.) - Wednesday, 18 November 2009, 07:41 GMT
no way to force statusbar on anymore?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 18 November 2009, 07:44 GMT
there is.. but you will get the inbuilt one which is fine and will be used (if only on early usb :p )
Comment by Jonathan Gordon (jdgordon) - Friday, 20 November 2009, 06:00 GMT
fixed on target problems
Comment by Jonathan Gordon (jdgordon) - Monday, 23 November 2009, 05:21 GMT
this fixes the issue with leaving plugins (although its probably a problem in viewport_set_statusbar() which I really want to kill...

Loading...