Rockbox

Hi,
i tried the patch and noticed the following things need fixing:

-WPS when the statusbar is shown (%we) and the .wps uses text in the main viewport (for example the hardcoded default WPS and the zezayer rWPS). The text is drawn 8 pixel to high, so the statusbar overlaps the first line.
-included WPS that use %m tags (Cabbiev2). ( FS#9027  will have to be committed first to be able to fix them properly)
-recording screen (i guess this isn’t a surprise).

ok, first point is fixed, 2nd point has been made a dependancy
ill get to the rest of the screens later today or on the weekend

   remove_lcd_margins.2.diff (41.6 KiB)

with the 2. version of the patch there are still some issues:

- Failing hunk with current SVN because of the HAS_BUTTONBAR to HAVE_BUTTONBAR conversion (see http://svn.rockbox.org/viewvc.cgi/trunk/apps/screen_access.h?r1=17654;r2=17655;pathrev=17655 )

- The hardcoded default WPS reserves the space for the statusbar even when it is not shown (disabled via settings).

- when the WPS doesn’t specify if the Statusbar is shown or not (no %wd or %we tag) and when the statusbar is enabled via settings, then the statusbar still overlaps the first line of the WPS.

hopefully this one fixes those problems

   remove_lcd_margins.3.diff (44.6 KiB)

this fixes the statusbar flashing problem and also fixes the recording screen (I think…. please test)

   remove_lcd_margins.4.diff (48.7 KiB)

fix the problem where the volume and gain settings would be shifted if the line selector was the bar

   remove_lcd_margins.5.diff (49.9 KiB)

this is remove_lcd_margins.5.diff and cond_viewports.9b.diff merged to make sure they play nice together… please test

   margins_condvp_merged.1.diff (75.6 KiB)

I’m getting patch errors … below is a snippet:

/home/bpanesar/rockbox-r17647/build-x5/apps/gui/textarea.o: In function `gui_textarea_clear’:
textarea.c:(.text+0×38): undefined reference to `screen_set_ymargin’ /home/bpanesar/rockbox-r17647/build-x5/apps/recorder/radio.o: In function `scan_presets’:
radio.c:(.text+0xc9c): undefined reference to `screen_set_xmargin’ /home/bpanesar/rockbox-r17647/build-x5/apps/recorder/radio.o: In function `radio_screen’:
radio.c:(.text+0xd5a): undefined reference to `screen_set_xmargin’ radio.c:(.text+0×1092): undefined reference to `screen_set_xmargin’ radio.c:(.text+0x10e4): undefined reference to `screen_set_xmargin’ radio.c:(.text+0x12a0): undefined reference to `screen_set_xmargin’ collect2: ld returned 1 exit status
make[1]: * [/home/bpanesar/rockbox-r17647/build-x5/apps/rockbox.elf] Error 1
make: * [build] Error 2

Bob: r17647 is too old. Update your source code and try again.

sync and merge again…

   margins_condvp_merged.2.diff (69.4 KiB)

I’m builing win32 sims for every target with this and  FS#9027  (merged) … http://jdgordon.info:8080/~jonno/rockbox/wpschanges/

new version time… 24 viewports (part of 9027 which i wont bother updating tonight), converted the %m tags for cabbie in the mini, h105/6g, c200 WPS’s.
I’ve also triggered another build for my windows sims if anyone will test them out.
(also fixed the charcell build error)

   wpschanges.diff (27 KiB)
   margins_condvp_merged.3.diff (96.7 KiB)

fixed some more cabbie wps’

   wpschanges.diff (28.6 KiB)

I only used the conditional viewports patch so far and now wanted to try the merged patches. “Un”patched the former and patched margins_condvp_merged.3.diff which worked fine (updated to r17715) but compiling for my c200 fails with the following errors:

misc.c: In function ‘parse_list’:
misc.c:1341: warning: implicit declaration of function ‘printf’ … gui/wps_parser.c: In function ‘parse_progressbar’:
gui/wps_parser.c:847: warning: passing argument 2 of ‘parse_list’ from incompatible pointer type
… /1_builds/c200/apps/misc.o: In function `parse_list’:
misc.c:(.text+0x190c): undefined reference to `printf’ collect2: ld returned 1 exit status

I don’t think I did anything wrong, even tried a make clean and reconfigure.
Edit: Building a c200 sim gives me the 2 warnings but doesn’t fail.

arg, woops, left a printf in there… remove that line in misc.c and it will compile… ignore the other warning

Whoa… took me ages to find out why my wps didn’t work. It was not (as I thought first) in the code I added or removed to get rid of the %m in it but in my progress bar definitions. Coming from an older version of the conditional viewport patches (with the %pb changes) it took me a while to realise that I now need a - in case I don’t want to use a bmp… and the error message is not helpful in that case. It just says “can’t load <wpsfoldername>/” …

After fixing that it seems that the code changes seems to work well but didn’t have a look at the wps changes yet. Hope you tested a bit yourself in sims… ;)

This has probably to do with #9027 and with the %pb changes in it - but I’m currently testing this combined version here and noticed the following today with one of my WPSs that does not have the progressbar in its own viewport:

the line with the %pb tag is not interpreted as a blank line as before so that every text that follows below is one line up compared to SVN and in the worst case you have text in the same position as the progressbar. This is probably not a problem for new WPSs as you can freely position the progressbar with a y-coordinate but it would change existing and shipped WPSs, especially cabbiev2. The question is whether to restore the old behaviour or just insert an additional blank line in the affected WPSs…

P.S.: Just tried cabbiev2 in a M5 sim - and it doesn’t seem to be a problem in cabbiev2 because the line below the progressbar is in its own viewport but why should it be only displayed when cover art is present?? And lots of things are off in other shipped themes (didn’t have the time to see whether I can find what’s happening).
*EDIT* it almost looks like the %pb only tag (used in iAmp, DancePuffDuo, some others, calculates its position using the sysfont height and not the user font height as before). In iCatcher the y-position parameter in the %pb tag has a - (all wrt the 160x128x2 WPSs). Ah, I *guess* this should mean “use line position” then? Then the same applies to iCatcher what I said about iAmp and DancePuffDuo + have to say again that using a blank parameter (even marked with -) often make the code harder to understand…

Another thing that irritated me about this patch - loading a WPS that contains %m is possible it isn’t rejected as in other cases of syntax errors - the defined margin is just ignored.