Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11435 - Use the new skin parser in rockbox!

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Thursday, 24 June 2010, 11:59 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Sunday, 11 July 2010, 08:45 GMT+2
Task Type Patches
Category Battery/Charging
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch changes the core to use the new skin parser..

You should not see any difference... let me know if you do
   newparser.0.diff (41.4 KiB) 
 b/apps/SOURCES                         |    1 
 a/apps/gui/skin_engine/skin_buffer.c   |  170 ----------
 a/apps/gui/skin_engine/skin_buffer.h   |   64 ---
 b/apps/gui/skin_engine/skin_parser.c   |  550 +++++++++++++++++++++++++--------
 b/apps/gui/skin_engine/wps_debug.c     |    2 
 b/apps/gui/skin_engine/wps_internals.h |    4 
 b/apps/gui/theme_settings.c            |   27 +
 b/apps/menus/main_menu.c               |    1 
 b/lib/skin_parser/skin_buffer.c        |   31 +
 b/lib/skin_parser/skin_buffer.h        |    9 
 b/lib/skin_parser/skin_parser.c        |    5 
 b/lib/skin_parser/skin_parser.h        |    6 
 b/lib/skin_parser/skin_scan.c          |    7 
 b/lib/skin_parser/skin_scan.h          |    1 
 14 files changed, 487 insertions(+), 391 deletions(-)

This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Sunday, 11 July 2010, 08:45 GMT+2
Reason for closing:  Out of Date
Comment by Thomas Martitz (kugel.) - Thursday, 24 June 2010, 12:55 GMT+2
You didn't change the displayer, right? Do you think it could be a good idea to rework the displayer too? I haven't looked at the patch, but I assume you need to convert the parse tree into the old array like system so legacy displayer can work with it?
Comment by Jonathan Gordon (jdgordon) - Thursday, 24 June 2010, 13:01 GMT+2
I made sure to not touch the displayer.

I originally wanted to replace the entire engine in one hit but that turned out to be way too huge of a task.

This patch converts te tree to the array, the next step is to replace all the parse_X functions with ones which directly use the preparsed args, then slowly replace the displayer to work with the tree.
Comment by Jonathan Gordon (jdgordon) - Thursday, 24 June 2010, 13:15 GMT+2
This patch goes onto of the above one o fix the playlist viewer in the wps to use the tree directly... seems to work :p please try both. (most bugs will probably be in the first one only thugh). BE CAREFUL to not put any tags in the Vp that arnt supported yet.. very minimal error checking so far...
   newplaylistviewer.0.diff (11.2 KiB) 
 b/apps/gui/skin_engine/skin_display.c  |   35 ++++----
 b/apps/gui/skin_engine/skin_parser.c   |  135 ++++++---------------------------
 b/apps/gui/skin_engine/wps_internals.h |    7 -
 3 files changed, 46 insertions(+), 131 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Thursday, 24 June 2010, 13:48 GMT+2
put it on my d2 and skin usage and boot time is both up (hardly surprising really seen as there is an extra step).. I might boost to fix the boot time issue
Comment by Jonathan Gordon (jdgordon) - Thursday, 24 June 2010, 14:00 GMT+2
new version so all the plugins build
   newparser.1.diff (54.8 KiB) 
 b/apps/SOURCES                         |    1 
 a/apps/gui/skin_engine/skin_buffer.c   |  170 ------
 a/apps/gui/skin_engine/skin_buffer.h   |   64 --
 b/apps/gui/skin_engine/skin_parser.c   |  833 ++++++++++++++++-----------------
 b/apps/gui/skin_engine/wps_debug.c     |    2 
 b/apps/gui/skin_engine/wps_internals.h |    5 
 b/apps/gui/theme_settings.c            |   27 +
 b/apps/gui/viewport.c                  |   77 ---
 b/apps/gui/viewport.h                  |   21 
 b/apps/menus/main_menu.c               |    1 
 b/lib/skin_parser/skin_buffer.c        |   31 -
 b/lib/skin_parser/skin_buffer.h        |    9 
 b/lib/skin_parser/skin_parser.c        |    5 
 b/lib/skin_parser/skin_parser.h        |    6 
 b/lib/skin_parser/skin_scan.c          |    7 
 b/lib/skin_parser/skin_scan.h          |    1 
 16 files changed, 484 insertions(+), 776 deletions(-)

Comment by Michael Chicoine (mc2739) - Thursday, 24 June 2010, 17:39 GMT+2
testing newparser.1.diff with r27106 on e200v2 and cabbiev2

Vertical spacing is not correct. All lines are one line higher than svn. It seems to be caused by the "#Now Playing" comment line. With the comment line removed, it displays correctly.

Comment by Jonathan Gordon (jdgordon) - Friday, 25 June 2010, 06:44 GMT+2
OK, seems I was handling comments wrongly.. try this one. newplaylistivewier.0.diff should apply cleanly to this still

edit: updated again to hopefully make %ax work
   newparser.2.diff (54.9 KiB) 
 b/apps/SOURCES                         |    1 
 a/apps/gui/skin_engine/skin_buffer.c   |  170 ------
 a/apps/gui/skin_engine/skin_buffer.h   |   64 --
 b/apps/gui/skin_engine/skin_parser.c   |  834 ++++++++++++++++-----------------
 b/apps/gui/skin_engine/wps_debug.c     |    2 
 b/apps/gui/skin_engine/wps_internals.h |    5 
 b/apps/gui/theme_settings.c            |   27 +
 b/apps/gui/viewport.c                  |   77 ---
 b/apps/gui/viewport.h                  |   21 
 b/apps/menus/main_menu.c               |    1 
 b/lib/skin_parser/skin_buffer.c        |   31 -
 b/lib/skin_parser/skin_buffer.h        |    9 
 b/lib/skin_parser/skin_parser.c        |    5 
 b/lib/skin_parser/skin_parser.h        |    6 
 b/lib/skin_parser/skin_scan.c          |    7 
 b/lib/skin_parser/skin_scan.h          |    1 
 16 files changed, 485 insertions(+), 776 deletions(-)

Comment by Michael Chicoine (mc2739) - Friday, 25 June 2010, 21:19 GMT+2
newparser.2.diff does fix the comment problem.

New Issues:
Using: Wide Cabbie theme on e200v2 (http://themes.rockbox.org/download.php?themeid=1)

1. Some viewports do not display. These vps do not have any %Vf() or %Vb(). Adding, at a minimum, %Vf(CCCCCC) to the vp definition corrects the problem. It looks like vps with images only do not have a problem, only vps that display text info.
2. When album art is available, this theme has a display line which consists of two viewports. The first vp has fixed text "Next:" and the second vp scrolls "Artist - Title". The second vp definition uses "-" for the width. This causes the second the second vp to be 2 pixels higher than defined and overwrite the first vp. Replacing the "-" with the remaining screen width corrects this problem.


I am attaching screendumps of the incorrect and correct wps displays and a diff of the original wps file and the updated wps file.
   no-aa-bad.bmp (75.7 KiB)
   no-aa-correct.bmp (75.7 KiB)
   aa-bad.bmp (75.7 KiB)
   aa-correct.bmp (75.7 KiB)
   widecabbie.diff (1.6 KiB) 
 widecabbie-fixed.wps |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comment by Michael Chicoine (mc2739) - Friday, 25 June 2010, 21:36 GMT+2
I missed two vp definitons that did not display and needed the %Vf() added. They are line 69 and line 100, and were also text fields.
Comment by Michael Chicoine (mc2739) - Friday, 25 June 2010, 21:52 GMT+2
New issue:

e200v2 - r27106 with newparser.2.diff and newplaylistviewer.0.diff and using Wide Cabbie theme

When playback starts the last song in a playlist, either through normal playback or by skipping or fast forwarding to the last song, I get panics or lockups:

Undefined instruction
at 3082673C (or 30826760, 30826F30)
Comment by Jonathan Gordon (jdgordon) - Saturday, 26 June 2010, 10:55 GMT+2
OK thanks,
comment and viewport handling is a bit wierd so I'm going to try this from another angle....

Loading...