Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.0
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by kugel. - 2008-12-03
Last edited by jdgordon - 2008-12-31

FS#9603 - list and menu code cleanup

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.

Closed by  jdgordon
2008-12-31 05:59
Reason for closing:  Accepted
Additional comments about closing:  

the good part of this patch has been commited

Now with the patch

Dont’t forget about our touchscreen lists.

synced, works well

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).

only changed parent and list_display_title, dont know whether its right or not…

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!

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…

Roger that, wait a few minutes :)

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 :)

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 :) ).

Ooops, touchscreen again :)

Some more cleanup

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

saves about 100bytes binsize and 300bytes ram on h300.

tested on e200, works fine.

probably the last update

- enforce 80-char width coding guideline here and there

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing