GNOME Bugzilla – Bug 454244
GtkBuilder cleanup
Last modified: 2008-02-21 21:29:19 UTC
From bug #450635: "The skipto dialog doesn't update the label anymore though, so that'll need to be fixed." " - The "stock-tool-brightness-contrast-22.png" image on the Preferences dialog no longer shows up. I've tried everything I can to get it to appear, but no luck. We may want to move to using a proper stock icon instead of the one in /data. - I'm sure there are signals and GObjects I've missed, but since this is a first draft, I think that's OK, and they can be caught in later patches (once this one's been applied). - As discussed with Bastien, I haven't been able to move dialogs such as TotemOpenLocation completely into the XML, so we're still reparenting the vbox from the XML to the dialog GObject. I can't see much of a way round this at the moment, but there's still room for experimentation (and if this can be done, lots of stuff can be cleaned up). - There's still cleanup to do once bug #447969 lands, using the API it'll introduce."
Created attachment 91953 [details] [review] Patch to update reference to skip_to.glade After I built from svn today, I noticed that totem throws an error dialog while loading saying that it could not load Skipto plugin because it failed to load its dialog interface. For Philip, the label not working is probably an artifact of older build install of skip_to.glade. This patch * Updates references to skip_to.glade * Renames parameter glade_filename Also, I don't have any problem with brightness contrast icon in the preferences dialog after the fresh build.
Sorry both problems are not solved, I was sleepy and ran the wrong totem. The patch is correct anyway.
Created attachment 91957 [details] [review] Patch to connect signals in skipto plugin Ok, this one actually fixes the skipto label not updating problem by connecting the gtk builder ui signals.
An older skip_to.glade shouldn't affect the label, as your fix is only for the check that the UI file exists (which I missed :-( ) --- it's still loading the right UI file. What might be a better long-term fix is to create a new totem_plugin_load_interface function in totem-plugin.c, which is similar to totem_interface_load, but calls totem_plugin_find_file instead of totem_interface_get_full_path. In fact, both the new function and totem_interface_load could call a new totem_interface_load_with_path, which takes in the path of the interface to load. The skipto plugin (and any other plugins which use UI files) could then be changed to use totem_plugin_load_interface, which would take care of adding the signals and translation domain, so that they don't have to be boilerplated every time. Thanks.
Created attachment 91970 [details] [review] Patch to add totem_interface_load_with_full_path (In reply to comment #4) [...] I tried to do what you said with this patch: * created totem_interface_load_with_full_path (with_full_path instead of with_path because it seemed more consistent with get_full_path etc.) * totem_interface_load and totem_plugin_load_interface use totem_interface_load_with_full_path * Removed two glade related function declarations not used now * Removed libglade-dev build dependency in RPM spec file * In the end I could not think of a better way than to make TotemSkipto depend on TotemPlugin which doesn't look all right. Thanks you, I appreciate the time you look to review the patch (esp. because it takes you much less time to implement the things you said :)
Created attachment 91983 [details] [review] Fix display of stock-tool-brightness-contrast-22.png Bug #447966 related to gdkpixbuf loading in gtkbuilder has been fixed. So a simple change of property name from "file" to "pixbuf" fixes the image loading problem.
(In reply to comment #5) > Created an attachment (id=91970) [edit] > Patch to add totem_interface_load_with_full_path > > *snip* --- src/plugins/skipto/totem-skipto.c (revision 4445) +++ src/plugins/skipto/totem-skipto.c (working copy) +#include "totem-interface.h" You shouldn't have to add that. -totem_skipto_new (const char *builder_filename, Totem *totem) +totem_skipto_new (TotemPlugin *plugin, Totem *totem) You should be able to remove the *totem parameter, and just use plugin->totem, which should make things more simple. --- src/totem-interface.c (revision 4445) +++ src/totem-interface.c (working copy) + gchar * name = g_path_get_basename (filename); I don't think this is a good idea; now that UI files can be loaded from any/a multitude of paths, it would be helpful for people to be able to see exactly which path can't be found/loaded. Your patch in comment #6 looks good, but I think it might be a better idea to try and make use of a stock icon, and take the opportunity to stop using our own one in /data.
(In reply to comment #7) > +totem_skipto_new (TotemPlugin *plugin, Totem *totem) > > You should be able to remove the *totem parameter, and just use plugin->totem, > which should make things more simple. > TotemPlugin does not have member totem, TotemSkiptoPlugin does. Should we add it to TotemPlugin class? (because many plugins might need it). Or should we simply make totem member public in TotemSkiptoPlugin?
Whoops. I think it would be easier to just pass a TotemSkiptoPlugin to totem_skipto_new, then.
Created attachment 92066 [details] [review] Updated patch to add totem_interface_load_with_full_path Updates: * removed spurious totem-interface.h include. Sorry, it was an artifact of earlier experimentation :( * Made plugin->totem private and kept rest of the members private for TotemSkiptoPlugin. Passed TotemSkiptoPlugin as argument to totem_skipto_new. * Interface load fail error message will show full path of the interface file.
2007-07-23 Bastien Nocera <hadess@hadess.net> * src/plugins/skipto/*: upd for the changed below * src/plugins/totem-plugin.c: (totem_plugin_load_interface): * src/plugins/totem-plugin.h: Add totem_plugin_load_interface to allow plugins to load their interfaces from their own directory * src/totem-interface.c: (totem_interface_load), (totem_interface_load_with_full_path): * src/totem-interface.h: Implement helper functions to load GtkBuilder UI files * src/totem-statusbar.h: * src/totem-time-label.h: Remove unnecessary glade helper functions This change and above from a patch by Sunil Mohan Adapa <sunilmohan@gnu.org.in> * src/totem-object.c: (totem_object_class_init), (totem_object_get_property), (totem_get_current_time): Fix totem_get_current_time to return the current time, and not the length of the stream, add a "current-time" property as well
Apparently, it's not all fixed.
Icon problem is fixed, that only leaves the TotemOpenLocation problem. 2007-07-23 Bastien Nocera <hadess@hadess.net> * data/totem.ui: Use the stock_contrast icon, as shipped by the gnome icon theme (Helps: #454244)
Status?
Various commits between the time this bug was opened and now have cleaned up the TotemOpenLocation code a little, so I'd consider this bug fixed. We can always open a new bug once bug #447969 is fixed.