Merge lp:~pitti/ubuntuone-control-panel/gi-fixes into lp:ubuntuone-control-panel

[Martin Pitt]

Make the library work with both GI and static bindings, and consistently use static bindings in the GTK UI. (LP: #829186)

pygobject >= 2.90 is now absolutely zero tolerant against importing both the
static and the GI version of a particular library. This was mostly the case
with 2.28 as well, but did work in some cases (like "import gobject; from
gi.repository import Gtk", in particular for "glib" and "gobject"). These now
cause errors as well.

The GTK frontend has a rather long dependency chain, and porting it to
GTK3/PyGI is going to take a while. It is too risky for Ubuntu Oneiric at this
point. (See lp:~pitti/+junk/u1-control-panel-pygi for my current progress with
this).

On the other hand, the backend already uses the Soup GNOME module, which does
not have an 1:1 equivalent with a static binding. So fix up the last remaining
bit to make this use the GI bindings consistently.

With the fix from this branch we can use the web client library from both
programs with static bindings (GTK frontend), as well as from programs which
use the gobject-introspection bindings (backend).

See bug 829186 for more details.

[Natalia Bidart (nataliabidart)]

The branch looks great, but, instead of reimplementing the humanize function, please use the one defined in ubuntone.controlpanel.gui.

Thanks a lot for helping us!

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

Very likely you will need to import it the humanize method, and make the callers do not use it like an instance method, but as a global function (ie remove the self.humanize calls and replace by calls to humanize directly, there is no point of having it as an instance method).

[dobey (dobey)]

12 +if 'gobject' in sys.modules:
13 + import gobject as GObject
14 +else:
15 + from gi.repository import GObject

I'm pretty sure this if statement will always fail, even if the GI version isn't available, as sys.modules doesn't list what is available, but rather what is actively loaded in the current python process? Or at least, looking at sys.modules in python shell without loading anything else, doesn't seem to give particularly useful results for performing this sort of check.

And it's not clear to me why the GLib.format_size_for_display() was removed. I suppose because you didn't convert it to use Gtk from gi.repository?

review: Needs Fixing

[Martin Pitt (pitti)]

@Rodney: The 'gobject' in sys.modules works very well, and it's indeed intended to check what is already loaded into the Python process; that's exactly what we need to know -- "has anything before us already imported gobject, or any other static binding depending on it?" (such as glib or gtk). This check has been applied to some 10 other library packages, and is also the one that upstream uses:

  http://git.gnome.org/browse/pygobject/commit/?id=65ac35cca8d24f4c1

> And it's not clear to me why the GLib.format_size_for_display() was removed.

Because we can't use GLib (it's the GI binding) when the program importing dbus_service already has loaded static bindings (gobject, glib). Unfortunately the static glib does not export this function, so we can't fallback to the static variant in this case.

I'll change the code to use humanize() instead, I wasn't aware of this.

[lp:~pitti/ubuntuone-control-panel/gi-fixes]

204. By Martin Pitt on 2011-08-23

Entirely drop gui/gtk/gui.py's humanize() method, use the one from ubuntuone.controlpanel.gui instead

[Martin Pitt (pitti)]

review: Needs Resubmitting

[dobey (dobey)]

review: Abstain

[Natalia Bidart (nataliabidart)]

There is a failing test:

[ERROR]
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 321, in run
    testMethod()
  File "/usr/lib/pymodules/python2.7/mocker.py", line 146, in test_method_wrapper
    result = test_method()
  File "/home/nessita/canonical/u1/controlpanel/review_gi-fixes/ubuntuone/controlpanel/dbustests/test_dbus_service.py", line 111, in test_dbus_service_main
    mainloop = self.mocker.replace(mainloop)
  File "/usr/lib/pymodules/python2.7/mocker.py", line 701, in replace
    mock = self.proxy(object, spec, type, name, count, passthrough)
  File "/usr/lib/pymodules/python2.7/mocker.py", line 658, in proxy
    object = getattr(object, attr)
exceptions.AttributeError: 'module' object has no attribute 'gobject'

ubuntuone.controlpanel.dbustests.test_dbus_service.DBusServiceMainTestCase.test_dbus_service_main

Fix for this would be:

=== modified file 'ubuntuone/controlpanel/dbustests/test_dbus_service.py'
--- ubuntuone/controlpanel/dbustests/test_dbus_service.py 2011-07-05 19:12:10 +0000
+++ ubuntuone/controlpanel/dbustests/test_dbus_service.py 2011-08-25 13:38:19 +0000
@@ -107,7 +107,7 @@
         rs()
         self.mocker.result(True)

- mainloop = "ubuntuone.controlpanel.dbus_service.gobject.MainLoop"
+ mainloop = "ubuntuone.controlpanel.dbus_service.GObject.MainLoop"
         mainloop = self.mocker.replace(mainloop)
         mainloop()
         loop = self.mocker.mock()

Please also run ./run-tests after applying the fix since there are lint issues:

ubuntuone/controlpanel/dbus_service.py:
    29: [E0611] No name 'GObject' in module 'gi.repository'
    29: [W0404] Reimport 'GObject' (imported line 25)

review: Needs Fixing

[Martin Pitt (pitti)]

Rodney explained what "Resubmit" really means; reverting my "Resubmit" then.

review: Abstain

[lp:~pitti/ubuntuone-control-panel/gi-fixes]

205. By Martin Pitt on 2011-08-25

test_dbus_service.py: Fix mocker to use GObject, too

[Martin Pitt (pitti)]

I pushed the mocker and pylint fixes.

[lp:~pitti/ubuntuone-control-panel/gi-fixes]

206. By Martin Pitt on 2011-08-25

quiesce false-positive pylint warnings

[Natalia Bidart (nataliabidart)]

Looks great!

review: Approve

=== modified file 'ubuntuone/controlpanel/dbus_service.py'
--- ubuntuone/controlpanel/dbus_service.py	2011-06-29 17:59:07 +0000
+++ ubuntuone/controlpanel/dbus_service.py	2011-08-25 16:11:27 +0000
@@ -20,9 +20,17 @@
 """Export the control backend thru DBus."""
 
 from functools import wraps
+import sys
 
 import dbus.service
-import gobject
+# pylint: disable=E0611
+# pylint: disable=W0404
+if 'gobject' in sys.modules:
+    import gobject as GObject
+else:
+    from gi.repository import GObject
+# pylint: enable=W0404
+# pylint: enable=E0611
 
 from dbus.mainloop.glib import DBusGMainLoop
 from dbus.service import method, signal
@@ -608,9 +616,9 @@
 
 
 def run_mainloop(loop=None):
-    """Run the gobject main loop."""
+    """Run the GObject main loop."""
     if loop is None:
-        loop = gobject.MainLoop()
+        loop = GObject.MainLoop()
     loop.run()
 
 
@@ -643,7 +651,7 @@
     """Hook the DBus listeners and start the main loop."""
     init_mainloop()
     if register_service():
-        loop = gobject.MainLoop()
+        loop = GObject.MainLoop()
         publish_backend(shutdown_func=loop.quit)
         run_mainloop(loop=loop)
     else:
 
=== modified file 'ubuntuone/controlpanel/dbustests/test_dbus_service.py'
--- ubuntuone/controlpanel/dbustests/test_dbus_service.py	2011-07-05 19:12:10 +0000
+++ ubuntuone/controlpanel/dbustests/test_dbus_service.py	2011-08-25 16:11:27 +0000
@@ -107,7 +107,7 @@
         rs()
         self.mocker.result(True)
 
-        mainloop = "ubuntuone.controlpanel.dbus_service.gobject.MainLoop"
+        mainloop = "ubuntuone.controlpanel.dbus_service.GObject.MainLoop"
         mainloop = self.mocker.replace(mainloop)
         mainloop()
         loop = self.mocker.mock()
 
=== modified file 'ubuntuone/controlpanel/gui/gtk/gui.py'
--- ubuntuone/controlpanel/gui/gtk/gui.py	2011-08-18 16:30:27 +0000
+++ ubuntuone/controlpanel/gui/gtk/gui.py	2011-08-25 16:11:27 +0000
@@ -32,7 +32,6 @@
 from dbus.mainloop.glib import DBusGMainLoop
 from ubuntu_sso import networkstate
 # pylint: disable=E0611,F0401
-from gi.repository import GLib
 from ubuntuone.platform.credentials import (
     APP_NAME as U1_APP_NAME,
     CredentialsManagementTool,
@@ -171,10 +170,6 @@
 
         logger.debug('%s: started.', self.__class__.__name__)
 
-    def humanize(self, int_bytes):
-        """Return a human readble string of 'int_bytes'."""
-        return GLib.format_size_for_display(int_bytes)
-
     def _set_warning(self, message, label):
         """Set 'message' as warning in 'label'."""
         label.set_markup(WARNING_MARKUP % message)
@@ -496,7 +491,7 @@
                     scroll_to_cell = True
                 else:
                     free_bytes_str = self.FREE_SPACE
-                free_bytes_args = {'free_space': self.humanize(free_bytes)}
+                free_bytes_args = {'free_space': humanize(free_bytes)}
                 free_bytes = free_bytes_str % free_bytes_args
 
             row = (self.ROW_HEADER % (name, free_bytes),
@@ -1518,7 +1513,7 @@
         """Backend notifies of account info."""
         used = int(info['quota_used'])
         total = int(info['quota_total'])
-        data = {'used': self.humanize(used), 'total': self.humanize(total),
+        data = {'used': humanize(used), 'total': humanize(total),
                 'percentage': (used / total) * 100}
         self._update_quota(QUOTA_LABEL % data, data)
 
 
=== modified file 'ubuntuone/controlpanel/gui/gtk/tests/test_gui.py'
--- ubuntuone/controlpanel/gui/gtk/tests/test_gui.py	2011-08-18 15:50:38 +0000
+++ ubuntuone/controlpanel/gui/gtk/tests/test_gui.py	2011-08-25 16:11:27 +0000
@@ -42,6 +42,7 @@
 )
 from ubuntuone.controlpanel.gui.gtk.tests.test_package_manager import (
     SUCCESS, FAILURE)
+from ubuntuone.controlpanel.gui import humanize
 
 
 # Attribute 'yyy' defined outside __init__, access to a protected member
@@ -196,7 +197,7 @@
         treeiter = self.ui.volumes_store.get_iter_root()
         for name, free_bytes, volumes in FAKE_VOLUMES_INFO:
             name = "%s's" % name if name else gui.MY_FOLDERS
-            free_bytes = self.ui.humanize(int(free_bytes))
+            free_bytes = humanize(int(free_bytes))
             header = (name, self.ui.FREE_SPACE % {'free_space': free_bytes})
 
             # check parent row
@@ -277,7 +278,7 @@
         treeiter = self.ui.volumes_store.get_iter_root()
         for name, free_bytes, volumes in FAKE_VOLUMES_NO_FREE_SPACE_INFO:
             name = "%s's" % name if name else gui.MY_FOLDERS
-            free_bytes = self.ui.humanize(int(free_bytes))
+            free_bytes = humanize(int(free_bytes))
             free_bytes = self.ui.NO_FREE_SPACE % {'free_space': free_bytes}
 
             # check parent row
@@ -1912,8 +1913,8 @@
         used = int(info['quota_used'])
         total = int(info['quota_total'])
         percentage = round((used / total) * 100, 2)
-        expected = {'used': self.ui.humanize(used),
-                    'total': self.ui.humanize(total),
+        expected = {'used': humanize(used),
+                    'total': humanize(total),
                     'percentage': percentage}
         msg = gui.QUOTA_LABEL % expected
         self.assertEqual(self.ui.quota_label.get_text(), msg)