Rockbox mail archiveSubject: 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>:
> 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.
> 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
yep, no objections to fixing this one
Received on 2009-06-28