GNOME.org
 

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

On 19/01/15 17:17, Sam Thursfield wrote:

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

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.
You're right, GIO should be able to do this and I did initially look into doing this. The one main hurdle I came up against, was that the GFile is heavily used in Tracker and the backend used is also not consistent. From what I could tell, it was a costlier exercise to implement separate modules for: 

a) backend GType implementation for GFile
b) enumerator
c) info
d) file monitor
As far as a) is concerned, most of what is needed is already available in GDummyFile - talking specifically for non local files that is.
I'm not actually sure the info/monitor/enumerator code in GIO lends itself to 3rd party backends so well either - I've not checked. 


Also, have you looked at the gnome-online-miners at all? I'm wondering if any


I'll be honest, no, I have not.


code could be removed from those projects now that libtracker-miner provides
more flexibility.


Perhaps Debarshi has some comment?


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 :)
To be fair, there are no extract classes, it's all in one GInterface, TrackerDataProvider. 


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


:)

Your points are fair.
I probably have other reasons I couldn't think of tonight for why I started this, but GFile seemed a mammoth undertaking and I did try something quick and dirty and it wasn't quick that's for sure. 


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);
+}
+



--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]