Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface → Themes
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by gl.tter - 2006-03-05

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

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.

Closed by  midgey34
2007-02-16 02:24
Reason for closing:  Accepted

Note only one file is affected, gwps-common.c.

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).

Project Manager

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?

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.

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).

re. the abitrary conditional enums, I’ve started expanding this now and will post the update here once complete.

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 

mmohr commented on 2006-03-09 18:15

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?

  1. 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).

  1. 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!??

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 .

mmohr commented on 2006-03-13 07:21

Any progress?
Especially to extract the progressbar code and separate it from the other stuff?

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.

Updated to work with current CVS source.
It is now possible for the patch to get commited to CVS?

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

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?! :

Updated for current CVS.

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

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

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).

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.

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).

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

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

Could you try adding just this patch to a CVS build to see if it still happens ?

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

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

version with the “old” parsing strategy to stay backward
compatible with old wps code. So the finishing “|” may be missing

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.

but the parsing could be changed to detect that too
(if no y coord was specified) independent if a final “|” is present or not

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).

agreed :-)

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

Could you be a little more specific ?
i.e. what do you mean by “correct” and “not correct”, and also “different locations” ?

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 ;-)

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 ?

I committed the Y-coord-rewrite patch to SVN.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing