Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#4783 - WPS Extensions: ProgressBar Y coord + Enum Conditional Limits Removal

Attached to Project: Rockbox
Opened by gl (gl.tter) - Monday, 06 March 2006, 00:21 GMT+2
Task Type Patches
Category Themes
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch contains two WPS extensions:

1) Progress Bars can be positioned with an optional y coordinate, instead having to be line based:

Use %pb|height|leftpos|rightpos|[y coord|]

2) Proof-of-concept arbitrary value-based enum-conditionals:

I wanted to use 15 bitmaps for my work-in-progress WPS' volume bitmap, instead of the hardcoded 10. My solution is for the code to count how many conditional enums a WPS is using, and work off that. This allows a WPS designer to use however many bitmaps etc they choose, without breaking backwards compatibility.

If somebody more familiar with the WPS internals wants to extend this to all suitable enum tags, please do (otherwise I'll do it eventually).

As this patch isn't meant for inclusion as-is, I have left my // gl.tter - START / END code tags in place to highlight the changes.
   wps_extensions.zip (2.1 KiB)
This task depends upon

Closed by  Tom Ross (midgey34)
Friday, 16 February 2007, 03:24 GMT+2
Reason for closing:  Accepted
Comment by gl (gl.tter) - Monday, 06 March 2006, 00:24 GMT+2
Note only one file is affected, gwps-common.c.
Comment by gl (gl.tter) - Monday, 06 March 2006, 00:56 GMT+2
Sorry, the progress bar was bugged & broken, and I should've split the patches:

1) Updated Progress Bar patch attached - this _is_ now ready for inclusion.

2) Seperate 'conditional enum limit' patch (with gl.tter tags).
   progressbar_ycoord_extension.zip (1.5 KiB)
   arbitrary_enum_conditionals.zip (1.1 KiB)
Comment by Linus Nielsen Feltzing (linusnielsen) - Monday, 06 March 2006, 01:06 GMT+2
1) Please don't ZIP the file, it just creates more work for us who review it

2) Don't add those "gl.tter" comments, we can see perfectly well what was changed without them. That's what patches are for.

3) I like the enum approach, and I'd like it extended to all enums. Can you do that?
Comment by gl (gl.tter) - Monday, 06 March 2006, 01:13 GMT+2
1. Sure thing Linus. Saves me time too.
2. I do remove them for patches suitable for inclusion, this was the exception as it was only supposed to demo the approach.
3. As I said, I can do this eventually, but if somebody with more WPS code experience wants to do it now, great.
Comment by gl (gl.tter) - Monday, 06 March 2006, 03:13 GMT+2
Changes to Progress Bar patch:

