Rockbox

Tasklist

FS#9603 - list and menu code cleanup

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Wednesday, 03 December 2008, 20:26 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 31 December 2008, 05:59 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.0
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch cleans up code in (each) list.c and menu.c.

-stop passing viewports unnecessarily within list code (if we pass a list, we already pass the parent of it with it, we don't need to pass the parent seperately too)

-cleanup do_menu by using the viewport generated by synclist_init, remove the menu_vp which was marked as "will hopefully phased out" and removing the init_default_menu_viewports

-due to the above, viewport_set_defaults will now be able to handle the bars. That also enables us to get a real fullscreen initialized viewport(finally?).

Generally save multiple initializing of lists in the menu code.

Issuing redraws of menus *should* be unneeded at all now, since, with this patch, the actual used viewport gets modified.

This patch also yields a tiny binsize & ram usage decrease on my e200.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 31 December 2008, 05:59 GMT
Reason for closing:  Accepted
Additional comments about closing:  the good part of this patch has been commited
Comment by Thomas Martitz (kugel.) - Wednesday, 03 December 2008, 20:27 GMT
Now with the patch
Comment by Thomas Martitz (kugel.) - Wednesday, 03 December 2008, 20:53 GMT
Dont't forget about our touchscreen lists.
Comment by Johannes Linke (Jaykay) - Monday, 22 December 2008, 15:44 GMT
synced, works well
Comment by Steve Bavin (pondlife) - Monday, 22 December 2008, 16:54 GMT
One minor point; I believe we conventionally we uppercase #define macros, so

#define parent (list->parent[display->screen_type])
should be
#define PARENT (list->parent[display->screen_type])
etc.

Also if youi left the parameter to gui_synclist_do_touchscreen() named gui_list, rather than renaming it, the patch would be smaller (and clearer IMHO).


Comment by Johannes Linke (Jaykay) - Monday, 22 December 2008, 17:02 GMT
only changed parent and list_display_title, dont know whether its right or not...
Comment by Thomas Martitz (kugel.) - Monday, 22 December 2008, 17:06 GMT
Yea, this parent define was just to save search & replace work during the creation (which is always also a bit error-prone), so I planned to remove that altogether when it's working and when/if it's going to get in. I'm aware that this isn't conventional.
But since JdGordon told me it's going to sit for a while I left it for now.

So, just tell me if it's getting committed soon I'll immediately clean then up!
Comment by Steve Bavin (pondlife) - Monday, 22 December 2008, 17:14 GMT
JayKay, I think your tidy up may have confused PARENT and parent, but that's just from a quick look at the diff - maybe kugel can clarify.

kugel - I'd like to see this committed too, but even if not, it's always good to keep a patch as clean as possible. It makes it more readable (as a diff) and might reduce the chance of resync being needed...
Comment by Thomas Martitz (kugel.) - Monday, 22 December 2008, 17:14 GMT
Roger that, wait a few minutes :)
Comment by Thomas Martitz (kugel.) - Monday, 22 December 2008, 18:32 GMT
here you go! Sorry for the small delay :(

I think unifying the parameter names is actually a good idea. It'd also fit the idea of this patch, but if you prefer clean patches, I'm perfectly fine with it :)
Comment by Thomas Martitz (kugel.) - Monday, 22 December 2008, 18:36 GMT
BTW: With this patch, my customlist patch at  FS#8799  runs just fine (which indicates that things are done properly with this patch here, at least for me :) ).
Comment by Thomas Martitz (kugel.) - Monday, 22 December 2008, 20:14 GMT
Ooops, touchscreen again :)
Comment by Thomas Martitz (kugel.) - Friday, 26 December 2008, 18:53 GMT
Some more cleanup

tested on h300 & e200 sim. Real target testing is welcome.

saves about 100bytes binsize and 300bytes ram on h300.
Comment by Johannes Linke (Jaykay) - Saturday, 27 December 2008, 15:36 GMT
tested on e200, works fine.
Comment by Thomas Martitz (kugel.) - Saturday, 27 December 2008, 18:04 GMT
probably the last update

- enforce 80-char width coding guideline here and there
Comment by Jonathan Gordon (jdgordon) - Tuesday, 30 December 2008, 13:05 GMT
ok, im going to commit this patch some time before the weekend (unless i forget or something) which handles the statusbar how it should be handled. the "hide_bars" parameter needs to be removed completely from do_menu() which will always show the bars if enabled.

if you sync up the list cleanup to this patch I'll commit both one after another

Loading...