Rockbox

Tasklist

FS#10566 - Customizable statusbar (using skin engine)

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Friday, 28 August 2009, 00:52 GMT
Last edited by Thomas Martitz (kugel.) - Monday, 19 October 2009, 15:29 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

So, this is my work on a custom statusbar. It uses the full skin engine, and accepts all WPS tags.

It replaces the classic statusbar, but you can still use the old one. It integrates as 1) an extra setting (statusbar: custom) and a skin file (<theme>.[r]sb) which is specified with sb: <path/to/file>.sb (like wps files). If it's placed next to a corresponding wps file, it uses the same image folder.

It mostly uses the existing statusbar framework w.r.t. to updating and stuff, but the sb file loads regardless of the setting (unless you specify statusbar: -, i.e. an invalid filename), so that one can toggle between classic and custom statusbar seamlessly.

It works on remotes independently of the main screen.

However, there are 2 rules.
1) You must use viewports. Statusbars without viewports are not supported. The default viewport is deactivated (but you can still define images within it). A fullscreen default viewport as the wps has totally messes up list and wps drawing.
2) It is highly recommended that you specifiy a UI viewport the statusbar is supposed to work with (using the new %Vi tag). This makes it possible to use any theme with any statusbar, since the vp behind %Vi will be copied into the UI viewport. The WPS will also use this as default viewport. Not using %Vi is likely to cause nasty drawing problems if no custom ui viewport is used (and with the wps also).

I changed wpsbuild.pl to allow for pre-packaging custom statusbar enabled themes, but I guess we need to discuss if we really want to do that. The cabbiev2.176x220x16.sb I added is for testing and is probably not going to be committed.

Added 2 test themes. The first is for e200. The second is for fuze/h300/ipod color. It uses slick_letitblue for the main screen, and a cabbiev2.rsb and .rwps for the remote.

EDIT: You can always get the latest changes from my git repo: http://repo.or.cz/w/kugel-rb.git?a=shortlog;h=refs/heads/statusbar
This task depends upon

Closed by  Thomas Martitz (kugel.)
Monday, 19 October 2009, 15:29 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed in r23258.
Comment by Thomas Martitz (kugel.) - Friday, 28 August 2009, 02:41 GMT
Extracted the patch to make the skin engine id3==NULL safe, which is needed for the statusbar to function without music playing initially.
fix voice for filename extension also.