Bug Fix & func name change.
Now allows y=0
Now drawn after text (so it isn't hidden by it when it overlaps, confusing during design).


   progressbar_ycoord_extension2.patch (6 KiB) 
 apps/gui/gwps-common.c |   75 +++++++++++++++++++++++++++++++++----------------
 apps/gui/gwps.h        |    1 
 2 files changed, 52 insertions(+), 24 deletions(-)

Comment by gl (gl.tter) - Tuesday, 07 March 2006, 03:31 GMT+2
re. the abitrary conditional enums, I've started expanding this now and will post the update here once complete.
Comment by gl (gl.tter) - Wednesday, 08 March 2006, 12:25 GMT+2
I have finished the abitrary enum counts, progress bar y coord, and added a few more WPS features and fixes. New all-inclusive patch & demo now at:  FS#4802 
Comment by Matthias Mohr (aka Massa) (mmohr) - Thursday, 09 March 2006, 19:15 GMT+2
I tested "progressbar_ycoord_extension2.patch" - please tell me if it's obsolete.

Somewhere in the code you say if one is not using the new additional y-coordinate parameter,
it would behave as without the patch.

That's not true, if you don't use the y-coord parameter,
it displays your progress bar on top of the screen :-(

Another thing: I converted an existing WPS and just changed the progressbar line and added the addition parameter.
After that everything was on its place.
IMHO that's a bit illogical - you know why?
- when it works in line based mode (without y-coord parameter) then it should add an additional line and
follow the order of the lines (and "eat" a line).
- when it works in coordinate mode, it should work like the bitmaps do - not line oriented with direct position
and it should not affect other tags which are line oriented --> it should NOT "eat" a line.
==> to make a long story short: when I added the y-coord parameter, I expected that I also would have to add an
additional empty line because the progress bar is no longer part of the line oriented parsing and therefore the
I exepected that the whole text after the progress bar would jump a line above.
That did not happen!??
Comment by gl (gl.tter) - Friday, 10 March 2006, 00:37 GMT+2
Hi Mat, I've made some changes since that patch. My current code (see below for link) displays the traditional progress bars correctly on all the WPS' that ship with Rockbox.

But you're right, it does eat a line in y coord mode. I'll fix that and post a seperate PB patch there shortly:  FS#4802 .
Comment by Matthias Mohr (aka Massa) (mmohr) - Monday, 13 March 2006, 08:21 GMT+2
Any progress?
Especially to extract the progressbar code and separate it from the other stuff?
Comment by gl (gl.tter) - Monday, 13 March 2006, 10:02 GMT+2
No Matt, I'm withdrawing my patches (see  FS#4802  for the reason).

I did take a look at it eating a line though, but it's more complicated - %x| tags don't take a line, but %xd tags do. You could change y PBs to work like %x| tags, but then you couldn't use them in a conditional or subline, which can be useful. Or the WPS line formatting code could be rewritten to skip lines that only contain images or y PBs - but that's another issue.
Comment by Alexander Bondar (davinci) - Wednesday, 05 April 2006, 21:49 GMT+2
Updated to work with current CVS source.
It is now possible for the patch to get commited to CVS?
(application/octet-stream)    progressbar_ycoord_extension2.1.patch (6.1 KiB) 
 apps/gui/gwps-common.c |   75 +++++++++++++++++++++++++++++++++----------------
 apps/gui/gwps.h        |    1 
 2 files changed, 52 insertions(+), 24 deletions(-)

Comment by Norbert Preining (norbusan) - Monday, 05 June 2006, 09:54 GMT+2
Hi all! I am compiling builds for iriver h300 including some patches, and I also include this patch. But several of the users complain about that with some wps the progressbar is at the top of the screen. AFAIS this should have been fixed, but I get so many complaints (and I have fixed the theme I am using myself) so I assume it is not fixed for themese not using the y coordinate.
Are there any news on this?
Thanks a lot and all the best
Norbert
Comment by Nati (Kitt0s) - Monday, 13 November 2006, 06:46 GMT+2
Quote by NicolasP:
Progressbar Y coord hasn't been commited because it's author refuses to give his real name, which is against the project's policy.


all you need is give him your name?! :\\
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 03 January 2007, 06:09 GMT+2
Updated for current CVS.
   progressbar_ycoord_extension2.1_20070103.patch (6.2 KiB) 
 apps/gui/gwps-common.c |   78 ++++++++++++++++++++++++++++++++-----------------
 apps/gui/gwps.h        |    1 
 2 files changed, 53 insertions(+), 26 deletions(-)

Comment by Norbert Preining (norbusan) - Thursday, 04 January 2007, 00:26 GMT+2
Hi Nicolas!
Could it be that this update broke (old) wps without the y coordinate specified? I got one report concerning this, and I tested myself some wps (eg UniCatcher), and the pb actually was tucked to the top.

Prior version did work.

Any ideas what it could be?

Best wishes

Norbert
Comment by Norbert Preining (norbusan) - Thursday, 04 January 2007, 01:47 GMT+2
Hi Nicolas!
Ok, it seems to be something different. Maybe the combination of cuesheet parser (which makes a bit of magic in the gwps-common.c) and the ycoord patch? I reverted to the previous version and had the same problems. Sorry for the noise. Maybe I tell the guys simply to adjust their wps...

Bye Norbert
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 04 January 2007, 04:34 GMT+2
Norbert: The cuesheet patch doesn't apply quite cleanly with this one : the code that displays the cue markers in the progressbar (which should be rejected when you patch cuesheet over pb_y_coord) should be moved to the function that calls ab_draw_markers(), just after that call. Also there is a variable name that needs to be changed (sb_y becomes y).
Comment by Norbert Preining (norbusan) - Thursday, 04 January 2007, 11:59 GMT+2
Hi Nicolas: Thanks a lot. The markers are now drawn correct. I have seen the reject and patched it into the same place as before, right after the draw_progressbar, thought it might work. Now I have moved it to the draw_progress_bar function and the marks are shown right.
Still other problem persist, see my other post.
Comment by Nicolas Pennequin (nicolas_p) - Friday, 05 January 2007, 16:40 GMT+2
I rewrote this patch from scratch. It should work exactly the same but with full backwards compatibility (no more progressbar at the top of the screen on WPS that don't give the y-coordinate). I had to make some changes to the parsing of the %pb% tag to obtain this but it should work better now. Also I think it's a simpler way of doing the same thing (less big changes in the code than the other patch).
   progressbar_ycoord_rewrite.patch (4.4 KiB) 
 apps/gui/gwps-common.c |   61 ++++++++++++++++++++++++++++---------------------
 apps/gui/gwps.h        |    1 
 2 files changed, 37 insertions(+), 25 deletions(-)

Comment by Norbert Preining (norbusan) - Friday, 05 January 2007, 17:19 GMT+2
Hi Nicolas!
Thanks a lot. I have made a first build with your patch and have a lot of fun: The WPS screen is not redrawn. But this might be a problem from a different patch. Menus and file lists are shown but the wps is never shown after I have gone once into file list or settings screen. Hmm, I think I have to repatch everything from the beginning and see what happens.

Anyway, thanks a lot. Maybe now this patch can be included, since it is a rewrite and your name is known ;-)

Bye Norbert
Comment by Norbert Preining (norbusan) - Friday, 05 January 2007, 17:24 GMT+2
Ok, to be specific. It is redrawn, but after quite a long time, I don't know what triggers it. Furthermore the progressbar I have in my myPod-H wps has no frame anymore, only the bar itself.

I will do more tests as soon as I have time ...

Bye
Comment by Nicolas Pennequin (nicolas_p) - Friday, 05 January 2007, 17:36 GMT+2
Could you try adding just this patch to a CVS build to see if it still happens ?
Comment by Norbert Preining (norbusan) - Friday, 05 January 2007, 17:39 GMT+2
Hi NIc!
All well, I left out the custom stuff and everything is working perfectly. I tested some wps which didn't work before, now they do.
Thanks a lot!!! Go for inclusion ;-)

Now for the hard work to get the custom stuff working again...

Ciao
Comment by Max Weninger (maxwen) - Saturday, 06 January 2007, 15:46 GMT+2
One small issue about the new y-coord parsing.
In most "historic" wps I have the last finishing "|" is not
there (e.g. %pb|2|48|115|83) which
leads to a wrong y value since the parsing depends on it

Dont know if the wps writer did an error in the first place
by not providing the final "|" or if the parsing should
be "smart" enough to handle this case also.

The old patch did it that way
Comment by Max Weninger (maxwen) - Saturday, 06 January 2007, 16:07 GMT+2
version with the "old" parsing strategy to stay backward
compatible with old wps code. So the finishing "|" may be missing
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 06 January 2007, 18:14 GMT+2
Actually, allowing the last pipe to be omitted was precisely the reason why the progressbar was shown at the top of the WPS with no y-coord specified (atoi() returns 0). Making it compulsory seems to be the only way to avoid this issue, and anyway it was never supposed to be optional (which is consistent with all the other WPS tags). I suggest you tell WPS creators to correct their WPS, and I'm deleting your patch to avoid confusion with the correct one.
Comment by Max Weninger (maxwen) - Saturday, 06 January 2007, 18:28 GMT+2
but the parsing could be changed to detect that too
(if no y coord was specified) independent if a final "|" is present or not
Comment by Nicolas Pennequin (nicolas_p) - Saturday, 06 January 2007, 18:42 GMT+2
Well personally I don't really see how it could be done without significant changes to the parsing code, so I'm not sure it's worth it. All that's currently needed is a pipe at the end of a line on a few WPS, so I think it's easier to just ask WPS creators to respect the specified format (which has always been "%pb|height|leftpos|rightpos|[y coord|]" by the way, notice the final pipe).
Comment by Max Weninger (maxwen) - Saturday, 06 January 2007, 22:42 GMT+2
agreed :-)
Comment by Max Weninger (maxwen) - Friday, 12 January 2007, 18:59 GMT+2
FYI: I just discovered that the rewrite patch is not working
correct for some WPS e.g. ZenPod.

The old patch called the progress draw on a different location
for an y-coord enabled progress bar maybe that is the reason.

If I change the new patch so that it is called like the old
patch the progress bar is drawn correct for all WPS again
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 16 January 2007, 02:35 GMT+2
Could you be a little more specific ?
i.e. what do you mean by "correct" and "not correct", and also "different locations" ?
Comment by Max Weninger (maxwen) - Tuesday, 16 January 2007, 12:55 GMT+2
I mean the location in the code in gui_wps_refresh
The old patch calls the progress bar drawing at the end of the function
for y-coord enabled progress bars

Not correct means that the progress bar is not drawn with your rewrite
patch ;-)
Comment by Nicolas Pennequin (nicolas_p) - Monday, 22 January 2007, 14:21 GMT+2
maxwen: I tested the ZenPod WPS one the X5 sim and found no problem whatsoever. Are you sure your problem is not related to another patch ? If not, could you send me a WPS i can test ?
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 15 February 2007, 23:46 GMT+2
I committed the Y-coord-rewrite patch to SVN.

Loading...