Re: [PATCH 1/5] core: Add "content-changed" signal




Give me some time to review this feature.

Some comments about this particular patch follow below:

On Tue, 1 Feb 2011 09:07:42 +0100, "Juan A. Suarez Romero" <jasuarez igalia com> wrote:
(...)
+  /**
+   * GrlMediaSource::content-changed:
+   * @source: source that has changed
+ * @media: the media that has changed, or contains the children that have
+   * changed

I would use this wording instead:
   * @media: the media that changed or one of its ancestors.


+   * @change_type: what has happened

I would use this wording instead:
  * @change_type: the kind of change that occurred

+ * @location_unknown: @TRUE if the change can be in the @media or in any

I would use this wording instead:
* @location_unknown: @TRUE if the change happened in @media itself or in one of its direct children (when @media is a box). @FALSE otherwise.


+ * descendant of it. @FALSE means that either the @media has changed, or a
+   * direct child has changed
+   *
+ * Signals that the content in the source has changed. Usually @media is a
+   * #GrlBox, meaning that the content of that box has changed. if
+ * @location_unknown is @TRUE it means the source can establish where the
                                                       ^^^ cannot
+ * change happened: could be either in the box, in any child, or in any
+   * undirect descendant.
       ^^^^^^^^^^^^^^^^^^^^ I would use this wording instead:
      * other descendant of the box in the hierarchy.

+   *
+   * For the cases where the source only can signal that a change
                                       ^^^^^^^^
                                       can only
happened, but
+ * not where, it would use the root box (@NULL id) and location_unknown as
^^^^^^^^^^^^^^^ and set location_unkown to
+   * #TRUE.
+   */
+  registry_signals[SIG_CONTENT_CHANGED] =
+    g_signal_new("content-changed",
+                 G_TYPE_FROM_CLASS (gobject_class),
+                 G_SIGNAL_RUN_FIRST | G_SIGNAL_ACTION,
+ G_STRUCT_OFFSET(GrlMediaSourceClass, content_changed),
+                 NULL,
+                 NULL,
+                 grl_marshal_VOID__OBJECT_ENUM_BOOLEAN,
+                 G_TYPE_NONE,
+                 3,
+                 GRL_TYPE_MEDIA,
+                 GRL_TYPE_MEDIA_SOURCE_CHANGE_TYPE,
+                 G_TYPE_BOOLEAN);
 }

 static void
diff --git a/src/grl-media-source.h b/src/grl-media-source.h
index 6975004..83a5360 100644
--- a/src/grl-media-source.h
+++ b/src/grl-media-source.h
@@ -63,6 +63,19 @@
   (G_TYPE_INSTANCE_GET_CLASS ((obj),                            \
                               GRL_TYPE_MEDIA_SOURCE,            \
                               GrlMediaSourceClass))
+/**
+ * GrlMediaSourceChangeType:
+ * @GRL_CONTENT_CHANGED: content has changed.
+ * @GRL_CONTENT_ADDED: new content has been added.
+ * @GRL_CONTENT_REMOVED: content has been removed
+ *
+ * Specifies which kind of change has happened in the plugin
+ */
+typedef enum {
+  GRL_CONTENT_CHANGED,
+  GRL_CONTENT_ADDED,
+  GRL_CONTENT_REMOVED,
+} GrlMediaSourceChangeType;


I think we should be mre specific here, at least for the _CHANGE case. When exactly are we going to use _CHANGE instead of _ADDED/_REMOVED?

 /* GrlMediaSource object */

@@ -382,6 +395,12 @@ struct _GrlMediaSourceClass {

   /*< private >*/
   gpointer _grl_reserved[GRL_PADDING];
+
+  /* signals */
+  void (*content_changed) (GrlMediaSource *source,
+                           GrlMedia *media,
+                           GrlMediaSourceChangeType change_type,
+                           gboolean location_unknown);
 };


Hey! you are breaking the ABI here :)
you have to remove one slot from the _grl_reserved[] array to keep the size of the structure constant when you added the signal:

gpointer _grl_reserved[GRL_PADDING - 1];

Always keep that in mind when adding/removing APIs from now on.

 G_BEGIN_DECLS
@@ -502,6 +521,7 @@ GrlMedia
*grl_media_source_get_media_from_uri_sync (GrlMediaSource *source,
const GList *keys,

GrlMetadataResolutionFlags flags,
                                                     GError **error);
+
 G_END_DECLS

Aren't this grl-type-builtin* files auto-generated? In that case we should not have them stored.

 #endif /* _GRL_MEDIA_SOURCE_H_ */
diff --git a/src/grl-type-builtins.c.template
b/src/grl-type-builtins.c.template
new file mode 100644
index 0000000..734d23a
--- /dev/null
+++ b/src/grl-type-builtins.c.template
@@ -0,0 +1,35 @@
+/*** BEGIN file-header ***/
+#include "grl-type-builtins.h"
+#include "@filename@"
+/*** END file-header ***/
+
+/*** BEGIN file-production ***/
+/* enumerations from "@filename@" */
+/*** END file-production ***/
+
+/*** BEGIN value-header ***/
+GType
+@enum_name@_get_type (void)
+{
+    static GType etype = 0;
+    if (G_UNLIKELY(etype == 0)) {
+        static const G@Type@Value values[] = {
+/*** END value-header ***/
+
+/*** BEGIN value-production ***/
+            { @VALUENAME@, "@VALUENAME@", "@valuenick@" },
+/*** END value-production ***/
+
+/*** BEGIN value-tail ***/
+            { 0, NULL, NULL }
+        };
+        etype = g_@type@_register_static (g_intern_static_string
("@EnumName@"), values);
+    }
+    return etype;
+}
+
+/*** END value-tail ***/
+
+/*** BEGIN file-tail ***/
+
+/*** END file-tail ***/
diff --git a/src/grl-type-builtins.h.template
b/src/grl-type-builtins.h.template
new file mode 100644
index 0000000..6714f84
--- /dev/null
+++ b/src/grl-type-builtins.h.template
@@ -0,0 +1,24 @@
+/*** BEGIN file-header ***/
+#ifndef _GRL_TYPE_BUILTINS_H_
+#define _GRL_TYPE_BUILTINS_H_
+
+#include <glib-object.h>
+
+G_BEGIN_DECLS
+/*** END file-header ***/
+
+/*** BEGIN file-production ***/
+
+/* enumerations from "@filename@" */
+/*** END file-production ***/
+
+/*** BEGIN value-header ***/
+GType @enum_name@_get_type (void) G_GNUC_CONST;
+#define @ENUMPREFIX@_TYPE_@ENUMSHORT@ (@enum_name@_get_type())
+/*** END value-header ***/
+
+/*** BEGIN file-tail ***/
+G_END_DECLS
+
+#endif /* _GRL_TYPE_BUILTINS_H_ */
+/*** END file-tail ***/



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]