Apply both patches, they're incremental (the token one first).
Comment by Jonathan Gordon (jdgordon) - Friday, 28 August 2009, 06:40 GMT
the sub/lines stuff is kicking my ass so I didnt get to look at this more.... :(
Comment by David Kauffmann (BdN3504) - Friday, 28 August 2009, 21:55 GMT
I don't get this to work. i downloaded the latest tar from your site and compiled a win32 sim with debian vmware image for the gigabeat. i fear it might have simething to do with filenames containing spaces, am i right? I get no errors by the wps parser though. does my code look ok to you?
great work by the way. what i don't understand is, you mentioning the %Vi tag. what is it?
Comment by Thomas Martitz (kugel.) - Friday, 28 August 2009, 22:48 GMT
Doesn't look incorrect to me at the first glance. Have you chosen did you go to theme settings -> statusbar to select *custom*?

The %Vi tag describes the viewport which doesn't disturb statusbar, i.e. the viewport where the main menu would be drawn into (if you don't use the ui viewport, for example). That will act as default viewport for every part in rockbox (again, unless you specify a ui viewport).
Comment by David Kauffmann (BdN3504) - Friday, 28 August 2009, 22:52 GMT
Thanks for clearing that up. i forgot to choose custom indeed. this looks superdope! is there much keeping it from being committed?
Comment by Thomas Martitz (kugel.) - Friday, 28 August 2009, 23:06 GMT
The upcoming release for example.
Comment by Jonathan Gordon (jdgordon) - Friday, 28 August 2009, 23:11 GMT
/me is *really* looking forward to themes using this... I just hope everyone is reembering to do it in such a way that they dont look silly when no music is playing...
and before anyone asks, yes there will be a tag so the skin can know if its in the wps, or browser, or setting, or menu :)
Comment by Gman (Thecoolgman) - Saturday, 29 August 2009, 03:26 GMT
This is brilliant, if I remember correctly jBlackglass had a patch like this except it wasn't customizable.
Comment by Thomas Martitz (kugel.) - Saturday, 29 August 2009, 19:38 GMT
sync
Comment by Gman (Thecoolgman) - Sunday, 30 August 2009, 00:56 GMT
It can't compile due to filetypes.o?
Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 11:04 GMT
make clean should help.
Comment by Julius Bagdonas (raudonkepuraite) - Sunday, 30 August 2009, 17:10 GMT
Works indeed :) Its possible to merge WPS and menus/settings into one thing, but there are some bugs though. Some images fail to redraw after exiting plugins and some settings and so does text which contains song information (like song name, album name and etc.) But it works ... can't wait to see it commited!
Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 17:48 GMT
Please upload the theme so I that I can investigate it.
Comment by Julius Bagdonas (raudonkepuraite) - Sunday, 30 August 2009, 18:16 GMT
Here You go.
Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 18:19 GMT
the first thing I see is that you didn't define a %Vi (mentioned in the first post), it's not strictly required, but highly recommended to avoid some redraw issues. That could be one reason.
Comment by David Kauffmann (BdN3504) - Sunday, 30 August 2009, 19:23 GMT
plugins dice, mazeam, pong, sliding puzzle, snake, sokoban and solitaire either need to be adjusted to use the custom ui viewport dimensions, or have to be made conform to other game menus (i.e. have the font being written on a black background).
all application plugins except clock, lamp, paint and text_editor need the same adjustments.
demos that have to be fixed: bounce, cube, logo, mosaique, snow and vu_meter.

It'll look funny on targets that have small screens, since then the plugins will only have a tiny space left in which they are displayed (depending on the dimensions of the custom ui vp of course).

i know this'll be an awful load of work, but i think to really commit this as clean as possible it has to be done. in the end, i think it'd be logical to have all plugins act the same (i.e. have the same background for the menu and also have the menu as the starting screen OR have them all start right away).
Comment by Julius Bagdonas (raudonkepuraite) - Sunday, 30 August 2009, 19:52 GMT
DONE! Still no effect...
Album art/ "art" images still disappear, and information about track (as mentioned before) disappears as wall, BUT if I replace disappearing lines with something different like volume or battery level information, then those lines wont disappear. What I'm trying to say is that this bug has nothing to do with my theme :)
I took your "slick" theme (the one which is uploaded here), modified .sb file by replacing clock with information about track ( replaced this "%ac%ck:%cM %cp" with this "%it") and after exiting plugin or using color changer (or whatever that thing is called) this line (songs title) disappears.
There are some more strange things:
Replacing backdrop (using "Set As Backdrop" setting) has some strange effects;
and
Changing themes SOMETIMES couse UI viewport not to reset. (try changin your "slick letitblue" to "slick bushfire").


Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 19:57 GMT
>>plugins dice, mazeam, pong, sliding puzzle, snake, sokoban and solitaire either need to be adjusted to use the custom ui viewport dimensions, or have to be made conform to other game menus (i.e. have the font being written on a black background).
>> all application plugins except clock, lamp, paint and text_editor need the same adjustments.
>>demos that have to be fixed: bounce, cube, logo, mosaique, snow and vu_meter.

I rather fix the plugins that use a black background. It should eventually be done, but likely not before this is committed.

>> It'll look funny on targets that have small screens, since then the plugins will only have a tiny space left in which they are displayed (depending on the dimensions of the custom ui vp of course).
Entirely the themers fault. That applies to all menus, not just plugins.


>> Album art/ "art" images still disappear, and information about track (as mentioned before) disappears as wall, BUT if I replace disappearing lines with something different like volume or battery level information, then those lines wont disappear. What I'm trying to say is that this bug has nothing to do with my theme :)

Alright, static information is apparently not being redrawn when it should (originating from the WPS, static information is supposed to be updated on track change (or a similar event). I can see that they aren't being
redrawn. I'll have a look. Album art is a bit tricky anyway, it's not really supposed to work as I haven't done any changes.

>> "Replacing backdrop (using "Set As Backdrop" setting) has some strange effects;"
What do you mean here?

>> Changing themes SOMETIMES couse UI viewport not to reset. (try changin your "slick letitblue" to "slick bushfire").
Yea, the themes need updating to include "ui viewport: -" which resets it.

Comment by Julius Bagdonas (raudonkepuraite) - Sunday, 30 August 2009, 20:10 GMT
>>What do you mean here?
Its hard for me to explain this in English :D. OK, by using your DAP go to : Files>.rockbox>backdrops>select any backdrop file (except one which is currently being used) and enter Context Menu. Now hit "Set As Backdrop" item. And this is it- screen is completely messed :) I know, this is minor bug, but still...
Comment by Thomas Martitz (kugel.) - Sunday, 30 August 2009, 20:15 GMT
Works fine here. Of course the statusbar draws over the backdrop which might look funny, but that's expected and cannot be fixed. The backdrop is part of a theme.
Comment by PaulJam (PaulJam) - Tuesday, 01 September 2009, 09:13 GMT
H300 with the custom-statusbar-v1c patch and r22570:

When holding REC during startup in order to reset the settings the device hangs at the rockbox logo. The disc continues spinning and a hard reset is required.
Comment by Gman (Thecoolgman) - Wednesday, 02 September 2009, 04:41 GMT
Take a look at this, I compile this on my 5G 60 gig. It's not on a clean build but I don't have a patches that affect filetypes.o
Comment by Jonathan Gordon (jdgordon) - Wednesday, 02 September 2009, 06:59 GMT
comments from the v1c diff:

- lots of debug code which shouldnt be needed if its ready for commit
- dont use goto in find_image(), its just as easy to use a fail bool on the while loop, actually all that is debug so dont touch that function
- 1 code line per line... "vp_refresh_mode = 0; hidden_vp = true;" is unacceptable
- +#define SDEBUGF(varargs...) //DEBUGF(varargs) <- remove
- get rid of all the debug code
- skin_data_load() should be changed to accept its default vp
- %vi stands for *info* ?! I assumed invisible untill I saw VP_INFO_LABEL....
- +/* these are never drawn, nor cleared, i.e. just ignored */
+#define VP_NEVER_VISIBLE 0x8 <- then call it VP_IGNORED and remove the comment
- unsigned char sb_file[MAX_FILENAME+1]; /* last wps */ <- dont be lazy.... fix the comment... ditto on the rsb_file
- I think sbs and rsbs is better extensions (statusbar skin)

+ TEXT_SETTING(F_THEMESETTING,sb_file, "sb",
+ DEFAULT_WPSNAME, WPS_DIR "/", ".sb"), <- more copy+paste laziness





and in make_tokes_id3_save.diff :
- I'm not wild about the HANDLE_NULL_ID3* macros... but can live with them... the zero and na variables should be renamed to something better... zero_str or something...



other than all that, my only comment would be that it would be better if the old/current/svn statusbar was completly removed (expect in the rec screen), but I can live with that being a seperate patch for later...
Comment by Thomas Martitz (kugel.) - Wednesday, 02 September 2009, 12:09 GMT
heh, half of your comments on the debug stuff, which is going to be removed any way :)

%Vi stands indeed info. It's also invisible, but that also applies for the default viewport. As for that, I was also always curious what %Vd and %Vl stand for :)


>> + TEXT_SETTING(F_THEMESETTING,sb_file, "sb",
>> + DEFAULT_WPSNAME, WPS_DIR "/", ".sb"), <- more copy+paste laziness

That is not a typo, since DEFAULT_WPSNAME is just cabbiev2, without the ext. That would also be correct for the sb (i.e. default sb is cabbiev2.sb). But, we could also make it like "#define DEFAULT_SBNAME DEFAULT_WPSNAME" and use DEFAULT_SBNAME.

>> - 1 code line per line... "vp_refresh_mode = 0; hidden_vp = true;" is unacceptable
Since when is that unacceptable? I didn't know that, I couldn't find something in CONTRIBUTING regarding that either.
Comment by Thomas Martitz (kugel.) - Thursday, 03 September 2009, 15:34 GMT
Resync. NOTE: I changed the extensions to .sbs and .rsbs as per JdGordons suggestion.

I also fixed the bug that exiting plugins don't redraw the statusbar properly (artist info was missing, for example).
Also addressed a few of JdGordon's remarks (comment typos, DEFAULT_SBS_NAME, unchange find_image()).

Full changelog (i.e. commit log) is on the git repo linked in the first post.
Comment by Gman (Thecoolgman) - Saturday, 05 September 2009, 20:23 GMT
This still can't compile on my 5G, see the image.
Comment by Thomas Martitz (kugel.) - Saturday, 05 September 2009, 20:35 GMT
Delete the entire build/ directory, and make a new one, then try again.
Comment by Thomas Martitz (kugel.) - Monday, 07 September 2009, 22:43 GMT
Sync and fix a few redraw issues.
Comment by Gman (Thecoolgman) - Thursday, 10 September 2009, 03:06 GMT
So dose this work with Custom patches or dose it not?
Comment by PaulJam (PaulJam) - Friday, 11 September 2009, 11:29 GMT
Concerrning the freeze when resetting settings during boot reported earlier:
The freeze seems to be related to the splash screen that is shown then. When commenting out the line 'splash(HZ*2, str(LANG_RESET_DONE_CLEAR));' in apps/main.c then there is no freeze anymore.
Maybe this helps finding the problem.
Comment by Thomas Martitz (kugel.) - Friday, 11 September 2009, 11:45 GMT
Alright, first go at supporting other screens with the skins. Apply these patches to
a) have %mp indicate state of recording and radio as well (recording, rec pause, radio, radio pause are new values for %mp [in that order])
b) have the %cs tag which indicates the current screen (lists or wps, rec, radio screens [in that order]). it can be used conditionally

both patches could be used with conditional viewports showing different information in the FM screen for example.

More that this shouldn't be really needed for the statusbar to work with other screens. However, displaying metadata tags within rec screen or radio still shows the tags for the last played music.

Patches are against SVN, I don't have them tested yet with the statusbar patch itself.
Comment by Will Hauck (moonscapex) - Friday, 11 September 2009, 18:06 GMT
When I try to patch this I get an error saying the file apps/gui/skin_engine/skin_tokens.c can't be found.
Everything else patched properly.
What do I do to fix this?
Thanks
Will
Comment by Robin Bertram (berti) - Wednesday, 16 September 2009, 20:14 GMT
.kugel, i think you changed the %mp tag and not the %pm tag
Comment by Thomas Martitz (kugel.) - Wednesday, 16 September 2009, 20:15 GMT
ah yes, that was a type in the comment.
Comment by Gman (Thecoolgman) - Monday, 21 September 2009, 01:28 GMT
Any updates?
Comment by Thomas Martitz (kugel.) - Monday, 21 September 2009, 08:39 GMT
Is something not working?
Comment by Thomas Martitz (kugel.) - Thursday, 24 September 2009, 02:04 GMT
Sync.
This one includes the patches I posted on sept 11th (i.e. the two above).
Comment by Will Hauck (moonscapex) - Saturday, 26 September 2009, 14:17 GMT
Almost everything works except album art and the peak meter. Album art is displayed but not resized. Also, Rockbox (3.4) has crashed a few times after patching this ('PANIC - Event list full' or something like that.)
Comment by PaulJam (PaulJam) - Saturday, 26 September 2009, 18:36 GMT
Hi,

did you really include the patches you posted on sept 11th?
With the v3b version and r22843 neither the %pm nor the %cs tags work correctly anymore:
The %pm tag shows in the rec- and radio screen always the first conditional (stop). So the new states (recording, rec pause, radio, radio pause) never get shown, the old states (play, pause, ff, rw) still work fine.
The %cs tag always shows the last conditional (radio screen), so you always see the radio specific information even when not in radio mode.
Comment by PaulJam (PaulJam) - Saturday, 26 September 2009, 18:37 GMT
sorry, i meant %mp (not %pm).
Comment by Thomas Martitz (kugel.) - Saturday, 26 September 2009, 21:42 GMT
Seems I made a little mistake. The branch on the git repo at repo.or.cz has the patches integrated.

The PANIC: event line full is easy to fix, I'll have a look (grep for "#define MAX_EVENTS" manually if you can't wait).

Album art is difficult since Rockbox doesn't support any way of multiple album art in memory right now. How exactly is the peakmeter not working? It may not update often enough (besides, peak meter needs to update really often, which is likely to noticeably slow down browsing experience, hence I don't think it's a good idea to use it - but yea, it should probably still work regardless).
Comment by Will Hauck (moonscapex) - Sunday, 27 September 2009, 01:33 GMT
The dots in the middle are displayed, but the peak doesn't seem to update. I doubt anybody would use it, so I would just remove it from the custom stausbar code. If you did get it to work, it would probably be too slow anyways.
Comment by Thomas Martitz (kugel.) - Tuesday, 13 October 2009, 01:01 GMT
This one supports albumart (apart from a few minor redraw issues).
Comment by Teruaki Kawashima (teru) - Tuesday, 13 October 2009, 13:20 GMT
great work, this has been worked well for me. and I have some questions.
I noticed that track informations on custom statusbar are not changed on track change while in WPS. Isn't it fixable?
it doesn't looks correct that HANDLE_NULL_ID3_NUM_ZERO and HANDLE_NULL_ID3_NUM_NULL are same in skin_tokens.c.
Also it doesn't looks correct for me that these macros depend on local variable. why not just use "0" and so on? And why do they return na but NULL?
What is SB_DIR in settings.h? it doesn't seem to be used. will it be used in the future?
DEFAULT_SBS_NAME and DEFAULT_WPSNAME seem to be mixed up in settings_list.c.
Comment by Will Hauck (moonscapex) - Tuesday, 13 October 2009, 16:50 GMT
This isn't for 3.4 is it?
Comment by Thomas Martitz (kugel.) - Tuesday, 13 October 2009, 19:37 GMT
>> great work, this has been worked well for me. and I have some questions.
>> I noticed that track informations on custom statusbar are not changed on track change while in WPS. Isn't it fixable?
Probably a resync bug slipped in.

>> it doesn't looks correct that HANDLE_NULL_ID3_NUM_ZERO and HANDLE_NULL_ID3_NUM_NULL are same in skin_tokens.c.
arg, typo. thanks for seeing that

>> Also it doesn't looks correct for me that these macros depend on local variable. why not just use "0" and so on? And why do they return na but NULL?
expanding "0"/"n/a" each time might mean a space waste. I don't know how gcc handles if the same literal constant is used several times. I wanted to make sure it's always using the same copy, that's why I allocate them explicitly.
Do you think they should all return NULL? That way the bar wouldn't show anything if id3 == NULL. With "n/a" it's clear that track info isn't available yet.

>> What is SB_DIR in settings.h? it doesn't seem to be used. will it be used in the future?
Should be defined the same as WPS_DIR, and used for (r)sbs; if not it's a bug (not a too critical one though).

>>DEFAULT_SBS_NAME and DEFAULT_WPSNAME seem to be mixed up in settings_list.c.
Possible another resync bug.
Comment by Teruaki Kawashima (teru) - Wednesday, 14 October 2009, 14:41 GMT
Thanks for the reply.
> Do you think they should all return NULL?
I'm not sure.
Comment by Thomas Martitz (kugel.) - Friday, 16 October 2009, 14:43 GMT
This are the 4 patches I mentioned on the mailing list that I intend to commit in a few hours.
The files contain the commit message (i.e. the description).
Comment by Thomas Martitz (kugel.) - Friday, 16 October 2009, 15:16 GMT
This would be the statusbar patch after I committed the above ones.
Comment by Thomas Martitz (kugel.) - Friday, 16 October 2009, 23:38 GMT
Tiny update

Loading...