GNOME Bugzilla – Bug 166267
Large cleanup patch
Last modified: 2005-05-12 10:50:21 UTC
This patch does the following: - remove a load of leaks in the theme manager code - move said code to use GtkIconTheme - clean up some sparse warnings here and there - build fixes - maybe more? :-)
Created attachment 36978 [details] [review] patch
New take: - fix more leaks - fix to make sure newly installed themes show up in the relevant lists without having to close/open the dialog again
- remove spurious change in accessibility/keyboard
Created attachment 36981 [details] [review] new patch
This all runs find when installing themes and changing between them and customizing stuff. The only problem I've seen with this patch is that I get an invalid read in valgrind when installing an icon theme, but that could be the theme itself or something outside of the theme code's fault. Also, if you delete directories in ~/.icons or ~/.themes that hold the current theme and open gnome-theme-manager it spews a lot of warnings and is a bit unhappy, but that's kind of extreme behaviour on the users part anyway... I would like to get this into CVS ASAP, but I don't want to just commit it without review. Could someone take a look at this during the weekend, since I'm going to be without connectivity at least until sunday evening CEST. Also testing more themes would be nice.
I'm not really familiar with this code, some comments ... - capplets/common/gnome-theme-info.c: why not changing the add_data_to_hash_by_name () call line 708 in the same way ? Are you sure that the data is not changed somewhere during the running time ? - capplets/common/theme-thumbnail.c: why using GTK_ICON_LOOKUP_FORCE_SVG ? - capplets/theme-switcher/gnome-theme-installer.c: what do you think about adding some g_free (uri) with the g_free (src_uri) ? @@ -380,7 +441,7 @@ install_dialog_response (GtkWidget *widg why changing the declaration ? The other changes seem to be good.
- capplets/common/gnome-theme-info.c: > > why not changing the add_data_to_hash_by_name () call line 708 in the same way ? > Are you sure that the data is not changed somewhere during the running time ? > > I'm planning on changing all calls to add_data_to_hash_by_name() since it copies the data internally and we shouldn't have to do it twice. Needs some more testing with valgrind after doing that. > - capplets/common/theme-thumbnail.c: > > why using GTK_ICON_LOOKUP_FORCE_SVG ? > > Not sure really, maybe just confusion wrt the explicit call to allow SVGs in the old code. > - capplets/theme-switcher/gnome-theme-installer.c: > > what do you think about adding some g_free (uri) with the g_free (src_uri) ? > > Yeah, I think I've missed a few in there... @@ -380,7 +441,7 @@ install_dialog_response (GtkWidget *widg > > why changing the declaration ? > > Hmm, maybe just some temporal change that made it in there. I'll remove it and see if something evil pops back up. > The other changes seem to be good. > > Thanks. I'll upload an updated patch sometime tomorrow
Created attachment 37188 [details] [review] new and updated patch
Comment on attachment 37188 [details] [review] new and updated patch patch commited, thanks
Closing since the patch was commited.