Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category LCD
  • Assigned To
    linuxstb
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by linuxstb - 2007-12-29
Last edited by linuxstb - 2008-03-21

FS#8385 - Viewports

Attached is my work-in-progress implementation of viewports.

Currently, they are only implemented in the lcd-16bit.c LCD driver - so this patch only compiles on targets with a colour screen and no LCD remote.

This patch adds a test_viewports.c plugin, plus a new %V WPS tag to allow use of viewports in the WPS.

The format of the %V tag is:

%V|x|y|width|height|fgcolor|bgcolor|

e.g.

%V|100|50|125|125|FF0000|00FF00|

All lines in the WPS up to the first %V tag are displayed in the default (full-screen) viewport. Lines following a %V are drawn in the new viewport - giving pixel-accurate positioning, left/right scrollmargins and custom colours.

Note that if the WPS specifies a backdrop, the bgcolor is ignored - backdrops are still global and the viewport will be transparent.

As an example of how viewports can be used, I’m attaching a modified version of jClix_Night adapted to use viewports - note that this is 220×176, so will currently only work on the iPod Color (the H300 has a remote).

Work left to do:

Finalise API - this implementation is still a work-in-progress
Adapt other LCD drivers
Modify the core Rockbox UI code to use viewports

Closed by  linuxstb
2008-03-21 19:39
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Patch v4.1 committed to SVN - r16733

Synced to SVN (plugin.[ch] changes) - no other updates.

The attached patch adds viewports to a second LCD driver - lcd-remote-1bit-v.c (used by iriver remotes). So this patch should now compile on all colour targets apart from the X5(L).

It also includes various bugfixes and API enhancements/changes - this is likely to be the final API, so work can now start on implementing viewport changes to the rest of the apps/ code.

My current to-do list (apart from implementing the remaining 5 LCD drivers):

* Validate the Viewport position/size when reading a WPS (it is the apps/ code’s responsibility to ensure that viewports are wholly contained on the screen - the LCD drivers don’t check).

* WPS images are still being drawn in the full-screen viewport, regardless of where in the .wps file they appear. Should this be changed?

* There are now three different functions to stop scrolling lines (lcd_stop_scroll, lcd_scroll_stop(vp), lcd_scroll_stop_line(vp,line)) - can these be consolidated/named better?).

* Currently the fg/bg colours in the %V tag are always mandatory, even for mono WPSs - this needs fixing.

My intention is to commit the completed version of this patch (LCD driver, scrolling and WPS changes) as soon as possible (hopefully within the next couple of days), without changing any more of the apps/ code. Work can then start on adapting the rest of the apps code to exploit viewports.

So all comments are welcome, preferably sooner than later… But if anyone wants to seriously review this patch before I commit it, but doesn’t have time immediately, please let me know.

Also very useful would be testing on various targets to ensure that no current functionality is broken - everything (including all WPSs) should work the same as without this patch.

I’m building sims with this patch applied, available here: http://rasher.dk/rockbox/viewports/

I’ll try to keep them up to date, but watch the “Last updated” date to make sure you get a version with the latest patch applied.

Attached is a corrected version of viewports-v4.diff - I forgot to add #ifdef HAVE_REMOTE_LCD to the new functions in plugin.[ch] - thanks to rasher for spotting (his sim builds already have this fix).

Can you please provide a synced version of your preview theme, I couldn’t get it to work with the latest sims of rasher. Or did I something wrong?

@kugel - I’ve just tried the zip file attached to this thread, and it works fine in both a h300 and ipod Color sim. I’m using an iBook, so can’t test rasher’s sim, but can’t think why it wouldn’t work there.

What exactly happens when you try it? Which sim did you use?

I used rashers sim for h320 and ipod photo. I played a file, and selected the jclix theme. The wps was the standard wps, for both sims.

There must be a bug in rashers simulators. I had the same problem. The sim loaded the theme but showed the default WPS. Also, I had no success when I tried to make a own WPS to work with viewports. I compiled my own sim, got my WPS working with viewports and now I tested the jClix and it works with my sim.

I’ve successfully ported a WPS that was using customline to viewports. It works perfectly on my custom-built e200 sim with bmpresize and this viewport patch. It is on the e200WPS page at http://www.rockbox.org/twiki/bin/view/Main/WpsSansaE200.

Nice work on the patch so far!

I have adapted the Cabbie v2 theme for gigabeat to viewports for reference (and made a few other personal preference changes). It can be found on the wiki at http://www.rockbox.org/twiki/bin/view/Main/WpsGigabeatF#Cabbie_BB

I’ve found what I think is a bug. Maybe its just that I didn’t write the .wps right, but I think I did since I’ve looked over it several times.

Both in the sim and on the player - whenever the volume is changed, a transparent bar appears across the AlbumArt image. Nothing else seems to be affected.

