GNOME.org
 

Re: [Tracker] REVIEW: 'data-provider-monitor-interface' branch

On 1/15/15, Martyn Russell <martyn lanedo com> wrote:

Hi all,

I've recently completed a feature branch to allow 3rd party data
provider implementations to notify when data has been updated. This
mimics the existing TrackerMonitor API that already exists but
incorporates it into the existing TrackerDataProvider interface.

The branch is available here:

https://git.gnome.org/browse/tracker/log/?h=data-provider-monitor-interface

Please review. I plan to do an unstable release with this soon.

The TrackerMonitor code is untouched and the TrackerFileDataProvider
(which is the file system and default implementation for
tracker-miner-fs) simply connects all signals up to this. I've done it
this way to preserve history and code clarity in tracker-monitor.c and
to keep things as simple as possible right now.

The interface specifically is defined here:
https://git.gnome.org/browse/tracker/tree/src/libtracker-miner/tracker-data-provider.h?h=data-provider-monitor-interface

Internally, we now have removed some of the places where a
TrackerIndexingTree is required because a TrackerDataProvider already
exists (in classes like TrackerFileNotifier, etc). This is because the
Most classes in connection with the data provider need to know about the
indexing tree.


I worry (it's a bit late for this I realise) that we seem to be building
a VFS abstraction on top of a VFS abstraction -- i.e. GIO should be able
to do all this stuff anyway. It might be nice to have a summary somewhere in
the DataProvider API of where and why the API GIO provides is lacking.

Also, have you looked at the gnome-online-miners at all? I'm wondering if any
code could be removed from those projects now that libtracker-miner provides
more flexibility.

On the whole I like the direction this is going, although it's a shame to be
introducing yet more classes and abstractions into libtracker-miner, I think
it'll make things simpler in the long run. Would be fun to play with this new
code and try to write a simpler miner in Python using TrackerDataProvider. One
snowy day perhaps :)

Some minor (miner!) comments and questions below. I think the branch
is fine to merge though.


commit a750a5c2e27c9bd73729657fde5ed4d7583cc1b6
Author: Martyn Russell <martyn lanedo com>
Date:   Mon Dec 22 16:00:36 2014 +0000

    libtracker-miner: Moved monitor_{add|remove} to TrackerDataProvider

    Now TrackerMonitor is a part-implementation for that interface managed in
    TrackerFileDataProvider



diff --git a/src/libtracker-miner/tracker-data-provider.h b/src/libtracker-miner/tracker-data-provider.h
index deab933..bcd4204 100644
--- a/src/libtracker-miner/tracker-data-provider.h
+++ b/src/libtracker-miner/tracker-data-provider.h
@@ -63,6 +63,10 @@ typedef struct _TrackerDataProviderIface TrackerDataProviderIface;
  * Completed using @end_finish.
  * @end_finish: Called when the data_provider is completing the
  * asynchronous operation provided by @end_async.
+ * @add_monitor: Called when the data_provider is asked to monitor a
+ * container for changes.
+ * @remove_monitor: Called when the data_provider is asked to stop
+ * monitoring a container for changes.


add_monitor and remove_monitor above don't match the names below.


  * @item_created: Signalled when an item is created in a monitored
  * container. This can be another container or object itself. A
  * container could be a directory and an object could be a file in
@@ -124,6 +128,15 @@ struct _TrackerDataProviderIface {
                                                    GAsyncResult           *result,
                                                    GError                **error);

