Rockbox

Tasklist

FS#8385 - Viewports

Attached to Project: Rockbox
Opened by Dave Chapman (linuxstb) - Saturday, 29 December 2007, 20:14 GMT
Last edited by Dave Chapman (linuxstb) - Friday, 21 March 2008, 19:39 GMT
Task Type Patches
Category LCD
Status Closed
Assigned To Dave Chapman (linuxstb)
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

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

Closed by  Dave Chapman (linuxstb)
Friday, 21 March 2008, 19:39 GMT
Reason for closing:  Accepted
Additional comments about closing:  Patch v4.1 committed to SVN - r16733
Comment by Dave Chapman (linuxstb) - Saturday, 29 December 2007, 23:54 GMT
Synced to SVN (plugin.[ch] changes) - no other updates.
Comment by Dave Chapman (linuxstb) - Monday, 31 December 2007, 10:29 GMT
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.
Comment by Jonas Häggqvist (rasher) - Monday, 31 December 2007, 11:44 GMT
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.
Comment by Dave Chapman (linuxstb) - Monday, 31 December 2007, 12:17 GMT
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).
Comment by Thomas Martitz (kugel.) - Monday, 31 December 2007, 15:21 GMT
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?
Comment by Dave Chapman (linuxstb) - Monday, 31 December 2007, 16:33 GMT
@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?
Comment by Thomas Martitz (kugel.) - Monday, 31 December 2007, 16:49 GMT
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.
Comment by Daniel Siebrecht (DimensionSix) - Monday, 31 December 2007, 16:50 GMT
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.
Comment by Mike Kasberg (digerati1338) - Monday, 31 December 2007, 20:08 GMT
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!
Comment by Alex Parker (BigBambi) - Tuesday, 01 January 2008, 11:20 GMT
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
Comment by Mike Kasberg (digerati1338) - Thursday, 03 January 2008, 03:49 GMT
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.
Comment by Marvin Miller (Mouser X) - Thursday, 03 January 2008, 06:40 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 03 January 2008, 08:04 GMT
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.
Comment by Bryan Childs (GodEater) - Thursday, 03 January 2008, 10:57 GMT
This is v4.2 of the patch which fixes build issues on the gigabeats.
Comment by Bryan Childs (GodEater) - Thursday, 03 January 2008, 12:14 GMT
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.

Comment by Dave Chapman (linuxstb) - Thursday, 03 January 2008, 14:30 GMT
@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 1x1 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?
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 03 January 2008, 14:34 GMT
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.
Comment by Dave Chapman (linuxstb) - Thursday, 03 January 2008, 15:34 GMT
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...)

Comment by Bryan Childs (GodEater) - Thursday, 03 January 2008, 19:50 GMT
This fixes a slight typo in the v5 patch which prevented compiling.
Comment by Dave Chapman (linuxstb) - Thursday, 03 January 2008, 20:05 GMT
Attached is the first version of the patch to compile on all targets (I hope).

Please test and provide feedback...
Comment by Dave Chapman (linuxstb) - Thursday, 03 January 2008, 21:50 GMT
And this time, a version that works...

Comment by Dave Chapman (linuxstb) - Friday, 04 January 2008, 00:26 GMT
The attached patch fixes a bug where the fg/bg colours were not being used with scrolling lines on greyscale displays.
Comment by Sander Sweers (infirit) - Friday, 04 January 2008, 21:47 GMT
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
Comment by Dave Chapman (linuxstb) - Friday, 04 January 2008, 22:23 GMT
@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...
Comment by Sander Sweers (infirit) - Friday, 04 January 2008, 23:40 GMT
Ah, there where many in there. Removed the file, reverted and re-applied and all is building fine now. Sorry for this.
Comment by Dave Chapman (linuxstb) - Saturday, 05 January 2008, 12:00 GMT
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 (6x8) 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.
Comment by Henri Valta (cg) - Saturday, 05 January 2008, 14:39 GMT
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?
Comment by Dave Chapman (linuxstb) - Saturday, 05 January 2008, 16:11 GMT
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...
Comment by Travis Tooke (tdtooke) - Saturday, 05 January 2008, 23:58 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 06 January 2008, 00:37 GMT
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...

Comment by Jonathan Gordon (jdgordon) - Sunday, 06 January 2008, 06:57 GMT
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 :(
Comment by Dave Chapman (linuxstb) - Sunday, 06 January 2008, 11:08 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Sunday, 06 January 2008, 19:42 GMT
hmm, I guess that could work. I'm still looking for a way to not have to pass a viewport and screen pointer around.
Comment by PaulJam (PaulJam) - Monday, 07 January 2008, 11:25 GMT
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.
Comment by Dave Chapman (linuxstb) - Monday, 07 January 2008, 22:38 GMT
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).
Comment by Dave Chapman (linuxstb) - Tuesday, 08 January 2008, 01:26 GMT
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...
Comment by Jonathan Gordon (jdgordon) - Tuesday, 08 January 2008, 07:06 GMT
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..
Comment by Dave Chapman (linuxstb) - Tuesday, 08 January 2008, 11:07 GMT
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.

Comment by Jonathan Gordon (jdgordon) - Tuesday, 08 January 2008, 16:57 GMT
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...
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 January 2008, 08:21 GMT
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. :)
Comment by Thomas Martitz (kugel.) - Wednesday, 09 January 2008, 15:31 GMT
You seem to ignore the user setting for the scrollbar.
Comment by Travis Tooke (tdtooke) - Wednesday, 09 January 2008, 22:11 GMT
@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?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 January 2008, 22:32 GMT
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....
Comment by Travis Tooke (tdtooke) - Wednesday, 09 January 2008, 22:56 GMT
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.
Comment by Dave Chapman (linuxstb) - Thursday, 10 January 2008, 00:51 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 January 2008, 01:45 GMT
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
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 January 2008, 07:11 GMT
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...)
Comment by Linus Nielsen Feltzing (linusnielsen) - Thursday, 10 January 2008, 08:58 GMT
Why do you create newlist.h/c instead of changing list.c/h?
Comment by Travis Tooke (tdtooke) - Thursday, 10 January 2008, 09:03 GMT
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?
Comment by Travis Tooke (tdtooke) - Thursday, 10 January 2008, 09:41 GMT
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.
Comment by Travis Tooke (tdtooke) - Thursday, 10 January 2008, 09:44 GMT
Sorry for the triple post, but also not having a grandparent would basically take away backdrops.
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 January 2008, 17:34 GMT
Linus, dont worry, they wont be there when I'm finished.
Travis, ?
Comment by Michael Hahn (disorganizer) - Thursday, 10 January 2008, 22:28 GMT
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).
Comment by Travis Tooke (tdtooke) - Friday, 11 January 2008, 01:29 GMT
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.
Comment by Travis Tooke (tdtooke) - Friday, 11 January 2008, 12:04 GMT
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.
Comment by Sacha (Angyman) - Sunday, 13 January 2008, 12:01 GMT
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.
Comment by Dave (dberg918) - Monday, 14 January 2008, 18:27 GMT
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?
Comment by Sasha Khamkov (sanusart) - Thursday, 17 January 2008, 02:05 GMT
It might be incorrect width of the %V tag causing the line e.g. glitching to the album art as the volume changes.
Comment by Dave Chapman (linuxstb) - Thursday, 17 January 2008, 11:17 GMT

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.
Comment by Pascal Briehl (ColdSphinX) - Thursday, 17 January 2008, 12:22 GMT
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.
Comment by ApooMaha (crzyboyster) - Saturday, 26 January 2008, 17:24 GMT
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...
Comment by robin (robin0800) - Sunday, 27 January 2008, 21:39 GMT
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< |>
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 15:30 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Monday, 04 February 2008, 16:57 GMT
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.
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 20:11 GMT
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. >-)
Comment by Thomas Martitz (kugel.) - Monday, 04 February 2008, 20:17 GMT
Where do you get hunk errors? I can apply in just fine!
Comment by Jonathan Gordon (jdgordon) - Monday, 04 February 2008, 20:35 GMT
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...
Comment by Michael Hahn (disorganizer) - Monday, 04 February 2008, 21:39 GMT
@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.
Comment by Travis Tooke (tdtooke) - Thursday, 07 February 2008, 11:19 GMT
@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?
Comment by Johannes Voggenthaler (ZincAlloy) - Monday, 18 February 2008, 15:58 GMT
numeric names sounds fine to me and seems to make perfect sense.
Comment by Jonathan Gordon (jdgordon) - Monday, 18 February 2008, 17:02 GMT
I really would like to hear a sane argument on how having a number represent a font makes and sense what so ever?
Comment by Sasha Khamkov (sanusart) - Monday, 18 February 2008, 21:15 GMT
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.
Comment by Michael Hahn (disorganizer) - Tuesday, 19 February 2008, 09:24 GMT
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.
Comment by Johannes Voggenthaler (ZincAlloy) - Tuesday, 19 February 2008, 16:07 GMT
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.
Comment by Travis Tooke (tdtooke) - Wednesday, 20 February 2008, 14:43 GMT
@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.
Comment by Ben Zavala (bzavala) - Thursday, 21 February 2008, 18:40 GMT
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!
Comment by Johannes Voggenthaler (ZincAlloy) - Friday, 22 February 2008, 15:29 GMT
that wouldn't be viewport specific, would it? in my opinion it's a general wps feature.
Comment by Jonathan Gordon (jdgordon) - Saturday, 23 February 2008, 00:08 GMT
Ben, why did you reply with that message again? once is enough.. and no its not possible
Comment by Ben Zavala (bzavala) - Saturday, 23 February 2008, 03:24 GMT
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...!
Comment by Michael Hahn (disorganizer) - Thursday, 28 February 2008, 15:59 GMT
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, .....).
Comment by ApooMaha (crzyboyster) - Saturday, 08 March 2008, 03:26 GMT
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?
Comment by Michael Hahn (disorganizer) - Sunday, 09 March 2008, 13:50 GMT
hopefully synced to 16589 without any functional changes.

Comment by ApooMaha (crzyboyster) - Friday, 14 March 2008, 20:41 GMT
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?
Comment by Thomas Martitz (kugel.) - Saturday, 15 March 2008, 17:11 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 16 March 2008, 10:03 GMT
@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?
Comment by Jonathan Gordon (jdgordon) - Sunday, 16 March 2008, 10:17 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 16 March 2008, 11:49 GMT
@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.
Comment by Thomas Martitz (kugel.) - Sunday, 16 March 2008, 13:00 GMT
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.
Comment by Michael Hahn (disorganizer) - Sunday, 16 March 2008, 14:59 GMT
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).
Comment by ApooMaha (crzyboyster) - Sunday, 16 March 2008, 17:28 GMT
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.
Comment by Michael Hahn (disorganizer) - Sunday, 16 March 2008, 19:18 GMT
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.
Comment by Thomas Martitz (kugel.) - Sunday, 16 March 2008, 20:15 GMT
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?
Comment by Paul Louden (Llorean) - Sunday, 16 March 2008, 20:35 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 16 March 2008, 21:06 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 16 March 2008, 22:22 GMT
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.
Comment by Dave Chapman (linuxstb) - Sunday, 16 March 2008, 23:20 GMT
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...
Comment by ApooMaha (crzyboyster) - Monday, 17 March 2008, 00:06 GMT
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.
Comment by Michael Hahn (disorganizer) - Monday, 17 March 2008, 18:41 GMT
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*
Comment by Michael Hahn (disorganizer) - Monday, 17 March 2008, 21:56 GMT
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.
Comment by Dave Chapman (linuxstb) - Wednesday, 19 March 2008, 00:09 GMT
A small update to fix images displayed by the %x tag - thanks to PaulJam for reporting this in IRC.

Comment by Clément Pit--Claudel (CFP) - Thursday, 20 March 2008, 15:55 GMT
Tested successfully on gigabeat f60 with last svn and provided theme
Comment by Dave Chapman (linuxstb) - Friday, 21 March 2008, 17:51 GMT
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...