Rockbox

Tasklist

FS#9051 - remove lcd margins

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Thursday, 29 May 2008, 15:01 GMT
Last edited by Jonathan Gordon (jdgordon) - Monday, 23 June 2008, 13:21 GMT
Task Type Patches
Category LCD
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 removes the x/y lcd margins from everywhere.. to replicate the old behaviour you have to use viewports.

please test and let me know which screens need aditional fixing...
Closed by  Jonathan Gordon (jdgordon)
Monday, 23 June 2008, 13:21 GMT
Reason for closing:  Accepted
Comment by PaulJam (PaulJam) - Thursday, 29 May 2008, 16:49 GMT
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).
Comment by Jonathan Gordon (jdgordon) - Friday, 30 May 2008, 01:45 GMT
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
Comment by PaulJam (PaulJam) - Friday, 30 May 2008, 11:23 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Monday, 02 June 2008, 14:23 GMT
hopefully this one fixes those problems
Comment by Jonathan Gordon (jdgordon) - Tuesday, 03 June 2008, 08:41 GMT
this fixes the statusbar flashing problem and also fixes the recording screen (I think.... please test)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 03 June 2008, 09:57 GMT
fix the problem where the volume and gain settings would be shifted if the line selector was the bar
Comment by Jonathan Gordon (jdgordon) - Wednesday, 04 June 2008, 09:16 GMT
this is remove_lcd_margins.5.diff and cond_viewports.9b.diff merged to make sure they play nice together... please test
Comment by Bob (viperman3) - Wednesday, 04 June 2008, 21:21 GMT
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+0x38): 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+0x1092): 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
Comment by PaulJam (PaulJam) - Thursday, 05 June 2008, 08:31 GMT
Bob: r17647 is too old. Update your source code and try again.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 05:17 GMT
sync and merge again...
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 11:37 GMT
I'm builing win32 sims for every target with this and  FS#9027  (merged) ... http://jdgordon.info:8080/~jonno/rockbox/wpschanges/
Comment by Jonathan Gordon (jdgordon) - Wednesday, 11 June 2008, 14:24 GMT
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)
Comment by Jonathan Gordon (jdgordon) - Thursday, 12 June 2008, 09:35 GMT
fixed some more cabbie wps'
Comment by Marianne Arnold (pixelma) - Thursday, 12 June 2008, 15:48 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Friday, 13 June 2008, 00:16 GMT
arg, woops, left a printf in there... remove that line in misc.c and it will compile... ignore the other warning
Comment by Marianne Arnold (pixelma) - Friday, 20 June 2008, 22:41 GMT
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... ;)
Comment by Marianne Arnold (pixelma) - Sunday, 22 June 2008, 21:56 GMT
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.

Loading...