With the current implementation of viewports, would it be possible to display the current playlist in the WPS, and still have full access to the play controls? If not, how hard would it be to do that? Or, might this be a feature for the (near) future? If this is possible (or will be), this is one of the things I’d find most useful for the viewports. It’d be nice to know what’s ahead by more than 1 song, without having to look at the playlist almost everytime.

firstly there is no current implementation… all this patch does is add the backend code to allow “viewports” to be seperated on the lcd. the fun part (the part where we decide how far we want to take it) needs to be discussed…

as for the playlist view in the wps, if what I want to do happens then it will be possible, but untill apps/ starts getting converted, we dont know what will and wont happen.

This is v4.2 of the patch which fixes build issues on the gigabeats.

With the testing I’ve done on this (both on Sim and on my 5.5G ipod) the fullscreen viewport ignores the currently selected foreground colour. You can go change it in the Settings / Theme Settings / Colours, and it doesn’t change in the WPS.

@Mike Kasberg,

I think your problem is the same as I found when porting jClix - if you have a line of conditional images, then whenever that line is updated, the text-line the conditional appears on is cleared, and the images redisplayed. I’m assuming WPSes have always behaved this way.

The solution is either to do what I did in jClix (which I don’t like…) and create a 1×1 pixel viewport and use that for all your bitmaps (bitmaps are drawn based on full-screen co-ordinates, not inside the current viewport), or put your conditional image lines at the end of the WPS, so that they are drawn off the bottom of the screen.

Does anyone who knows the WPS code better than me have any other ideas/solutions?

The cleanest solution is to put the conditionals on a line that won’t cause problems being updated often. This can be a line that’s off the screen, or simply a line that already has dynamic contents.

Latest patch - this implements viewports for lcd-16bit.c, lcd-2bit-vert.c, lcd-2bit-horz.c, lcd-remote-1bit.c and lcd-remote-2bit-vi.c. This leaves two drivers (lcd-1bit-vert.c and lcd-charcell.c)

This also includes various little cleanups everywhere, plus a fix for GodEater’s fg/bg colour bug (untested, but I’m confident…)

This fixes a slight typo in the v5 patch which prevented compiling.

Attached is the first version of the patch to compile on all targets (I hope).

Please test and provide feedback…

And this time, a version that works…

The attached patch fixes a bug where the fg/bg colours were not being used with scrolling lines on greyscale displays.

On the sansa E200 I always get the following error (tested version 4.2 through 7)

Using arm-elf-gcc 4.0.3 (400)
Using arm-elf-ld 2.16.1

CC test_viewports.c
test_viewports.c:252: error: redefinition of ‘header’
test_viewports.c:22: error: previous definition of ‘
header’ was here
test_viewports.c:262:7: warning: extra tokens at end of #else directive
test_viewports.c:268: error: redefinition of ‘vp0’
test_viewports.c:26: error: previous definition of ‘vp0’ was here
test_viewports.c:289: error: redefinition of ‘vp1’
test_viewports.c:47: error: previous definition of ‘vp1’ was here
test_viewports.c:310: error: redefinition of ‘vp2’
test_viewports.c:68: error: previous definition of ‘vp2’ was here
test_viewports.c:332: error: redefinition of ‘vp3’
test_viewports.c:90: error: previous definition of ‘vp3’ was here
test_viewports.c:391: error: redefinition of ‘plugin_start’
test_viewports.c:149: error: previous definition of ‘plugin_start’ was here
test_viewports.c:561: error: redefinition of ‘header’
test_viewports.c:22: error: previous definition of ‘
header’ was here
test_viewports.c:571:7: warning: extra tokens at end of #else directive
test_viewports.c:577: error: redefinition of ‘vp0’
test_viewports.c:268: error: previous definition of ‘vp0’ was here
test_viewports.c:598: error: redefinition of ‘vp1’
test_viewports.c:289: error: previous definition of ‘vp1’ was here
test_viewports.c:619: error: redefinition of ‘vp2’
test_viewports.c:310: error: previous definition of ‘vp2’ was here
test_viewports.c:641: error: redefinition of ‘vp3’
test_viewports.c:332: error: previous definition of ‘vp3’ was here
test_viewports.c:700: error: redefinition of ‘plugin_start’
test_viewports.c:149: error: previous definition of ‘plugin_start’ was here
test_viewports.c:870: error: redefinition of ‘header’
test_viewports.c:22: error: previous definition of ‘
header’ was here
test_viewports.c:880:7: warning: extra tokens at end of #else directive
test_viewports.c:886: error: redefinition of ‘vp0’
test_viewports.c:577: error: previous definition of ‘vp0’ was here
test_viewports.c:907: error: redefinition of ‘vp1’
test_viewports.c:598: error: previous definition of ‘vp1’ was here
test_viewports.c:928: error: redefinition of ‘vp2’
test_viewports.c:619: error: previous definition of ‘vp2’ was here
test_viewports.c:950: error: redefinition of ‘vp3’
test_viewports.c:641: error: previous definition of ‘vp3’ was here
test_viewports.c:1009: error: redefinition of ‘plugin_start’
test_viewports.c:149: error: previous definition of ‘plugin_start’ was here
test_viewports.c:1179: error: redefinition of ‘header’
test_viewports.c:22: error: previous definition of ‘
header’ was here
test_viewports.c:1189:7: warning: extra tokens at end of #else directive
test_viewports.c:1195: error: redefinition of ‘vp0’
test_viewports.c:886: error: previous definition of ‘vp0’ was here
test_viewports.c:1216: error: redefinition of ‘vp1’
test_viewports.c:907: error: previous definition of ‘vp1’ was here
test_viewports.c:1237: error: redefinition of ‘vp2’
test_viewports.c:928: error: previous definition of ‘vp2’ was here
test_viewports.c:1259: error: redefinition of ‘vp3’
test_viewports.c:950: error: previous definition of ‘vp3’ was here
test_viewports.c:1318: error: redefinition of ‘plugin_start’
test_viewports.c:149: error: previous definition of ‘plugin_start’ was here
make[2]: * [/home/sander/rockbox-svn/build/apps/plugins/test_viewports.o] Error 1
make[1]:
* [rocks] Error 2
make: *** [build] Error 2

@Sander - I’m guessing you applied one version of the patch, then did “svn revert”, then applied a new version? If so, open up apps/plugins/test_viewports.c and count how many copies of the plugin is in that file…

Ah, there where many in there. Removed the file, reverted and re-applied and all is building fine now. Sorry for this.

This version has a couple of minor changes, plus a major one - it adds a font option to the %V tag in the WPS (apologies to WPS authors who have already updated WPS files to an earlier version of %V).

The syntax is now as follows:

%V|x|y|width|height|font|fgcolor|bgcolor|

Specifying “0” for font means that that viewport draws text using the built-in (6×8) system font (FONT_SYSFIXED). Note that this font only includes ASCII characters, so isn’t suitable for displaying information from tags.

Specifying “1” for font will use the currently selected user font (FONT_UI).

Other values for font are not rejected, but are replaced with FONT_UI. The intention is that the multifont patch can be updated to work with viewports, and (until that patch is suitable for and committed to SVN), themes using multifonts can be designed to degrade gracefully in the official builds.

I’m also attaching an updated version of jClix_Night_VP, which also removes the “dummy viewport hack” and instead puts all the bitmap conditionals on a single line with some dynamic content, as suggested by Nico_P. Screenshot also attached.

cg commented on 2008-01-05 14:39

For those trying to test this on sim on Linux:
The batt[5cp].bmp in jClix should be renamed to Batt* for the sim to find them. Also stop.bmp seems to be missing completely?

cg,

Thanks - I’m using a Mac, so didn’t notice the filenames were wrong. I’ve now renamed everything to all lower-case and changed the .wps appropriately.

You’re right that stop.bmp is missing (it was missing in the original jClix), but the WPS is never displayed when music is stopped…

Multifont could easily be adapted to use this. The only problem I had with multifont was the switch-case statement to set the font had to be called every single time in write_line in gwps_common.c or fonts would periodically “bleed over” onto lines they didn’t belong on. In the past it was not like that, but due to some change I never could track down it became that way. You don’t have to do that with coldfire targets though, I never could figure it out. I can update multifont, but somebody is going to have to help me if that problem still exists with viewports, it might not if we’re lucky. Also will this patch do anything with customlists/nb_lines and all that? In the past in IRC a few times I was led to believe it would.

tdtooke,

I would be happy to help debug any problems you have with porting the multifonts patch. But I don’t think you’ll have the same problem, as long as you stick to the intended design of one font per viewport.

Regarding customlists - see the comment I’ve just posted on that task. This patch is just the start…

Any idea how we want the apps/ layer to look? I keep getting stuck when I want to start updating screens… my current thinking is along the lines of moving the current screens struct (screen_access.[ch]) into firmware and calling it lcd_funcs (or something) than in apps/ get a new sturct going which would have a lcd_funcs pointer and a viewports struct, and then some extra stuff, although the only thing I can think of for the extras is the line count in the viewport…

although, typing this is turning me off that idea a bit also :(

I’ve had similar thoughts about moving the screens API into firmware, but I’m not sure it’s needed.

I was simply thinking that each screen (let’s take the list widget as an example) would have a set of viewports defined (per screen), and the code would just draw into those viewports. e.g.

struct list_viewports = {

  struct viewport title;
  struct viewport scrollbar;
  struct viewport cursor;
  struct viewport main;
  struct viewport icons;

};

(or maybe less viewports, I know we’ve discussed that before)

and then the list code would define an array of these, one per screen:

static struct list_viewports list_vp[NB_SCREENS];

You could then have a function to initialise a list_viewports struct based on a display:

static void init_viewports(struct list_viewports* vp, struct screen* display);

This function could also take overall x,y,width,height values as parameters, to draw the list within that area.

And finally, the list drawing code would draw the elements into those viewports, calling display→set_viewport() appropriately.

hmm, I guess that could work. I’m still looking for a way to not have to pass a viewport and screen pointer around.

Hi,
i have noticed a few issues with this patch while making a WPS.
Attached is a screendump showing the issues and the test WPS for reproduction.

1. In the viewport only lines that actually contain text use the specified backgroundcoulour (i would have expected, that the whole viewport uses it).

2. The peakmeter inside the viewport is at the wrong position (it should appear between the lines test3 and test4) and uses the height of the user font although the viewport uses the systemfont.

3. When an image is loaded after a viewport then the image is shown in the main viewport (like expected), but the clearing of the area for the image happens in the other viewport. In the test WPS this causes that when enabling HOLD the image appears in the main viewport, but parts of the first line in the other viewport get cleared. And when disabling HOLD again, the image doesn’t dissappear.
When moving the WPScode that shows the image before the %V line, then it works as expected.

H300 uisimulator r16011 with the v8 patch.

I’ve now committed part of this patch (the low-level LCD driver changes). The WPS changes (and other changes in apps/) still need more work - patch against SVN r16019 attached.

This patch also includes some optimisations to the WPS code (to reduce binsize a little), but doesn’t yet address PaulJam’s issues (thanks for the report).

I’ve now committed some more of this, so the only thing left in the patch are the WPS changes. So let’s rename it and start from v1 again…

so I’m _finally_ starting some coding and we need somewhere to put some viewports functions which arnt directly lcd related.. mainly a function to say how many usable lines are in the viewport (mostly used by the list, but probbaly useful elsewhere) and one to copy a viewport so we dont have this at the start of every drawing function numerous times… “struct viewport title = { .x = parent→x, .y=parent→y,

                            .width  = parent->width,
                            .height = display->get_font(FONT_UI)->height,
                            .font   = FONT_UI,
                            .drawmode = DRMODE_DEFAULT,

….

for now i’ll stick em in the list file, but a proper place is needed. not sure if it should go apps/viewports.c or firmware/viewports.c…

edit: the other thing which I just thought of which may turn out to be pointless is whenever the current_vp changes, the old value should be stored so we can just do lcd_viewport_prev()

edit2:
ok, http://rafb.net/p/2es7LV31.html is the completly unchecked code to draw the title area of the list. does this look about like how we want viewports used? (and I just realised I havnt updated the viewport there, but I may just do the one viewport_update() on the parent once all the drawing is done… the drawing for the individual items will be very similar..

You don’t need a viewport_copy function - just do “vp1 = vp2;” (where vp1 and vp2 are both “struct viewport”).

But (as I described in an earlier comment) I was thinking it would work differently to your patch - you’ve combined the initialisation of the viewports with the actual drawing. I was imagining that these would be separate - you would define all the viewports for the list centrally, and then the draw_title() function would take the viewport(s) to draw into as parameters, and do the drawing.

You also can’t define viewports on the stack - the scrolling code stores a pointer to the viewport used at the time the scrolling was set up, so it needs to be statically declared, or at least at a higher-level function.

ok, I didnt realise viewports couldnt be on the stack… that sucks a bit but can be worked around.
as for the viewport_copy().. you cant use = when one is a pointer… what I was trying to show is how I eventually want the general drawing functions to look, its given a parnt viewport and told to draw inside it…

OK, I dont know how useful this patch is but anyway… its most the of drawing logic for the lists fixed for vp. does this look workable?
http://img258.imageshack.us/my.php?image=snapshot2he6.png is an earlier version of the patch… just me showing off for irc :p

Have a play with the values of the parent viewport sturct in list.c if you want to see why I’m excited about viewports… (the code to decide what the first item to draw - and so making sure the selection is on the screen - hasnt been fixed yet, so if you change the height to be much smaller than the height of the full lcd it will go funny.. but it will still be drawn correctly. :)

You seem to ignore the user setting for the scrollbar.

@JdGordon, is it ok if I add to settings.c/h and tinker a bit on list.c on your patch to make x,y,width,height configurable from a theme?

go for it… I dont know how you were thinking about doing it… but what I was thinkg about is having the whole vp read in from a .vp file s only the filename needs to be stored in settings….

I like that, once everything is moved into viewports that would make multifont a lot easier as well. What I had in mind was initializing the viewport with things like x = global_settings→listx, but a .vp file would make it less tangled.

What about adding a line to the .cfg of the form:

menu_viewport: 0,0,100,100,0,ff0000,00ffff

i.e. add a new “type” to the settings code of “viewport”, and read all the values in a single line.

The code to parse this could even be merged with the WPS code that parses a %V tag - we would just need to pass the separator (, or |) as a parameter to a generic “parse_viewport” function.

we could do that… but that would make cfg files incompatible between targets, unless we force every item to be there. it also wouldnt allow for only setting some of the items like a seperate file would

minor changes to the previous patch… this one works properly for all viewport sizes.

do we want to get this to a commitable state asap? or can we wait a little while so I can fiddle with the list api (I’ve never liked it and want to change it, and I’d rather change it all once instead of twice…)

Project Manager

Why do you create newlist.h/c instead of changing list.c/h?

I have a quick question. If I wanted to have a setup like in one of the old PJulius themes I’d basically have to make the parent viewport fullscreen and then configure title_text, title_icons, list_text, and list_icons in newlist.c, right?

I was just thinking about having a grandparent? viewport which is fullscreen, Maybe make having the grandparent to be determined by a .cfg or a .vp file. That way it’d be very easy to configure the parent to make the customlist dimensions that are very popular with a lot of people.

Sorry for the triple post, but also not having a grandparent would basically take away backdrops.

Linus, dont worry, they wont be there when I’m finished.
Travis, ?

i noticed that when adding more than 15 %V tags to a wps file the ui-simulator crashes.
is this the natural restriction of the number of viewports or just a bug?

also i think the viewports should draw their complete “rectangle” with the background colour no matter if anything is displayed at all. atm only the lines where text is are “backgrounded”.

i also had a minor issue with overlapping viewports:
when having one viewport spanning a complete textline containing a left-aligned text and a right aligned text, i then made a second viewport on the same y coorinates but between both texts containing a scrolling line.
problem there was that the “middle” viewport was not displayed at all. i had to split the texts up into 3 viewports to make the line work like i wanted it to work.

and i also faught with the previously mentioned problem with conditional graphics in the background viewport clearing a complete textline beneath the graphics (including a existing albumart).

What I meant was you have a parent viewport in list.c now with it’s children in newlist.c. If the parent had a parent of it’s own then it’d be possible to have the parent of the parent fullscreen so you could have a full backdrop displayed and could set the size of the parent to whatever your customlist dimensions were.

Disregard all of that, no reason for me to complicate things.. I was just thinking maybe a .vp file containing entries for the parent viewport and the individual components of the list maybe like what linuxstb was talking about for a .cfg.

I played around with viewports everything is working fine (iRiver H10). But i also noticed that a viewport containing album art isnt redrawn correctly if you have a viewport called by a conditional directly over it. I remember thats because album art is only loaded once in the gwps-common.c code at the beginning of a new song.

That must be why I see there’s a huge line in my album art…

In my own custom WPS, I have the volume level aligned left and the time aligned right on either side of the album art, and it creates a white line through the picture. If I create two viewports on either side that don’t invade that space, will that eliminate the line?

It might be incorrect width of the %V tag causing the line e.g. glitching to the album art as the volume changes.

1) @dberg918 - Within a viewport, the WPS will behave exactly as it used to behave full-screen without viewports. So if you have a line with text left-aligned and right-aligned, and album-art displayed in the middle, then the album-art will be overwritten. As you say, you should create two viewports - one for the left text, and one for the right text.

2) Sacha - I’m not understanding what you mean by “a viewport called by a conditional”. But viewports must be on a line by themselves, it’s not (currently) possible to make them conditional.

