Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: Re: Center on loaded theme FS#10391 (+ set_file bugfix FS#10392)

Re: Center on loaded theme FS#10391 (+ set_file bugfix FS#10392)

From: Jonathan Gordon <jdgordy_at_gmail.com>
Date: Sun, 28 Jun 2009 10:56:35 -0700

2009/6/28 Jonas Häggqvist <rasher_at_rasher.dk>:
> ========
> FS#10391
> ========
>
> After committing FS#10093, which will center the currently loaded file in
> the WPS, Font, FM Preset and Language selection the theme selection stood
> out like a sore thumb.
>
> To this end, I've created FS#10391, which fixes this by remembering each
> time a theme is loaded (the actual check is "a config file from
> THEME_DIR"). The filename is saved in the global_settings struct, which is
> checked when browsing themes. The setting will not be used for anything
> else (so no auto-loading of the theme or anything like that).
>
> There is the issue that this will not technically be 100% correct if the
> user has changed a theme setting (say, colours or backdrop). I don't think
> this will cause any confusion, since a user with that attention to detail
> shouldn't forget that he's changed anything. It could also be seen as a
> quick way to reset settings to the loaded theme.
>

I dont know if this should go in... unlike font and wps filenames, the
.cfg can be loaded from anywhere, so just assuming that it will be
loaded from the themes dir is a bad assumption. Also the fact that
changeing any "theme" setting will make the selection technically
wrong... and finally, there is nothing stopping you from loading two
theme .cfg's and have them work together.
There is a reason we dont save the theme filename and I dont tihnk
this should be changed.

> ========
> FS#10392
> ========
>
> In the course of writing FS#10391, I discovered that set_file(const char*
> filename, char* setting, int maxlen) from settings.c will in fact modify
> the filename argument. Line 1157 sets the location of the last '/' in the
> string to \0, which of course wreaks havoc if you try to use the filename
> after calling set_file. As far as I can tell, this is completely
> unnecessary, since the filename argument is only used in the following
> statement: strncasecmp(ROCKBOX_DIR, filename ,strlen(ROCKBOX_DIR))  which
> will give the same result regardless where the filename string is
> terminated, since it will only compare up to strlen(ROCKBOX_DIR)
> characters anyway.
>
> FS#10392 simply removes line 1157.
>
> Any objections to committing both of these?
>
> --
> Jonas Häggqvist
> rasher(at)rasher(dot)dk
>

yep, no objections to fixing this one
Received on 2009-06-28


Page was last modified "Jan 10 2012" The Rockbox Crew
aaa