Rockbox

Tasklist

FS#9027 - conditional viewports

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Thursday, 22 May 2008, 15:01 GMT
Last edited by Jonathan Gordon (jdgordon) - Monday, 23 June 2008, 06:05 GMT
Task Type Patches
Category Themes
Status Closed
Assigned To No-one
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

this patch adds the ability to en/disable viewports based on conditionals..

how it works is that viewports can now take an optional paramater at the end which is a letter 'a' to 'a' + max amount of viewports.. (so a-p atm)

that letter is then used with %Ve to tell the wps to display that viewport. (viewporets with the extra letter are assumed hidden unless %Ve is passed)


quick example .wps....

%wd
%?mp<Stop|Play %Vea|Pause %Veb|Ffwd|Rew>
%V|6|50|100|20|1|ffffff|000000|a|
%ia

%V|6|100|100|20|1|ff0aff|000000|b|
%ia

what that does is just display the artists title in a different viewport if music is paused or playing...

CAVEAT:
you cant have the %Ve token inside the viewport you want to enable (e.g"
%V|.....|a|
%Vea
")

This means that if you want to check the condition inside the vp you need to check it BEFORE the %V line... I guess its good practive to make sure you have all your %Ve's done before the first %V line.


one more thing (which is why the code is a bit messy).
I've tried to make it so it wont redraw the static bits of enabled viewports unless its just been re-enabled, it seems to work, but I havnt tested to make sure its not doing extra redraws... (if this does work, maybe we shuold make it genereic and use it for the images so extra redraws arnt done there also?)

(edit: changed the diff because it had WPS_REFRESH_STATIC instead of WPS_REFRESH_DYNAMIC for %Ve)
This task depends upon

View Dependency Graph

This task blocks these from closing
 FS#9051 - remove lcd margins 
Closed by  Jonathan Gordon (jdgordon)
Monday, 23 June 2008, 06:05 GMT
Reason for closing:  Accepted
Comment by PaulJam (PaulJam) - Thursday, 22 May 2008, 16:25 GMT
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).
Comment by Jonathan Gordon (jdgordon) - Friday, 23 May 2008, 01:24 GMT
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...
Comment by Dave Chapman (linuxstb) - Saturday, 24 May 2008, 10:43 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Saturday, 24 May 2008, 14:14 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Saturday, 24 May 2008, 15:35 GMT
done and done...
Comment by Jonathan Gordon (jdgordon) - Monday, 26 May 2008, 13:13 GMT
some minor code changes, %Ve is changed to %Vd
Comment by Jonathan Gordon (jdgordon) - Sunday, 01 June 2008, 09:14 GMT
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
Comment by Jonathan Gordon (jdgordon) - Monday, 02 June 2008, 11:24 GMT
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
Comment by Jonathan Gordon (jdgordon) - Monday, 02 June 2008, 11:32 GMT
and this is multi-pb.diff merged with cond_viewports.5.diff and actually compliing
Comment by Jonathan Gordon (jdgordon) - Tuesday, 03 June 2008, 09:39 GMT
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
Comment by Jonathan Gordon (jdgordon) - Tuesday, 03 June 2008, 09:48 GMT
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
Comment by Erkka Kettunen (Siku) - Tuesday, 03 June 2008, 18:26 GMT
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.
   Test.wps (0.6 KiB)
Comment by Marianne Arnold (pixelma) - Tuesday, 03 June 2008, 22:13 GMT
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).
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 June 2008, 05:36 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 June 2008, 07:19 GMT
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
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 June 2008, 08:12 GMT
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
Comment by Erkka Kettunen (Siku) - Wednesday, 04 June 2008, 16:54 GMT
Jonathan: Thanks! That fixed the problem.
Comment by Jonathan Gordon (jdgordon) - Thursday, 05 June 2008, 07:24 GMT
cabbiev2 for the c200 updated to not use %m.
I think it works... not 100% sure
Comment by Jonathan Gordon (jdgordon) - Thursday, 05 June 2008, 13:02 GMT
resync
Comment by Erkka Kettunen (Siku) - Thursday, 05 June 2008, 16:30 GMT
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? :)
Comment by Jonathan Gordon (jdgordon) - Saturday, 07 June 2008, 12:53 GMT
resync and fix that boo boo so %V works again
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 06:25 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 06:49 GMT
and minor update... merge in the wpschanges.diff
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 08:01 GMT
removew the charcell wps from the diff.. no other changes
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 11:36 GMT
I'm builing win32 sims for every target with this and  FS#9051  ... http://jdgordon.info:8080/~jonno/rockbox/wpschanges/
Comment by Jacob Brooks (jac0b) - Monday, 16 June 2008, 21:50 GMT
@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
Comment by Jonathan Gordon (jdgordon) - Monday, 16 June 2008, 22:15 GMT
you got the syntax wrong..
%Vl|tag|x|y|.....
Comment by tomas (hakimio) - Wednesday, 18 June 2008, 07:40 GMT
Isn't it ready to commit yet?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 18 June 2008, 07:45 GMT
if it was, this would be closed....
itll go in sometime this weekend probably
Comment by Jonathan Gordon (jdgordon) - Sunday, 22 June 2008, 07:30 GMT
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

Loading...