Merge lp:~rye/ubuntuone-client/stop-killing-thumbnails into lp:ubuntuone-client

[Roman Yepishev]

Removed emblem updating by utime, keeping references to NautilusFileInfo and invalidating the file info

[Roman Yepishev (rye)]

Depends on emblems code fixup otherwise the the code that updates the files would not get called.

[Vincenzo Di Somma (vds)]

Could you please describe what this branch does?

[Roman Yepishev (rye)]

> Could you please describe what this branch does?

This branch fixes #491777 by creating internal hash for all file objects that were handled to us by Nautilus in ubuntuone_nautilus_update_file_info. Later on this info is used to request nautilus to invalidate extension date (in our case - emblems). After the call tonautilus_file_info_invalidate_extension_info the emblems get cleared and a call of ubuntuone_nautilus_update_file_info is performed.
The old method, updating the modification time for a file caused the thumbnails to be regenerated and is a pretty nasty practice, since other applications might use these timestamps for their purposes.

[dobey (dobey)]

This depends on https://edge.launchpad.net/~romdel/ubuntuone-client/bring-emblems-back/+merge/16314 as one of the changes there is needed for this branch to merge correctly. I'm going to set my review to Needs Fixing as a method of noting this, until that branch can be landed.

review: Needs Fixing

[Roman Yepishev (rye)]

Just a reminder.

Since the branch https://edge.launchpad.net/~rtgz/ubuntuone-client/bring-emblems-back/+merge/16314 has been merged, this can be merged as well.

[dobey (dobey)]

I think here you need to do the weak_ref before inserting into the hash table.

70 + g_hash_table_insert (uon->observed, g_strdup (path), file);
71 + g_object_weak_ref (G_OBJECT(file),
72 + (GWeakNotify)ubuntuone_nautilus_observed_file_unref,
73 + uon);

And I think here, you need to specify g_object_weak_unref as the destroy callback for the data.

+ uon->observed = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

I may be totally wrong on the second one (as weak refs are a little vague), but the first change definitely improves the readability of the code (makes more sense to ref before doing more stuff). Thanks for looking into this.

review: Needs Fixing

[lp:~rye/ubuntuone-client/stop-killing-thumbnails]

298. By Roman Yepishev on 2009-12-24

Reversed the order of weak ref callback registration and hash insert

[Roman Yepishev (rye)]

Reversed the order of the statements for the first item.

Unfortunately, it is not possible to call any destroy callback routine with custom arguments when uon->observer is destroyed. The callback accepts value pointer only and therefore our uon object cannot be easily accessed.

It is possible to add code that runs through the hash and removes all references, however it looks like nautilus keeps additional reference to NAUTILUS_TYPE_INFO_PROVIDER module (looks like ubuntuone-nautilus and nautilus-share are affected). Our shutdown and finalize routines are never called so I would rather not write code blindly :)

I am investigating this nautilus bug at the moment and will file a bug when I get some useful info.

[Roman Yepishev (rye)]

Okay, I found the guilty extension - nautilus-open-terminal.
https://bugzilla.gnome.org/show_bug.cgi?id=605390

Will comment here once I check the behavior of the finalize code regarding these weak references.

[lp:~rye/ubuntuone-client/stop-killing-thumbnails]

299. By Roman Yepishev on 2009-12-30

Added weak reference cleanup

All weak reference callbacks for existing file items are now removed prior to
hash destruction.

[dobey (dobey)]

Looks ok to me now. Thanks for working on this.

review: Approve

[Rick McBride (rmcbride)]

Very nice!

review: Approve

=== modified file 'nautilus/ubuntuone-nautilus.c'
--- nautilus/ubuntuone-nautilus.c	2009-12-18 22:24:01 +0000
+++ nautilus/ubuntuone-nautilus.c	2009-12-30 21:32:14 +0000
@@ -43,7 +43,6 @@
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <utime.h>
 
 /* The header is generated from ubuntuone-marshallers.list */
 #include "ubuntuone-marshallers.h"
@@ -80,6 +79,8 @@
   /* Lists of sync/unsync'd files */
   GHashTable * updated;
   GHashTable * needsupdating;
+  /* List of files that are observed for changes */
+  GHashTable * observed;
 
   /* Extra data we need to free on finalization */
   ShareCBData * share_cb_data;
@@ -94,6 +95,17 @@
 static GType ubuntuone_nautilus_get_type (void);
 static void ubuntuone_nautilus_register_type (GTypeModule * module);
 
