GNOME Bugzilla – Bug 622779
Chapters plugin for the master branch
Last modified: 2010-08-09 15:38:38 UTC
Created attachment 164662 [details] [review] Chapters plugin for master Here is a patch to integrate chapters plugin into the master branch. Also it adds option in preferences to autoload external chapters.
Review of attachment 164662 [details] [review]: First pass at a review. Note that I would prefer the chapter data to be editable from within the treeview, rather than in a separate dialogue, if at all possible. ::: configure.in @@ +575,3 @@ ;; + chapters) + PKG_CHECK_MODULES(CHAPTERS, libxml-2.0 >= 2.6.0 gtk+-x11-2.0 glib-2.0 >= 2.15.0, Why do you need gtk+-x11? And it's the wrong version of it for master anyway. ::: data/totem.schemas.in @@ +327,3 @@ + <short>Whether to autoload external chapter files when a movie is loaded</short> + <long> + Whether to autoload external chapter files when a movie is loaded Missing period in the long description. ::: src/plugins/chapters/chapters.totem-plugin.in @@ +2,3 @@ +Module=chapters +IAge=1 +Builtin=false Should be true. ::: src/plugins/chapters/totem-chapters-utils.c @@ +34,3 @@ + + g_return_val_if_fail (filename != NULL, g_strdup ("")); + g_return_val_if_fail (filename[0] != '\0', g_strdup ("")); Return NULL instead and do that in the caller instead. @@ +54,3 @@ + + g_return_val_if_fail (filename != NULL, NULL); + g_return_val_if_fail (ext != NULL, g_strdup (filename)); Return NULL instead and do that in the caller. ::: src/plugins/chapters/totem-chapters-utils.h @@ +33,3 @@ + * @return empty string if NULL given, + * string given if extension could not be detected or + * filename without extension otherwise In-line documentation should be in the .c file instead. ::: src/plugins/chapters/totem-chapters.c @@ +55,3 @@ + +#define TOTEM_CHAPTERS_PIXBUF_WIDTH 77 +#define TOTEM_CHAPTERS_PIXBUF_HEIGHT 48 Why 77x48? Can't the width be deducted from the height? It should probably use the same height as one of the icons. See gtk_icon_size_lookup(). @@ +77,3 @@ + GtkActionGroup *action_group; + GtkUIManager *ui_manager; + GList *chapters; Why do you keep a separate GList when the chapter data is already in the treestore? @@ +108,3 @@ + +GType totem_chapters_plugin_get_type (void) G_GNUC_CONST; + No need for so many linefeeds. @@ +243,3 @@ + TOTEM_CHAPTERS_PIXBUF_WIDTH, + TOTEM_CHAPTERS_PIXBUF_HEIGHT); + if (pixbuf != NULL) pixbuf should never be NULL, glib will assert on OOM situations. @@ +244,3 @@ + TOTEM_CHAPTERS_PIXBUF_HEIGHT); + if (pixbuf != NULL) + gdk_pixbuf_fill (pixbuf, 0x000000ff); No alpha in the gdk_pixbuf_new() call, but you still set the alpha here. @@ +270,3 @@ + + tree = ((TotemChaptersPlugin *) user_data)->priv->tree; + store = (GtkTreeStore *) gtk_tree_view_get_model (GTK_TREE_VIEW (((TotemChaptersPlugin *) user_data)->priv->tree)); Don't like that. Declare a plugin variable, and set it below the g_return_val_if_fail()
Created attachment 164842 [details] [review] New patch to merge chapters plugin
> First pass at a review. Note that I would prefer the chapter data to be > editable from within the treeview, rather than in a separate dialogue, if at > all possible. I removed editing dialog in favour of editing within treeview widget. > ::: configure.in > @@ +575,3 @@ > ;; > + chapters) > + PKG_CHECK_MODULES(CHAPTERS, libxml-2.0 >= 2.6.0 gtk+-x11-2.0 > glib-2.0 >= 2.15.0, > > Why do you need gtk+-x11? And it's the wrong version of it for master anyway. Well, I thought there should be all dependencies in configure.in. Anyway, I removed them excluding libxml2. > ::: data/totem.schemas.in > @@ +327,3 @@ > + <short>Whether to autoload external chapter files when a movie is > loaded</short> > + <long> > + Whether to autoload external chapter files when a movie is loaded > > Missing period in the long description. Fixed. > ::: src/plugins/chapters/chapters.totem-plugin.in > @@ +2,3 @@ > +Module=chapters > +IAge=1 > +Builtin=false > > Should be true. Fixed. > ::: src/plugins/chapters/totem-chapters-utils.c > @@ +34,3 @@ > + > + g_return_val_if_fail (filename != NULL, g_strdup ("")); > + g_return_val_if_fail (filename[0] != '\0', g_strdup ("")); > > Return NULL instead and do that in the caller instead. > > @@ +54,3 @@ > + > + g_return_val_if_fail (filename != NULL, NULL); > + g_return_val_if_fail (ext != NULL, g_strdup (filename)); > > Return NULL instead and do that in the caller. Fixed. > ::: src/plugins/chapters/totem-chapters-utils.h > @@ +33,3 @@ > + * @return empty string if NULL given, > + * string given if extension could not be detected or > + * filename without extension otherwise > > In-line documentation should be in the .c file instead. Fixed. > ::: src/plugins/chapters/totem-chapters.c > @@ +55,3 @@ > + > +#define TOTEM_CHAPTERS_PIXBUF_WIDTH 77 > +#define TOTEM_CHAPTERS_PIXBUF_HEIGHT 48 > > Why 77x48? Can't the width be deducted from the height? It should probably use > the same height as one of the icons. See gtk_icon_size_lookup(). Done. I doubled height taken from gtk_icon_size_lookup() as it was too small. > @@ +77,3 @@ > + GtkActionGroup *action_group; > + GtkUIManager *ui_manager; > + GList *chapters; > > Why do you keep a separate GList when the chapter data is already in the > treestore? Actually, not all data was stored in treestore: start time and title were missed. So I moved them into the treestore and removed separate GList. > @@ +108,3 @@ > + > +GType totem_chapters_plugin_get_type (void) G_GNUC_CONST; > + > > No need for so many linefeeds. Fixed. > @@ +243,3 @@ > + TOTEM_CHAPTERS_PIXBUF_WIDTH, > + TOTEM_CHAPTERS_PIXBUF_HEIGHT); > + if (pixbuf != NULL) > > pixbuf should never be NULL, glib will assert on OOM situations. Fixed. > @@ +244,3 @@ > + TOTEM_CHAPTERS_PIXBUF_HEIGHT); > + if (pixbuf != NULL) > + gdk_pixbuf_fill (pixbuf, 0x000000ff); > > No alpha in the gdk_pixbuf_new() call, but you still set the alpha here. Fixed. > @@ +270,3 @@ > + > + tree = ((TotemChaptersPlugin *) user_data)->priv->tree; > + store = (GtkTreeStore *) gtk_tree_view_get_model (GTK_TREE_VIEW > (((TotemChaptersPlugin *) user_data)->priv->tree)); > > Don't like that. Declare a plugin variable, and set it below the > g_return_val_if_fail() Tried to find and fix all such things.
(In reply to comment #3) > > ::: configure.in > > @@ +575,3 @@ > > ;; > > + chapters) > > + PKG_CHECK_MODULES(CHAPTERS, libxml-2.0 >= 2.6.0 gtk+-x11-2.0 > > glib-2.0 >= 2.15.0, > > > > Why do you need gtk+-x11? And it's the wrong version of it for master anyway. > > Well, I thought there should be all dependencies in configure.in. Anyway, I > removed them excluding libxml2. The point was that it should have been "gtk+-3.0", not the x11 variant.
Review of attachment 164842 [details] [review]: Looks fine other than that. I'll give it a test tomorrow. ::: data/totem.schemas.in @@ +193,3 @@ <short>Whether the main window should stay on top</short> <long> + Whether the main window should stay on top of the other ones. Only do this for your own additions, file a separate bug if you want to fix those. ::: src/plugins/chapters/totem-chapters-utils.c @@ +1,1 @@ +/* Created on: 07.05.2010 */ Remove those. @@ +56,3 @@ + p = g_strrstr (filename, "."); + if (G_UNLIKELY (p == NULL)) + return g_strdup (filename); No, return NULL here. And make the caller use the original variable when you return NULL. It saves a memory allocation. @@ +83,3 @@ + g_return_val_if_fail (ext != NULL, NULL); + + no_ext = totem_remove_file_extension (filename); if (no_ext == NULL) return NULL;
Created attachment 164870 [details] [review] New patch to merge chapters plugin (2)
> The point was that it should have been "gtk+-3.0", not the x11 variant. My mistake, I thought gtk+-3.0 is the same as gtk+-x11-3.0. Dependencies are restored.
> ::: data/totem.schemas.in > @@ +193,3 @@ > <short>Whether the main window should stay on top</short> > <long> > + Whether the main window should stay on top of the other ones. > > Only do this for your own additions, file a separate bug if you want to fix > those. Ok, I'll make a separate patch. > ::: src/plugins/chapters/totem-chapters-utils.c > @@ +1,1 @@ > +/* Created on: 07.05.2010 */ > > Remove those. Done. > @@ +56,3 @@ > + p = g_strrstr (filename, "."); > + if (G_UNLIKELY (p == NULL)) > + return g_strdup (filename); > > No, return NULL here. And make the caller use the original variable when you > return NULL. > > It saves a memory allocation. Done. > @@ +83,3 @@ > + g_return_val_if_fail (ext != NULL, NULL); > + > + no_ext = totem_remove_file_extension (filename); > > if (no_ext == NULL) > return NULL; Done. Also I made some various fixes and improvements in the code.
Created attachment 165053 [details] [review] Update for last changes in master, some fixes
Full of awesome, pushed to master. commit 48e0e269b4516cdae91a3fd666a6f0251df7706b Author: Bastien Nocera <hadess@hadess.net> Date: Mon Aug 9 16:37:10 2010 +0100 Port chapters plugin to libpeas 0.5.4 Also fix a number of compile-time warnings, and the inability to load UI files from the uninstalled directory. commit 66d1958a89affb91380777409bd863faf668e872 Author: Alexander Saprykin <xelfium@gmail.com> Date: Fri Jul 2 00:17:08 2010 +0400 Merge chapters plugin with a master branch New chapters plugin Add option to preferences to auto-load external chapters