Rockbox

Hi,

i have tried the patch and expreienced some problems. Attached is the test WPS i used.

It seems as if currently only the contents of the viewport are shown/hidden conditionally but the viewport itself is still there. I would have expected that the "inactive" Viewport would be entirely hidden, so that the area that isn't occcupied by other viewports shows the main viewport.
Another problem is when the viewports overlap, then the "inactive" viewport seems to be always shown in front of the active one (see the attached image).

hmm... yeah, I've had a bit of a play and cant see how to fix this. its mostly a problem with the order the viewports are redrawn.. this version makes it minutely better.. there isnt really anythignt hat can be done about it unless the entire screen is redrawn when a viewport changesm, and even that wont work perfectly...

I haven't looked at the patch itself yet, but have some comments about the idea itself. I basically like the idea though, and think it could work...

1) I think it's more logical to have the viewport ID near the start of the tag - to mirror the other WPS tags (e.g. %xl). We could perhaps use %Vl|a-z|... to define conditional viewports, meaning the original %V tag stays as it is. IMO this makes it clearer to WPS authors that the two types of viewports (static and conditional) work differently.

We could even allow WPS authors to group %Vl definitions - i.e. using the same letter for multiple viewports. So for example, a group of two viewports could be defined for the case that album art is present, and a single viewport for when there is no album art.

2) To display a group of viewports, %Vdn (where n is a-z) could be used (analogous to %xd).

3) Regarding the comments/problems about overlapping viewports, and clearing inactive viewports, I think we could just avoid that problem completely. i.e. WPS authors should be required to ensure that at all times, areas of the screen that are displayed by conditional viewports have one set of active viewports that completely cover that area.

this patch makes them work somewhet better (and implements point 3..). the only thing cleared in a hidden viewport is now its scrolling lines... so unless you want artifacts make sure your viewports are completly covered...
(attached is a really lame wps demonstrating)

reply to above points...
1) I'm not generally in favour of using i,L,l,I in tags, because too many fonts make them look too similar... but I dont really care what letter is used, so if you want to use l, then fine by me.. (coming in next version)
2) yes! that would be good... not sure how best to do this though, but it is definatly a good idea.

done and done...

some minor code changes, %Ve is changed to %Vd

This update fixes the problems with only allowing one progress bar on the WPS... now you can define up to 3, you are still only allowed one per viewport, but this should make conditional viewports a bit better.
The %P tag has been removed, so to specify a bitmap progressbar you need to use the %pb tag with all 4 number values, and then the final value is the filename... "%pb|8|5|171|182|pb-176x220x16.bmp|" One thing that should be dont is checking if the same file is used twice, in which case dont reload it...

multi-pb.diff is the changes for the progress bar... cond_viewports.5.diff is 4 merged with multi-pb

updated multi-pb.diff... the %pb syntax is now %pb|filename|x|y|width|height| each can be left blank to get a sane value (leaving out the filename uses the old style bar, missing x,y,width values use the viewport, height uses the font height)

apparentyl i stuffed up the merged patch so ill fix that now

and this is multi-pb.diff merged with cond_viewports.5.diff and actually compliing

quick update to allow the use of %pb by itself to get a progress bar using 1 line full width at the current line position

changed my mind on the default value for the y param if its not specified... it will now be put on the screen on the same line it is in the .wps

Nice work with the patch! I tested it and found that scrolling text isn't working inside a conditional viewport. Other than that, it's working as expected. See the attachment for an example WPS.

I tested this (and before commenting made sure this is still true using the cond_viewports.7.diff) with a possibly bit complicated project, but the problem also shows in a simplified version of it which I'll attach - you'll need a c200 sim (or target) to try out.

My idea was to use viewports with differently assigned colours and show them conditionally, e.g. low battery gets a different foreground colour, volume above 0dB etc. This also means that there are %Vd tags in all different sorts of conditionals. The problem I encountered was that sometimes lines in a viewport get cleared (only show the global background colour) when - how to explain... - a different part of the conditional is true, it doesn't necessarily mean different viewports should be displayed. As example: to get a different colour for battery level below 20%, I needed to fill the battery level conditional with 6 %Vds (unknown, 0-19% viewport, 20-39% vp, 40-59% vp, 60-79% vp, 80-100% vp) - the last 4 are actually the same viewports and I get the "blankening" mostly in a different viewport and sometimes even when the battery level changes from 80% to 79% etc. This is even more noticable with viewports changing on hold switch status.

In contrast to the above I didn't have problems with scrolling lines, it was quite the opposite: areas which were redrawn because of the scrolling "fixed" themselves and it seems the whole wps is redrawn on track change and so it looked as expected and wanted. I'm quite sure the viewports have the right position and size (tested out with an onlyviewport.wps with different background colours and tested coordinates and dimensions, could deliver that too if wanted.) There are no gaps between all the viewports, the default "full screen" viewport is always covered.

Attachments
- a picture with diverse screenshots (with explanation) of my full blown wps, hope that makes it easier to understand.
- a simplified test.wps of my idea which has empty viewports with different background colours and a different set of viewports is shown on hold - shows the blankening of the first line (height dependent on the font size used) in the upper left viewport

P.S.: I just noticed problems with the new %pb tag...
when I first used it, I still had some leftovers from the old syntax and probably had a completely wrong width, height or position and starting playback made the sim crash (only started the sim after changing the wps, so should have been there on load but didn't give me an error). Unfortunately I couldn't reproduce after correcting to use the new syntax and change some values to something completely off at will (one by one).
But also a reproducible problem: if I only use "%pb" without specifying anything the progressbar seems to be drawn with an offset of the statusbar height away from the viewport border it is in (here: should have been drawn on the first line of a 10 pixels high viewport and I only see the upper two pixel rows of the progressbar at the bottom of said viewport).

thanks guys.
Erkka: it seems the %?C conditional is broken... I changed that to %?mh and the scrolling works fine!
pixelma: the %pb y offset is a problem because this patch still assumes the initial viewport is at 0,0 even if the statusbar is being displayed. This wont be a problem when it gets commited because the remove lcd margins patch will take care of that... but untill then, I've added some code which will deal with that problem. (ill update the patch later in the day with the fix).
re your other problem... firstly that wps looks cool, but one quick request... please add some text to the .wps next time... its not immediatly obvious where each vp is supposed to be (i've added some here which makes debugging much easier).
I'm not sure why the line is disappearing but ill look into it.

last thing I want to add before going back to studying...

you can reuse the viewport identifiers so you only have to add one %Vd tag if you have a few viewports which are always displayed together.

hopefully fix the issue with %?C, although I tihnk the viewports will be redrawn more often than needed now... dont know for sure though.
also fix the default width value for %pb which was mentioned in the ml
and include that fix for the statusbar thing

workaround - fix - the line blanking problem. drawing is disabled in the default viewport if any other viewports are defined. hopefully not a major problem...

edit: woops, stuffed up that fix... make sure you have version .9b.diff

Jonathan: Thanks! That fixed the problem.

cabbiev2 for the c200 updated to not use %m.
I think it works... not 100% sure

resync

Was viewport syntax changed or is there something missing in the latest patch? I'm getting error 'Failed parsing on line 11 : Invalid parameter list for token 8: "No token"' if I use my previously attached 'Test.wps' in a simulator. Or is it just me missing something? :)

resync and fix that boo boo so %V works again

attached is the diff of all the .wps files in svn converted to the new %pb syntax and the perl script I used to do it.

and minor update... merge in the wpschanges.diff

removew the charcell wps from the diff.. no other changes

I'm builing win32 sims for every target with this and  FS#9051  ... http://jdgordon.info:8080/~jonno/rockbox/wpschanges/

@jdgordon

If you use the peak meter in my conditional it is on top of my cover art which is the other part of the conditional.
%?C<%C %Vda|%Vdb>
%V|40|40|150|150|0|000000|FFFFFF|a|
%Cl|40|40|s150|s150|
%V|0|170|240|10|0|000000|FFFFFF|b|
%pm

you got the syntax wrong..
%Vl|tag|x|y|.....

Isn't it ready to commit yet?

if it was, this would be closed....
itll go in sometime this weekend probably

a few more minor changes.....
fixed the possibly last issue with the %pb but im not sure If the fix will stay....
in current svn %pb is used as a "text" line, so doing "a\n%pb|...|\nb\n" will use 3 screen lines... now that is 100% fine and understandable for %pb when it doesnt have any params... but im not so sure its correct if the params are used... I'm split between leaving this fix in and having %pb act the same in both cases (but maybe somewaht confusingly because %X doesnt do this...) and taking it out and only using the line if its plain %pb (or maybe also if the y value is left out....)

I'm off to a wedding tonight, but would really like to commit this tomorrow