Merge lp:~alecu/ubuntuone-client/restrain-out-of-space-dialog into lp:ubuntuone-client

[Alejandro J. Cura]

show out-of-space dialog with an interval stored in gconf, 0 to disable (fixes LP:#650671)

show out-of-space dialog with an interval stored in gconf, 0 to disable

[Roberto Alsina (ralsina)]

I agree with the idea, and the code looks ok to me. I think this is the best solution we can provide currently. I didn't fieldtest it though.

review: Approve

[Alejandro J. Cura (alecu)]

How to test this code:

First let's build the branch:
    cd $HOME/canonical/ubuntuone-client
    bzr branch lp:ubuntuone-client review_restrain-out-of-space-dialog
    cd review_restrain-out-of-space-dialog
    bzr merge lp:~alecu/ubuntuone-client/restrain-out-of-space-dialog
    gnome-autogen.sh && make

now let's make sure that syncdaemon is stopped, because the testing script will take its d-bus spot:
    u1sdtool -q

let's start up the testing script:
    cd gsd-plugin
    ./test-flood.py

The dialog will very quickly go thru three different messages. But since it keeps popping up, changing the messages but keeping popping up, that means that the bug was reproduced. Now let's replace the plugin with the one we just compiled. In a different terminal do:

    cd /usr/lib/gnome-settings-daemon-2.0
    sudo mv libubuntuone.so old.libubuntuone.so
    sudo ln -s $HOME/canonical/ubuntuone-client review_restrain-out-of-space-dialog/gsd-plugin/.libs/libubuntuone.so
    killall gnome-settings-daemon # your gnome settings will melt. Don't worry!
    gnome-settings-daemon --debug --no-daemon

Now go back to the first terminal and run the testing script again. It should now only show each message once every hour! The events should still be logged on the gnome-settings-daemon debug output.

Since we don't want you to waste a whole hour, let's open gconf-editor and change this (newly created) gconf key:
* /apps/gnome_settings_daemon/plugins/ubuntuone/out_of_space_warnings_interval

it should say 3600; let's change it to 10 and test the script again. (no need to restart g-s-d) Now the messages should take turns, but each message won't repeat more than every 10 seconds.

Finally, we can change that value to 0, and if you run the script no new messages should pop up at all.

You may now kill that g-s-d and start it again in the background:
 * killall gnome-settings-daemon ; sleep 3; gnome-settings-daemon

[Alejandro J. Cura (alecu)]

I made a mistake on a line above. Here's how it should look like:

sudo ln -s $HOME/canonical/ubuntuone-client/review_restrain-out-of-space-dialog/gsd-plugin/.libs/libubuntuone.so .

[dobey (dobey)]

This seems like an overly complex stopgap 'solution' to the issue. I think the proper fix would be simpler code, and it certainly wouldn't require more incorrect usage of gconf.

I would only show the dialog once per session, unless the quota limit has changed since the message was shown. This way, if there are pending uploads, and the user never upgrades, then at most, they should only get shown the dialog once at log-in time; unless their quota was increased and they hit the new limit, or gnome-settings-daemon crashes (which is a larger bug and possibly not ours).

review: Disapprove

[dobey (dobey)]

Sigh. :(

review: Abstain

[lp:~alecu/ubuntuone-client/restrain-out-of-space-dialog]

793. By Alejandro J. Cura on 2011-01-06

GTK_DIALOG_NO_SEPARATOR has been deprecated in GTK+ 2.22 and will be removed in GTK+ 3

[Natalia Bidart (nataliabidart)]

The code makes sense though I'm not an experienced gnome dev. Works like a charm.

review: Approve (fieldtest)

[dobey (dobey)]

Part of the problem is also that the dialog is destroyed and shown again, even if it's already shown. Instead it needs to be in a static variable, and if already shown, simply presented again.

review: Needs Fixing

[lp:~alecu/ubuntuone-client/restrain-out-of-space-dialog]

794. By Alejandro J. Cura on 2011-01-06

only nag once per day

[Roberto Alsina (ralsina)]

rodney: that's for another bug, isn't it?

[Roberto Alsina (ralsina)]

> Part of the problem is also that the dialog is destroyed and shown again, even
> if it's already shown. Instead it needs to be in a static variable, and if
> already shown, simply presented again.

Now replying the right way ;-)

That's not the issue in this bug. You can add another for that and assign it to alecu or yourself, if you want.

[Alejandro J. Cura (alecu)]

> Part of the problem is also that the dialog is destroyed and shown again, even
> if it's already shown. Instead it needs to be in a static variable, and if
> already shown, simply presented again.

That's a valid point, but it's not something that's being done on this branch.
Would you mind opening a different bug for it?

[dobey (dobey)]

I'm not sure it's necessarily another bug entirely. Rather, after a little bit more thought, I think it is part of the fundamental flaw in the code that leads to this issue.

With 3000 files say, that would cauase the dialog to appear, instead of just show()ing the same dialog 3000 times, it is instead destroyed and then created again, over and over. This means it creates a state of constant flicker until the last display of the dialog, and the action happens fast enough that the user can't click "Cancel" in the middle. Destroying and creating the dialog so many times and so fast, consumes a lot of resources. While this isn't necessarily the primary fundamental flaw, I think it exposes another flaw. That is the code does not wait for all the events to coalesce, so that only one dialog is shown, at the end of the spam of messages from DBus.

While not necessarily a total fix, I do think it would be better than this patch, and be more of a step in the right direction for the total fix. And since the real target of this fix is an SRU for Maverick, I do think we need to exercise more caution in deciding what goes in, and try to minimize the change set before rushing it in.

[dobey (dobey)]

Changing this back to disapprove, and marking as rejected, since I proposed a better solution which is now approved.

review: Disapprove

Unmerged revisions

794. By Alejandro J. Cura on 2011-01-06

only nag once per day

793. By Alejandro J. Cura on 2011-01-06

GTK_DIALOG_NO_SEPARATOR has been deprecated in GTK+ 2.22 and will be removed in GTK+ 3

792. By Alejandro J. Cura on 2011-01-06

include the bug number in the branch

791. By Alejandro J. Cura on 2011-01-06

show out-of-space dialog with an interval, 0 to disable

790. By Alejandro J. Cura on 2011-01-06

disable dialog by a checkbox

789. By Alejandro J. Cura on 2011-01-05

use a gconf-client for the plugin

788. By Alejandro J. Cura on 2011-01-05

show each message only once per hour

787. By Alejandro J. Cura on 2011-01-05

was never shown recently

=== modified file 'gsd-plugin/gsd-ubuntuone.c'
--- gsd-plugin/gsd-ubuntuone.c	2010-10-04 18:47:52 +0000
+++ gsd-plugin/gsd-ubuntuone.c	2011-01-06 22:07:46 +0000
@@ -21,6 +21,7 @@
 
 #include "config.h"
 
+#include <glib.h>
 #include <glib/gi18n-lib.h>
 #include <gmodule.h>
 #include <gconf/gconf-client.h>
@@ -41,6 +42,9 @@
 #define UPGRADE_SUBSCRIPTION_TEXT _("Upgrade Subscription")
 #define UPGRADE_SUBSCRIPTION_URI "http://one.ubuntu.com/plans/"
 #define CHECKED_BOOKMARK_FILE_KEY "/apps/gnome_settings_daemon/plugins/ubuntuone/checked_bookmark_file"
+#define OUT_OF_SPACE_INTERVAL_KEY "/apps/gnome_settings_daemon/plugins/ubuntuone/out_of_space_warnings_interval"
+/* default is to show at most once per day */
+#define DEFAULT_OUT_OF_SPACE_INTERVAL 86400
 
 static void
 dialog_closed_callback (GtkDialog *dialog,
@@ -54,12 +58,45 @@
 	plugin->out_of_space_dialog = NULL;
 }
 
+static gboolean
+was_shown_recently (GsdUbuntuOne *plugin, const gchar *dialog_body, gint interval_between_warnings)
+{
+	gboolean result = FALSE;
+	GTimeVal current_time, *last_time_shown;
+	last_time_shown = g_hash_table_lookup (plugin->map_last_times_shown,
+					       dialog_body);
+	if (last_time_shown != NULL) {
+		g_get_current_time (&current_time);
+		if (last_time_shown->tv_sec + interval_between_warnings > current_time.tv_sec) {
+		    result = TRUE;
+		}
+	}
+	return result;
+}
+
+static void
+set_just_shown (GsdUbuntuOne *plugin, const gchar *dialog_body)
+{
+	GTimeVal *current_time = g_new0 (GTimeVal, 1);
+	g_get_current_time (current_time);
+	g_hash_table_insert (plugin->map_last_times_shown,
+			     g_strdup(dialog_body),
+			     current_time);
+}
+
 static void
 show_out_of_space_dialog (GsdUbuntuOne *plugin,
 			  const gchar *title,
 			  const gchar *body,
-			  gboolean show_upgrade_link)
+			  gboolean show_upgrade_link,
+			  gint interval_between_warnings)
 {
+	if (was_shown_recently (plugin, body, interval_between_warnings)) {
+		g_debug ("not showing notification: %s "
+		         "(as it was shown recently)", body);
+		return;
+	}
+
 	if (plugin->out_of_space_dialog != NULL) {
 		gtk_widget_destroy (GTK_WIDGET (plugin->out_of_space_dialog));
 		plugin->out_of_space_dialog = NULL;
@@ -68,7 +105,7 @@
 	g_debug ("notification: %s - %s", title, body);
 
 	plugin->out_of_space_dialog = gtk_message_dialog_new (NULL,
-							      GTK_DIALOG_NO_SEPARATOR,
+							      0,
 							      GTK_MESSAGE_WARNING,
 							      GTK_BUTTONS_CLOSE,
 							      "%s",
@@ -92,17 +129,35 @@
 	g_signal_connect (G_OBJECT (plugin->out_of_space_dialog), "response",
                           G_CALLBACK (dialog_closed_callback), plugin);
 	gtk_widget_show (plugin->out_of_space_dialog);
-}
-
+	set_just_shown (plugin, body);
+}
+
+static gint
+get_interval_between_warnings (GsdUbuntuOne *plugin)
+{
+        if (gconf_client_get (plugin->conf_client, OUT_OF_SPACE_INTERVAL_KEY, NULL) == NULL) {
+		gconf_client_set_int (plugin->conf_client, OUT_OF_SPACE_INTERVAL_KEY,
+				      DEFAULT_OUT_OF_SPACE_INTERVAL, NULL);
+	}
+
+        return gconf_client_get_int (plugin->conf_client, OUT_OF_SPACE_INTERVAL_KEY, NULL);
+}
 
 static void
 quota_exceeded_callback (SyncdaemonDaemon *daemon,
 			 GHashTable *file_info,
 			 gpointer user_data)
 {
+	gint interval_between_warnings;
 	gchar * volume_type;
 	GsdUbuntuOne *plugin = GSD_UBUNTUONE (user_data);
 
+	interval_between_warnings = get_interval_between_warnings (plugin);
+	if (interval_between_warnings == 0) {
+		g_debug ("Not showing out of space dialog due to gconf setting.\n");
+		return;
+	}
+
 	volume_type = g_hash_table_lookup (file_info, "type");
 	if (g_strcmp0 (volume_type, "Share") == 0) {
 		gchar * other_visible_name, * path, * message;
@@ -110,10 +165,12 @@
 		other_visible_name = g_hash_table_lookup (file_info, "other_visible_name");
 		path = g_hash_table_lookup (file_info, "path");
 		message = g_strdup_printf (NO_SPACE_SHARE, path, other_visible_name);
-		show_out_of_space_dialog (plugin, NO_SPACE_TITLE, message, FALSE);
+		show_out_of_space_dialog (plugin, NO_SPACE_TITLE, message, FALSE,
+					  interval_between_warnings);
 		g_free (message);
 	} else {
-		show_out_of_space_dialog (plugin, NO_SPACE_TITLE, NO_SPACE, TRUE);
+		show_out_of_space_dialog (plugin, NO_SPACE_TITLE, NO_SPACE, TRUE,
+					  interval_between_warnings);
 	}
 }
 
@@ -122,6 +179,7 @@
 {
         gchar *contents = NULL;
         GError *error = NULL;
+        GsdUbuntuOne *plugin = GSD_UBUNTUONE(user_data);
 
         g_file_load_contents_finish (G_FILE (source), res, &contents, NULL, NULL, &error);
         if (error == NULL) {
@@ -151,10 +209,8 @@
                                                      NULL,
                                                      FALSE,
                                                      0, NULL, NULL, &error)) {
-                                GConfClient *conf_client;
 
-                                conf_client = gconf_client_get_default ();
-                                gconf_client_set_bool (conf_client, CHECKED_BOOKMARK_FILE_KEY, TRUE, NULL);
+                                gconf_client_set_bool (plugin->conf_client, CHECKED_BOOKMARK_FILE_KEY, TRUE, NULL);
                         } else {
                                 g_warning ("Could not save bookmarks file: %s\n", error->message);
                                 g_error_free (error);
@@ -171,15 +227,13 @@
 }
 
 static void
-check_bookmark_file (void)
+check_bookmark_file (GsdUbuntuOne *plugin)
 {
         gchar *filename;
-        GConfClient *conf_client;
         GError *error = NULL;
 
         /* We only check the bookmark file if we haven't already done so */
-        conf_client = gconf_client_get_default ();
-        if (!gconf_client_get_bool (conf_client, CHECKED_BOOKMARK_FILE_KEY, &error)) {
+        if (!gconf_client_get_bool (plugin->conf_client, CHECKED_BOOKMARK_FILE_KEY, &error)) {
                 if (error == NULL) {
                         gchar *u1_folder;
 
@@ -191,7 +245,7 @@
                                         GFile *file;
 
                                         file = g_file_new_for_path (filename);
-                                        g_file_load_contents_async (file, NULL, bookmark_file_loaded, NULL);
+                                        g_file_load_contents_async (file, NULL, bookmark_file_loaded, plugin);
 
                                         g_object_unref (G_OBJECT (file));
                                         g_free (filename);
@@ -204,14 +258,13 @@
                         g_error_free (error);
                 }
         }
-
-        g_object_unref (conf_client);
 }
 
 static void
 credentials_found_cb (SyncdaemonAuthentication *auth, SyncdaemonCredentials *credentials, gpointer user_data)
 {
-        check_bookmark_file ();
+        GsdUbuntuOne *plugin = GSD_UBUNTUONE (user_data);
+        check_bookmark_file (plugin);
 }
 
 static gboolean
@@ -225,16 +278,20 @@
 
 	plugin->syncdaemon = syncdaemon_daemon_new ();
 	plugin->out_of_space_dialog = NULL;
+	plugin->map_last_times_shown = g_hash_table_new_full (g_str_hash,
+							      g_str_equal,
+							      g_free, g_free);
+	plugin->conf_client = gconf_client_get_default ();
 	g_signal_connect (G_OBJECT (plugin->syncdaemon), "quota_exceeded",
 			  G_CALLBACK (quota_exceeded_callback), plugin);
 
         /* Check for authentication */
         auth = syncdaemon_daemon_get_authentication (plugin->syncdaemon);
         if (syncdaemon_authentication_has_credentials (auth))
-                check_bookmark_file ();
+                check_bookmark_file (plugin);
         else
                 g_signal_connect (auth, "credentials_found",
-                                  G_CALLBACK (credentials_found_cb), NULL);
+                                  G_CALLBACK (credentials_found_cb), plugin);
 
 	return FALSE;
 }
@@ -259,6 +316,12 @@
 	GsdUbuntuOneClass *klass = GSD_UBUNTUONE_GET_CLASS (object);
 	GObjectClass *parent_class = G_OBJECT_CLASS (klass);
 
+	if (plugin->conf_client != NULL)
+	      g_object_unref (plugin->conf_client);
+
+	if (plugin->map_last_times_shown != NULL)
+	      g_hash_table_destroy (plugin->map_last_times_shown);
+
 	if (plugin->syncdaemon != NULL)
 	      g_object_unref (plugin->syncdaemon);
 

=== modified file 'gsd-plugin/gsd-ubuntuone.h'
--- gsd-plugin/gsd-ubuntuone.h	2010-09-30 13:38:15 +0000
+++ gsd-plugin/gsd-ubuntuone.h	2011-01-06 22:07:46 +0000
@@ -25,6 +25,7 @@
 #include <glib.h>
 #include <glib-object.h>
 #include <gmodule.h>
+#include <gconf/gconf-client.h>
 
 #include "gnome-settings-daemon/gnome-settings-plugin.h"
 #include <libsyncdaemon/libsyncdaemon.h>
@@ -48,6 +49,8 @@
 	/*< private >*/
 	SyncdaemonDaemon *syncdaemon;
 	GtkWidget *out_of_space_dialog;
+	GHashTable *map_last_times_shown;
+	GConfClient *conf_client;
 };
 
 struct _GsdUbuntuOneClass

=== added file 'gsd-plugin/test-flood.py'
--- gsd-plugin/test-flood.py	1970-01-01 00:00:00 +0000
+++ gsd-plugin/test-flood.py	2011-01-06 22:07:46 +0000
@@ -0,0 +1,95 @@
+#!/usr/bin/env python
+# -*- encoding: utf-8 -*-
+#
+# Authors: Alejandro J. Cura <[e-mail]>
+#
+# Copyright (C) 2010 Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+#
+
+
+import gobject
+
+import dbus
+import dbus.service
+import dbus.mainloop.glib
+
+import sys
+
+OBJECT_PATH = "/"
+OBJECT_NAME = "com.ubuntuone.SyncDaemon"
+OBJECT_INTERFACE = "com.ubuntuone.SyncDaemon.SyncDaemon"
+SAMPLE_VOLUME_INFO = {
+    "volume_id": "sample_volume_id",
+    "owner_id": "sample_owner_id",
+    "volume_name": "sample_volume_name",
+    "type": "UDF",
+}
+SAMPLE_SHARE_INFO = {
+    "volume_id": "sample_volume_id",
+    "owner_id": "sample_owner_id",
+    "volume_name": "sample_volume_name",
+    "type": "Share",
+    "other_visible_name": "Some Body",
+    "path": "~/A shared folder",
+}
+SAMPLE_SHARE_INFO2 = {
+    "volume_id": "sample_volume_id",
+    "owner_id": "sample_owner_id",
+    "volume_name": "sample_volume_name",
+    "type": "Share",
+    "other_visible_name": "Some Other Body",
+    "path": "~/A shared folder",
+}
+
+class SyncDaemonMocker(dbus.service.Object):
+    def __init__(self, conn, object_path=OBJECT_PATH):
+        dbus.service.Object.__init__(self, conn, object_path)
+
+    @dbus.service.signal(OBJECT_INTERFACE)
+    def QuotaExceeded(self, volume_info):
+        pass
+
+    @dbus.service.method(OBJECT_INTERFACE)
+    def emitQuotaExceeded(self):
+        self.QuotaExceeded(SAMPLE_VOLUME_INFO)
+        return 'Signal emitted'
+
+    @dbus.service.method(OBJECT_INTERFACE, in_signature='', out_signature='')
+    def Quit(self):
+        main_loop.quit()
+
+if __name__ == '__main__':
+    dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+    bus = dbus.SessionBus()
+    name = dbus.service.BusName(OBJECT_NAME, bus)
+    object = SyncDaemonMocker(bus)
+
+    main_loop = gobject.MainLoop()
+
+    if "-d" not in sys.argv:
+        for n in range(30):
+            gobject.timeout_add(1000 * n, object.QuotaExceeded,
+                                SAMPLE_VOLUME_INFO)
+            if n - 3 >= 0:
+                gobject.timeout_add(1000 * n, object.QuotaExceeded,
+                                    SAMPLE_SHARE_INFO)
+            if n - 6 >= 0:
+                gobject.timeout_add(1000 * n, object.QuotaExceeded,
+                                    SAMPLE_SHARE_INFO2)
+        gobject.timeout_add(38000, main_loop.quit)
+
+    main_loop.run()

=== modified file 'nautilus/test-contacts-picker.c'
--- nautilus/test-contacts-picker.c	2010-10-07 14:19:53 +0000
+++ nautilus/test-contacts-picker.c	2011-01-06 22:07:46 +0000
@@ -77,7 +77,7 @@
 	/* Create the main window */
 	window = gtk_dialog_new_with_buttons ("Test contacts picker",
 					      NULL,
-					      GTK_DIALOG_NO_SEPARATOR,
+					      0,
 					      GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
 					      GTK_STOCK_OK, GTK_RESPONSE_OK,
 					      NULL);