+/* Utility functions */
+static void ubuntuone_nautilus_reset_emblem (UbuntuOneNautilus * uon,
+					     const char * path);
+
+static void ubuntuone_nautilus_observed_file_unref (gpointer user_data,
+                                                    GObject *where_the_object_was);
+
+static void ubuntuone_nautilus_observed_file_foreach_unref (gpointer key,
+							    gpointer value,
+							    gpointer user_data);
+
 /* DBus signal and async method call handlers */
 static void ubuntuone_nautilus_update_meta (DBusGProxy * proxy,
 					    DBusGProxyCall * call_id,
@@ -132,6 +144,46 @@
 
 static GObjectClass * parent_class = NULL;
 
+static void ubuntuone_nautilus_reset_emblem (UbuntuOneNautilus * uon,
+					     const char * path) {
+
+  NautilusFileInfo *file = g_hash_table_lookup (uon->observed, path);
+  if (file)
+    nautilus_file_info_invalidate_extension_info (file);
+}
+
+/* Called when NautilusFileInfo is unreferenced */
+static void ubuntuone_nautilus_observed_file_unref (gpointer user_data,
+                                                    GObject *where_the_object_was) {
+  UbuntuOneNautilus * uon;
+  NautilusFileInfo * file;
+  gchar * path = NULL;
+
+  uon  = UBUNTUONE_NAUTILUS(user_data);
+  file = NAUTILUS_FILE_INFO(where_the_object_was);
+  path = g_filename_from_uri (nautilus_file_info_get_uri (file), NULL, NULL);
+
+  if (g_hash_table_lookup (uon->observed, path ))
+    g_hash_table_remove (uon->observed, path);
+
+}
+
+/* Cleanup routine for weak reference callbacks */
+static void ubuntuone_nautilus_observed_file_foreach_unref (gpointer key,
+							    gpointer value,
+							    gpointer user_data) {
+  UbuntuOneNautilus * uon;
+  NautilusFileInfo * file;
+
+  uon = UBUNTUONE_NAUTILUS(user_data);
+  file = NAUTILUS_FILE_INFO(value);
+
+  g_object_weak_unref (G_OBJECT(file),
+		       (GWeakNotify)ubuntuone_nautilus_observed_file_unref,
+		       uon);
+
+}
+
 /* Are we in an Ubuntu One managed directory */
 static gboolean ubuntuone_is_storagefs (UbuntuOneNautilus * uon,
 					const char * path) {
@@ -171,6 +223,11 @@
   if (!path || !ubuntuone_is_storagefs (uon, path))
     goto updating_meta;
 
+  g_object_weak_ref (G_OBJECT(file),
+		     (GWeakNotify)ubuntuone_nautilus_observed_file_unref,
+		     uon);
+  g_hash_table_insert (uon->observed, g_strdup (path), file);
+
   if (!g_hash_table_lookup (uon->updated, path) &&
       !g_hash_table_lookup (uon->needsupdating, path)) {
     /* Add the synchronized emblem anyway, and update later */
@@ -566,6 +623,7 @@
   uon->shares = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
   uon->updated = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
   uon->needsupdating = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+  uon->observed = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
 
   /* Magic bag of holding +10:
    * We store some button widgets in a hash table here, so that we can
@@ -733,6 +791,14 @@
   g_hash_table_destroy (uon->needsupdating);
   uon->needsupdating = NULL;
 
+  /* We need to remove all weak reference callbacks */
+  g_hash_table_foreach (uon->observed, 
+			(GHFunc)ubuntuone_nautilus_observed_file_foreach_unref,
+			uon);
+
+  g_hash_table_destroy (uon->observed);
+  uon->observed = NULL;
+
   g_hash_table_destroy (uon->buttons);
   uon->buttons = NULL;
 }
@@ -833,7 +899,7 @@
     g_hash_table_replace (uon->updated, new_path, new_path);
   else
     g_hash_table_replace (uon->needsupdating, new_path, new_path);
-  utime (new_path, NULL);
+  ubuntuone_nautilus_reset_emblem (uon, new_path);
 }
 
 static void ubuntuone_nautilus_state_toggled (DBusGProxy * proxy,
@@ -939,7 +1005,7 @@
   if (!g_hash_table_lookup (uon->uploads, path)) {
     gchar *new_path = g_strdup (path);
     g_hash_table_insert (uon->uploads, new_path, new_path);
-    utime (path, NULL);
+    ubuntuone_nautilus_reset_emblem (uon, path);
   }
 }
 
@@ -955,7 +1021,7 @@
   new_path = g_strdup (path);
   g_hash_table_replace (uon->updated, new_path, new_path);
 
-  utime (path, NULL);
+  ubuntuone_nautilus_reset_emblem (uon, path);
 }
 
 static void ubuntuone_nautilus_download_started (DBusGProxy * proxy,
@@ -966,7 +1032,7 @@
   if (!g_hash_table_lookup (uon->downloads, path)) {
     gchar *new_path = g_strdup (path);
     g_hash_table_insert (uon->downloads, new_path, new_path);
-    utime (path, NULL);
+    ubuntuone_nautilus_reset_emblem (uon, path);
   }
 }
 
@@ -983,7 +1049,7 @@
   new_path = g_strdup (path);
   g_hash_table_replace (uon->updated, new_path, new_path);
 
-  utime (path, NULL);
+  ubuntuone_nautilus_reset_emblem (uon, path);
 }
 
 static void ubuntuone_nautilus_share_created (DBusGProxy * proxy,
@@ -996,6 +1062,7 @@
   if (!g_hash_table_lookup (uon->shares, path)) {
     gchar *new_share = g_strdup (path);
     g_hash_table_insert (uon->shares, new_share, new_share);
+    ubuntuone_nautilus_reset_emblem (uon, new_share);
   }
 }