+     /* Monitoring API - to tell data provider you're interested */
+     gboolean              (* monitor_add)        (TrackerDataProvider    *data_provider,
+                                                   GFile                  *container,
+                                                   GError                **error);
+     gboolean              (* monitor_remove)     (TrackerDataProvider    *data_provider,
+                                                   GFile                  *container,
+                                                   gboolean                recursively,
+                                                   GError                **error);
+
      /* Monitoring Signals - for container/object change notification */
      void                  (* item_created)       (TrackerDataProvider    *data_provider,
                                                    GFile                  *file,



@@ -1354,7 +1360,7 @@ indexing_tree_directory_removed (TrackerIndexingTree *indexing_tree,

      /* Remove monitors if any */
      /* FIXME: How do we handle this with 3rd party data_providers? */
-     tracker_monitor_remove_recursively (priv->monitor, directory);
+     tracker_data_provider_monitor_remove (priv->data_provider, directory, TRUE, NULL);


Not sure what you mean with this FIXME. Surely it just works the same?



commit a0cf39db37c1073bfad5882dc72ed7bfc409f7fe
Author: Martyn Russell <martyn lanedo com>
Date:   Mon Dec 22 17:33:04 2014 +0000

    libtracker-miner: Removed TrackerIndexingTree property everywhere

    It's now a property of a TrackerDataProvider

diff --git a/src/libtracker-miner/tracker-data-provider.c b/src/libtracker-miner/tracker-data-provider.c
index 58cf630..8f7fa90 100644
--- a/src/libtracker-miner/tracker-data-provider.c
+++ b/src/libtracker-miner/tracker-data-provider.c
@@ -493,3 +493,75 @@ tracker_data_provider_monitor_remove (TrackerDataProvider  *data_provider,

      return (* iface->monitor_remove) (data_provider, container, recursively, error);
 }
+
+/**
+ * tracker_data_provider_set_indexing_tree:
+ * @data_provider: a #TrackerDataProvider
+ * @indexing_tree: a #TrackerIndexingTree
+ * @error: location to store the error occurring, or %NULL to ignore
+ *
+ * Tells @data_provider to use @indexing_tree. This is used by
+ * implementations to know what the rules are for ignoring content
+ * (among other things).
+ *
+ * Returns: %TRUE on success, otherwise %FALSE and @error is set.
+ *
+ * Since: 1.4
+ **/
+gboolean
+tracker_data_provider_set_indexing_tree (TrackerDataProvider  *data_provider,
+                                         TrackerIndexingTree  *indexing_tree,
+                                         GError              **error)
+{
+     TrackerDataProviderIface *iface;
+
+     g_return_val_if_fail (TRACKER_IS_DATA_PROVIDER (data_provider), FALSE);
+     g_return_val_if_fail (TRACKER_IS_INDEXING_TREE (indexing_tree), FALSE);
+
+     iface = TRACKER_DATA_PROVIDER_GET_IFACE (data_provider);
+
+     if (iface->set_indexing_tree == NULL) {
+             g_set_error_literal (error,
+                                  G_IO_ERROR,
+                                  G_IO_ERROR_NOT_SUPPORTED,
+                                  _("Operation not supported"));
+             return FALSE;
+     }
+
+     return (* iface->set_indexing_tree) (data_provider, indexing_tree, error);
+}
+
+/**
+ * tracker_data_provider_get_indexing_tree:
+ * @data_provider: a #TrackerDataProvider
+ * @error: location to store the error occurring, or %NULL to ignore
+ *
+ * Return the #TrackerIndexingTree that @data_provider is using. For
+ * details about why this is needed by @data_provider, see
+ * tracker_data_provider_set_indexing_tree().
+ *
+ * Returns: (transfer none): A #TrackerIndexingTree, otherwise %NULL and
+ * @error is set.
+ *
+ * Since: 1.4
+ **/
+TrackerIndexingTree *
+tracker_data_provider_get_indexing_tree (TrackerDataProvider  *data_provider,
+                                         GError              **error)
+{
+     TrackerDataProviderIface *iface;
+
+     g_return_val_if_fail (TRACKER_IS_DATA_PROVIDER (data_provider), NULL);
+
+     iface = TRACKER_DATA_PROVIDER_GET_IFACE (data_provider);
+
+     if (iface->get_indexing_tree == NULL) {
+             g_set_error_literal (error,
+                                  G_IO_ERROR,
+                                  G_IO_ERROR_NOT_SUPPORTED,
+                                  _("Operation not supported"));
+             return NULL;
+     }
+
+     return (* iface->get_indexing_tree) (data_provider, error);
+}

diff --git a/src/libtracker-miner/tracker-data-provider.h b/src/libtracker-miner/tracker-data-provider.h
index bcd4204..d2fbd1f 100644
--- a/src/libtracker-miner/tracker-data-provider.h
+++ b/src/libtracker-miner/tracker-data-provider.h
@@ -29,6 +29,7 @@
 #include <gio/gio.h>

 #include "tracker-enumerator.h"
+#include "tracker-indexing-tree.h"
 #include "tracker-miner-enums.h"

 G_BEGIN_DECLS
@@ -156,12 +157,19 @@ struct _TrackerDataProviderIface {
                                                    GFile                  *other_file,
                                                    gboolean                is_container);

+     /* Indexing tree to know what we can monitor */
+     gboolean              (* set_indexing_tree)  (TrackerDataProvider    *data_provider,
+                                                   TrackerIndexingTree    *indexing_tree,
+                                                   GError                **error);
+     TrackerIndexingTree * (* get_indexing_tree)  (TrackerDataProvider    *data_provider,
+                                                   GError                **error);
+
      /*< private >*/
-     /* Padding for future expansion */
-     void (*_tracker_reserved1) (void);
+     /* Already +1 past padding :/ */
 };


Could this be achieved by adding a property rather than adding more vfuncs ?
I can't remember if GLib interfaces can have properties of their own or
not. And in fact, is there any reason to change the IndexingTree that's
in use after the DataProvider has been constructed? Could it be set once
at construct-time instead?



commit 0715c1a49e47c34a4a2287ea26096adf7e3be430
Author: Martyn Russell <martyn lanedo com>
Date:   Wed Jan 14 14:11:40 2015 +0000

    miners: Stopped using deprecated tracker_miner_fs_get_indexing_tree()




diff --git a/src/miners/apps/tracker-miner-applications.c b/src/miners/apps/tracker-miner-applications.c
index a01d3dd..8753ca0 100644
--- a/src/miners/apps/tracker-miner-applications.c
+++ b/src/miners/apps/tracker-miner-applications.c
@@ -91,11 +91,21 @@ static void
 miner_applications_basedir_add (TrackerMinerFS *fs,
                                 const gchar    *basedir)
 {
+     TrackerDataProvider *data_provider;
      TrackerIndexingTree *indexing_tree;
+     GError *error = NULL;
      GFile *file;
      gchar *path;

-     indexing_tree = tracker_miner_fs_get_indexing_tree (fs);
+     data_provider = tracker_miner_fs_get_data_provider (fs);
+     indexing_tree = tracker_data_provider_get_indexing_tree (data_provider, &error);
+
+     if (!indexing_tree) {
+             g_critical ("  Could not add directories to be indexed, %s",
+                         error ? error->message : "TrackerIndexingTree was NULL");
+             g_clear_error (&error);
+             return;
+     }



I'm a little confused why we support the case of a DataProvider having no
IndexingTree. what functionality still works and what functionality doesn't
work?



commit 10866ed1160388902565f0617cd16cdd685ec928
Author: Martyn Russell <martyn lanedo com>
Date:   Wed Jan 14 11:37:28 2015 +0000

    Revert "libtracker-miner: Removed unused APIs from tracker-monitor"

    This reverts commit 65951061a4e841f50cb2724657472697f1530b51.



It makes it easier to review if you can rebase and remove this commit and the
one it reverts!



commit e76506689768c01ce5dc71794ab699bbad614e3a
Author: Martyn Russell <martyn lanedo com>
Date:   Wed Jan 14 14:12:33 2015 +0000

    libtracker-miner: Add remaining monitor APIs to DataProvider interface

    These are needed for many of the unit tests.

diff --git a/src/libtracker-miner/tracker-data-provider.c b/src/libtracker-miner/tracker-data-provider.c
index 8f7fa90..ce36823 100644
--- a/src/libtracker-miner/tracker-data-provider.c
+++ b/src/libtracker-miner/tracker-data-provider.c
@@ -495,6 +495,149 @@ tracker_data_provider_monitor_remove (TrackerDataProvider  *data_provider,
 }

 /**
+ * tracker_data_provider_monitor_move:
+ * @data_provider: a #TrackerDataProvider
+ * @container_from: a #GFile
+ * @container_to: a #GFile
+ * @error: location to store the error occurring, or %NULL to ignore
+ *
+ * Tells @data_provider to move @container_from to @container_to.
+ * Typically used for file systems where a folder or directory is
+ * moved.
+ *
+ * Returns: %TRUE on success, otherwise %FALSE and @error is set.
+ *
+ * Since: 1.4
+ **/
+gboolean
+tracker_data_provider_monitor_move (TrackerDataProvider  *data_provider,
+                                    GFile                *container_from,
+                                    GFile                *container_to,
+                                    GError              **error)


Why are the parameters called 'container_from' and 'container_to' ?
surely just 'from' and 'to' would be clearer?

Also, what is the 'error' parameter for? I don't see how the
TrackerFileNotifier could handle an error raised by this function, other
than by logging it and carrying on, so why not just log it at the time
it happens?


+{
+     TrackerDataProviderIface *iface;
+
+     g_return_val_if_fail (TRACKER_IS_DATA_PROVIDER (data_provider), FALSE);
+     g_return_val_if_fail (G_IS_FILE (container_from), FALSE);
+     g_return_val_if_fail (G_IS_FILE (container_to), FALSE);
+
+     iface = TRACKER_DATA_PROVIDER_GET_IFACE (data_provider);
+
+     if (iface->monitor_move == NULL) {
+             g_set_error_literal (error,
+                                  G_IO_ERROR,
+                                  G_IO_ERROR_NOT_SUPPORTED,
+                                  _("Operation not supported"));
+             return FALSE;
+     }
+
+     return (* iface->monitor_move) (data_provider, container_from, container_to, error);
+}




commit fd3801cb2b0e6ef80db4b150642b64749cbeab91
Author: Martyn Russell <martyn lanedo com>
Date:   Wed Jan 14 16:13:02 2015 +0000

    libtracker-miner: Connect FileDataProvider monitor signals up

diff --git a/src/libtracker-miner/Makefile.am b/src/libtracker-miner/Makefile.am
index 5df74a3..60ca585 100644
--- a/src/libtracker-miner/Makefile.am
+++ b/src/libtracker-miner/Makefile.am
@@ -41,6 +41,8 @@ private_sources =                                  \
      tracker-file-notifier.c                        \
      tracker-file-system.h                          \
      tracker-file-system.c                          \
+     tracker-monitor.c                              \
+     tracker-monitor.h                              \
      tracker-priority-queue.h                       \
      tracker-priority-queue.c                       \
      tracker-task-pool.h                            \
@@ -59,8 +61,6 @@ private_sources +=                                 \
 endif

 miner_sources =                                     \
-     $(libtracker_miner_monitor_sources)            \
-     $(libtracker_miner_monitor_headers)            \
      tracker-data-provider.c                        \
      tracker-data-provider.h                        \
      tracker-decorator.c                            \


Do you plan to rename TrackerMonitor to TrackerFileMonitor? If it's now private
then that should be doable, right?


 static gboolean
+file_data_provider_monitor_move (TrackerDataProvider  *data_provider,
+                                 GFile                *container_from,
+                                 GFile                *container_to,
+                                 GError              **error)
+{
+     TrackerFileDataProvider *fdp;
+
+     g_return_val_if_fail (TRACKER_IS_FILE_DATA_PROVIDER (data_provider), FALSE);
+     g_return_val_if_fail (G_IS_FILE (container_from), FALSE);
+     g_return_val_if_fail (G_IS_FILE (container_to), FALSE);
+
+     fdp = TRACKER_FILE_DATA_PROVIDER (data_provider);
+
+     return tracker_monitor_move (fdp->monitor, container_from, container_to);
+}


Again, why are the parameters called 'container_from' and 'container_to' ?
Container suggests 'directory' to me, but these won't be directories.


+static gboolean
+file_data_provider_is_monitored (TrackerDataProvider  *data_provider,
+                                 GFile                *container,
+                                 GError              **error)
+{
+     TrackerFileDataProvider *fdp;
+
+     g_return_val_if_fail (TRACKER_IS_FILE_DATA_PROVIDER (data_provider), FALSE);
+     g_return_val_if_fail (G_IS_FILE (container), FALSE);
+
+     fdp = TRACKER_FILE_DATA_PROVIDER (data_provider);
+
+     return tracker_monitor_is_watched (fdp->monitor, container);
+}
+
+static gboolean
+file_data_provider_is_monitored_by_path (TrackerDataProvider  *data_provider,
+                                         const gchar          *container,
+                                         GError              **error)
+{
+     TrackerFileDataProvider *fdp;
+
+     g_return_val_if_fail (TRACKER_IS_FILE_DATA_PROVIDER (data_provider), FALSE);
+     g_return_val_if_fail (container != NULL, FALSE);
+
+     fdp = TRACKER_FILE_DATA_PROVIDER (data_provider);
+
+     return tracker_monitor_is_watched_by_string (fdp->monitor, container);
+}
+
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]