After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 622779 - Chapters plugin for the master branch
Chapters plugin for the master branch
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: general
2.90.x
Other Linux
: Normal enhancement
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2010-06-26 04:06 UTC by Alexander Saprykin
Modified: 2010-08-09 15:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Chapters plugin for master (100.61 KB, patch)
2010-06-26 04:06 UTC, Alexander Saprykin
needs-work Details | Review
New patch to merge chapters plugin (104.71 KB, patch)
2010-06-28 21:07 UTC, Alexander Saprykin
needs-work Details | Review
New patch to merge chapters plugin (2) (102.05 KB, patch)
2010-06-29 09:29 UTC, Alexander Saprykin
none Details | Review
Update for last changes in master, some fixes (102.53 KB, patch)
2010-07-01 20:22 UTC, Alexander Saprykin
none Details | Review

Description Alexander Saprykin 2010-06-26 04:06:17 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.
Comment 1 Bastien Nocera 2010-06-26 14:16:13 UTC
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()
Comment 2 Alexander Saprykin 2010-06-28 21:07:09 UTC
Created attachment 164842 [details] [review]
New patch to merge chapters plugin
Comment 3 Alexander Saprykin 2010-06-28 21:15:26 UTC
> 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.
Comment 4 Bastien Nocera 2010-06-28 23:17:53 UTC
(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.
Comment 5 Bastien Nocera 2010-06-28 23:24:08 UTC
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;
Comment 6 Alexander Saprykin 2010-06-29 09:29:22 UTC
Created attachment 164870 [details] [review]
New patch to merge chapters plugin (2)
Comment 7 Alexander Saprykin 2010-06-29 09:33:11 UTC
> 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.
Comment 8 Alexander Saprykin 2010-06-29 09:36:50 UTC
> ::: 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.
Comment 9 Alexander Saprykin 2010-07-01 20:22:28 UTC
Created attachment 165053 [details] [review]
Update for last changes in master, some fixes
Comment 10 Bastien Nocera 2010-08-09 15:38:38 UTC
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