I’m probably not going to get a chance to work on this patch for at least a couple of weeks, so if anyone else wants to carry on and try to get it to a committable state, please do.

but it would be nice if you can have a conditional like %?C<%V|100|50|125|125|FF0000|00FF00||%V|100|50|75|125|FF0000|00FF00|> so you have a bigger viewport if no cover was found, but if a cover is found the text only shows beside the cover in the smaller viewport. so you could have the full functionality of the viewport-idea. I thin that was what Sacha meaned.

It would be nice if you could have an album art conditional along with viewports as a lot of the themes being released nowadays use album art conditionals…

Modified the wps as per below to run on a video 5g ipod sim, found the play etc. icons flash in the viewport on and off not sure if this is a bug?

%Cl|21|41|s75|s75|
%X|background.bmp|
%P|progressbar.bmp|
#
%xl|a|stop.bmp|100|163|
%xl|b|play.bmp|100|163|
%xl|c|pause.bmp|100|163|
%xl|d|ff.bmp|100|163|
%xl|e|rew.bmp|100|163|
%xl|f|vol0.bmp|63|163|
%xl|g|vol1.bmp|63|163|
%xl|h|vol2.bmp|63|163|
%xl|i|vol3.bmp|63|163|
%xl|j|vol4.bmp|63|163|
%xl|k|vol5.bmp|63|163|
%xl|l|vol6.bmp|63|163|
%xl|m|vol7.bmp|63|163|
%xl|n|vol8.bmp|63|163|
%xl|o|vol9.bmp|63|163|
%xl|p|vol10.bmp|63|163|
%xl|q|Batt1.bmp|192|5|
%xl|r|Batt2.bmp|192|5|
%xl|s|Batt3.bmp|192|5|
%xl|t|Batt4.bmp|192|5|
%xl|u|Batt5.bmp|192|5|
%xl|y|Battp.bmp|192|5|
%xl|z|Battc.bmp|192|5|
%xl|A|Hold-on.bmp|175|5|
%xl|B|Hold-off.bmp|175|5|
%xl|C|hdd.bmp|156|5|
%wd
%C
%V|10|5|100|14|1|737675|000000|
# NOTE: These conditionals all blank the current line when they update,
# so we place them all on the same line, along with some dynamic content
%?mh<%xdA|%xdB>%?bp<%xdy|>%?bc<%xdy|>%?lh<%xdC|>%?pv<%xdf|%xdg|%xdh|%xdi|%xdj|%xdk|%xdl|%xdm|%xdn|%xdo|%xdp|%xdp>%?bl<%xdz|%xdq|%xdr|%xds|%xdt|%xdu|%xdu>%?mp<%xda|%xdb|%xdc|%xdd|%xde>%t5%?mh<Battery: %bl%%|%cl:%cM %cp>;%t3%?mh<Battery: %bt|%cd/%cm/%cy>
%V|16|124|200|15|1|C8BFB5|000000|
Next Song:
%V|100|68|150|15|0|FFFFFF|000000|
%al%pc%ar%pt
%V|129|69|100|6|1|FFFFFF|000000|
%pb|6|0|59|0|
%V|100|41|150|15|1|FFFFFF|000000|
%s%ac%?ia<%ia|%?d2<%d2|Artist Unknown» %V|100|80|150|30|1|FFFFFF|000000|
%s%?id<%id|%?d1<%d1|Album Unknown» %s%?it<%it|%fn>
%V|84|124|200|15|1|FFFFFF|000000|
%s%?It<%It|%Fn>
%V|16|158|200|15|1|C8BFB5|000000|
%al%?mp<Stopped|Playing|Paused|Forward|Rewind>%ar%?mm<%?ps<%<S%>| >|%?ps<%<S+All%>|%<All%»|%?ps<%<S+One%>|%<One%»|%?ps<%<S+Sfl%>|%<Sfl%»|%?ps<%<S+A-B%>|%<A-B%»> %pp of %pe%?mm< |>

hunk errors patching viewports-wps-v1.diff with the most recent svn.
i wasnt able to resolve them, so please resync. best would be to bring the wps viewports into a commitable state.

disorganiser: DO NOT post resync requests… its a sure fire way to get devs pissed with you and to lose interest in the patch if they havnt already.

and what would in yo be the right way to ask for a resync?
also i hope interest in the vp implementation is not lost.
and if so i hope the devs post it so we can continue to use the “Old” methods of font-usage etc. >-)

Where do you get hunk errors? I can apply in just fine!

the correct way is you dont ask for resyncs… either you do it or you wait… the patch owner will know when it needs syncing…

@kugel: it works now. propably some faulty svn copy on my other pc. i will check in detail when i get to the other pc.
@jg: ok, thanks for the info.

@Dave Chapman: on the multifont end I think I’ll be able to handle the string comparisons and all that to load a font from the font directory for the list viewport so font names can be real font names, but I think that’s going to be a problem for the WPS. I still kinda want to go with numeric names for the fonts already specified in the theme cfg file. Thoughts?

numeric names sounds fine to me and seems to make perfect sense.

I really would like to hear a sane argument on how having a number represent a font makes and sense what so ever?

The only sane argument of numeric font statements I see is ease of changing the font on theme globally.
Example:
If you state ‘2’ for ‘arial.fnt’ in .cfg file - all ‘2’ stated fonts will be changed.
With ‘FontName.fnt’ for font statement this quick fixes will be impossible. But personal opinion full names will be easier for theme developers.

when using a font-number (index) you can change the font in the config file rather than in the wps file.
thus the font must only be changed on one position instead of multiple position (in each vp).

the more vp’s we get (wps, lists, whateverwethinkofnext) the more places there will be where fonts may need to be changed.
as a theme imho will use the same or similar font for many of its vp’s, the fontchange can be easily done via theme-cfg compared to changing all cfg-files of the theme.

also the lines are shorter in the wps-files and have a fixed length so you can easily compare settings.
example:
%V|16|158|200|15|1|C8BFB5|000000|
%V|84|124|200|15|2|FFFFFF|000000|
compared to
%V|16|158|200|15|nimbus-14|C8BFB5|000000|
%V|84|124|200|15|hevetica-bold-0815|FFFFFF|000000|

but on the other hand of course the fontname is more “readable” in the cfg’s.

furthermore fonts were always set in the config, so the user could change them to his liking. changing a font shouldn’t manipulate the wps file.

@jdgordon <rolls eyes> seriously, your tone leaves something to be desired, but anyways.. From my perspective on the list it’s not a problem because you’d be loading that font when a .cfg is loaded and it work like every other font setting, but with a wps calling a font in that way means you’d want to be able to load a font “on the fly” that might not be preloaded from the fonts specified in a .cfg. That is unless it’s made so you can’t load a wps individually and changing that is restricted to when you change a theme.

I was wondering… when defining a viewport, could there be an option to tell it that you want the text to be wrapped to the next line if the text exceeds the limit of the viewport? This would be helpful if for example you want to define an area for the album name and happens to be a large name; instead of scrolling or cutting at the end of the line, it could display in 2 rows.

Just a thought… thanks!

that wouldn’t be viewport specific, would it? in my opinion it’s a general wps feature.

Ben, why did you reply with that message again? once is enough.. and no its not possible

I am so sorry. I didn’t mean to. I accidentally refreshed this page to see if anything was added and I didn’t remember the last time I saw this was when I posted that message so it got posted again. And I couldn’t find how to delete it. Maybe I’m not old enough to delete comments from here… :(

Sorry…!

another argument for the numbering of fonts:
you can easily use switch with an integer :-)

an argument for font naming:
you can easily generate a global function mapping font-identifiers or font names to numbers for use with switch.

another question would be:
will we really want to use fontnames in wps’s or vp definitions?
or will we rather use “font identifiers” indexing on the lines in the cfg-file and allow the user to customize all possible fonts via theme-setting menu?
example:
will we use “nimbus-12” and “nimbus-14” directly in the vp setting-line and the wps, or will we rather use:
user1 and user2 to index on the config-file values “userfont1: nimbus-12” and “userfont2: nimbus-14” and allow to user to freely choose all possible fonts (userfont1-15, menufont, …..).

I am for naming fonts in the theme file (.cfg) and fonts in the wps just as numbers (1, 2). And what has really been going on with actual viewports WPS implementation? Has the actual coding not been finished or refined or are people just opposed to it?

hopefully synced to 16589 without any functional changes.

I’m sorry to be a bother, but what is going on with this? Will it ever be committed or is there something that needs to be finished in order for it to be committed?

I’ve just had an idea regarding fonts:

Why not just defining the fonts used in the same way as bitmats? Something like

%Font1|[/.rockbox/fonts/]font.fnt
%Font2|[/.rockbox/fonts/]another_font.fnt

And in the viewport it would be like this:
%V|x|y|width|height|[%]Font1|fgcolor|bgcolor|

This is more logical for me than the current solution. And of course this should go hand in hand with a multifont solution.

@crzyboyster - there are lots of bugs reported in this task which need to be fixed, plus the issue of fonts to be resolved. I now have some free time for Rockbox, and am working on this again, but can’t say if/when it will be good enough to commit.

@kugel - If I understand your suggestion, fonts will be defined in the .wps file, and not globally in the global settings (.cfg). Why? How does this go hand-in-hand with multifont, when multifont is intended to be used in the whole of Rockbox, not just the WPS?

I tihnk the best solution for the fonts is to have the placeholder in the vp config line and just ignore it for now. That should cause the least problems when multifont is commited.

@jdgordon: I disagree. IMO, we should do things in the natural order - i.e. multifont support (or at least, agreement on what the multifont API should be) is a pre-requisite for all other UI rework. By ensuring the UI code works with multiple fonts (currently only system and user font) from the start, it will mean none of the UI code will need to be changed in order for full multifont to work.

At least, that’s how I want to work on this WPS patch.

But for now I’m ignoring fonts (keeping the existing 0/1 numeric implementation) and working on the other issues.

linuxstb, I only meant the fonts which are used in the WPS. Other global fonts (UI, recording screen or whatever) belong in the global .cfg.

My idea basically moves the userfont part of the multifont patch into the wps, which has more sense imo.

I just said it should go hand in hand with a multifont solution since those font definitions make only sense when one can actually choose between fonts.

my personal thinking is:
we first need to get agreement on how to enable multiple fonts in the config and theme files and how to work with fonts wherever we use them.
if we agree on anything there, we can work on getting a multifont patch submitted which enables all needed functions for other parts of rockbox (loading of fonts, font usage in viewports, font caching, settings management, userUI to select the fonts etc.).

after that is commited we can work on enabling mf for wps and other screens using viewports.

maybe it would be better to take this discussion (especially the discussion how to use fonts and which functions need to be in the mf-patch to get it commited) into the forum or irc.

atm all 3 patches are a bit stalled because the discussion is not finished (mf needs wps-vp-fontsupport, list-vp needs mf, and all must wait for outstanding decissions).

I think that the best idea is to define the fonts in the .cfg file and name them with numbers. This should preserve backwards compatibility with older themes because the fonts in the cfg file will still be read as the font being used and so on.

the mf patch could indeed read the font_ui from the config file, and if none of the new directives is present use this for all configurable fonts.

Can you tell me any reason why it’s better to define the WPS fonts in the global config instead of in the wps file?

Makes WPSes more flexible. Right now if you change the menu font, it affects the WPS. For a knowledgeable user, this can be positive, since if you pick a font with the same height, it works.

WPSes with viewports can be designed not to care about font height at all. And even if they aren’t, users can still use a .cfg to load an alternative set of fonts without having to directly modify the .wps, if they want to use a variant of that .wps.

It also allows a clever user to create a .wps that is mostly font independent, then use a simple .cfg to load an alternative set of fonts to make the same .wps more usable in the car (large fonts) but show less information (it would push certain optional information out of the displayed viewport).

I can see several reasons why having them in the .cfg would mean a much more flexible player overall, for me.

I’ve created a wiki page to discuss how multi-font support should work:

http://www.rockbox.org/twiki/bin/view/Main/MultiFontSupport

As much as possible, can we keep this task focused on the other aspects of WPS viewports patch - it’s getting hard to find WPS-specific posts here… Discussion of how multi-font should be implemented affects much more than just the WPS.

Here’s an updated version of the viewports patch, and matching version of jClix_Night_VP (my 220x176x16 test WPS - for the H300 and ipod Color/Photo).

Changes compared to viewports-wps-v1.diff:

1) Images are now displayed within the current viewport (see my jClix example for how this is working). This will break all existing WPSs written to use earlier versions of my viewports patch but I think it’s the more logical approach.

2) Fix the bugs reported by PaulJam on 7 Jan 2008 (thanks for the clear report) - the test WPS posted with that bug report now displays correctly.

3) Other small bug fixes and cleanups

Still to do before I would consider committing it:

1) Clean up the parsing of the %V tag - reduce code size and ensure viewports are fully validated before using. This parser could be shared with other parts of Rockbox that will be parsing viewport structs (e.g. custom lists).

2) Decide on how fonts should be specified - see http://www.rockbox.org/twiki/bin/view/Main/MultiFontSupport

3) More testing - help needed! Please try using these new features in your WPSs and report any issues.

I forgot to say that I would also like to find a way to make viewports conditional.

The normal method of conditionals won’t work because of the way I’ve implemented viewports. Previously, the WPS consisted of a single array of lines. With my viewports patch, a WPS is now an array of viewports, with each viewport containing an array of lines.

So we need some way to specify whether each viewport should or should not be displayed - suggestions for a syntax to accomplish this are welcome.

Given that the only use case I can think of is for album art, maybe we should just hard-code something for that (e.g. %V for viewports which are always displayed, %VC for display when there is album art, and %Vc for display when no album art), but it would be nicer to have a more flexible system.

Or do we need the ability to make the co-ordinates in a viewport conditional? That would be much more complex, but I can see the usefulness…

I think making coordinates in a viewport would be the best idea because it would allow the most flexibility if pulled off properly without making the syntax impossible to understand to beginners. Don’t ask me what the best way to implement that would be as I honestly have no idea. And solution 1 on the multifont page seems the most sensible to me.

another idea, maybe strange but:
would it make sense to define one viewport inside the wps which will be used for list-display?
so for example if i use VP5 for this “combined VP”, i could define AA to show there, and as soon as i press “select” the AA is not displayed in VP5 any more but the resulting list (for example context menu) is displayed inside VP5.
if i return to the WPS from the list the AA will be displayed in VP5.

if we then manage to update the WPS information during list-display, we could easily allow menu navigation, playlist editing etc during playback inside a wps ;-)

oh well, maybe we should first get the rest commited *g*

just some notes:
* remember to convert the coordinates of the pictures to viewport-related coordinates :-) * do NOT use overlapping viewports

mf patch got synced to this one, and aa-resize also works with it.

A small update to fix images displayed by the %x tag - thanks to PaulJam for reporting this in IRC.

CFP commented on 2008-03-20 15:55

Tested successfully on gigabeat f60 with last svn and provided theme

Synced to current SVN (I committed some conflicting changes as r16728 earlier today.

Also improves the viewport parser. Please continue testing and reporting any problems.

Still left to do:

1) Fix the checkwps tool - this won’t compile due to the validation of viewports based on the device’s LCD dimension/depth. So we need to add some way to specify the target we’re checking for to checkwps.

2) Decide what to do about conditionals

3) Decide what to do about fonts

EDIT: The v4 patch previously attached to this comment had a line missing, please use the attached v4.1 instead.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing