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

Proposed by Martin Pitt on 2011-08-22
Status: Merged
Approved by: Natalia Bidart on 2011-08-25
Approved revision: 206
Merged at revision: 204
Proposed branch: lp:~pitti/ubuntuone-control-panel/gi-fixes
Merge into: lp:ubuntuone-control-panel
Diff against target: 138 lines (+20/-16)
4 files modified
ubuntuone/controlpanel/dbus_service.py (+12/-4)
ubuntuone/controlpanel/dbustests/test_dbus_service.py (+1/-1)
ubuntuone/controlpanel/gui/gtk/gui.py (+2/-7)
ubuntuone/controlpanel/gui/gtk/tests/test_gui.py (+5/-4)
To merge this branch: bzr merge lp:~pitti/ubuntuone-control-panel/gi-fixes
Reviewer Review Type Date Requested Status
Natalia Bidart 2011-08-22 Approve on 2011-08-25
Martin Pitt (community) Abstain on 2011-08-25
dobey (community) Abstain on 2011-08-25
Review via email: mp+72449@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Natalia Bidart (nataliabidart) wrote :

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) wrote :

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) wrote :

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) wrote :

@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.

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: Resubmit
dobey (dobey) :
review: Abstain
Natalia Bidart (nataliabidart) wrote :

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) wrote :

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

review: Abstain
205. By Martin Pitt on 2011-08-25

test_dbus_service.py: Fix mocker to use GObject, too

Martin Pitt (pitti) wrote :

I pushed the mocker and pylint fixes.

206. By Martin Pitt on 2011-08-25

quiesce false-positive pylint warnings

Natalia Bidart (nataliabidart) wrote :

Looks great!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/dbus_service.py'
2--- ubuntuone/controlpanel/dbus_service.py 2011-06-29 17:59:07 +0000
3+++ ubuntuone/controlpanel/dbus_service.py 2011-08-25 16:11:27 +0000
4@@ -20,9 +20,17 @@
5 """Export the control backend thru DBus."""
6
7 from functools import wraps
8+import sys
9
10 import dbus.service
11-import gobject
12+# pylint: disable=E0611
13+# pylint: disable=W0404
14+if 'gobject' in sys.modules:
15+ import gobject as GObject
16+else:
17+ from gi.repository import GObject
18+# pylint: enable=W0404
19+# pylint: enable=E0611
20
21 from dbus.mainloop.glib import DBusGMainLoop
22 from dbus.service import method, signal
23@@ -608,9 +616,9 @@
24
25
26 def run_mainloop(loop=None):
27- """Run the gobject main loop."""
28+ """Run the GObject main loop."""
29 if loop is None:
30- loop = gobject.MainLoop()
31+ loop = GObject.MainLoop()
32 loop.run()
33
34
35@@ -643,7 +651,7 @@
36 """Hook the DBus listeners and start the main loop."""
37 init_mainloop()
38 if register_service():
39- loop = gobject.MainLoop()
40+ loop = GObject.MainLoop()
41 publish_backend(shutdown_func=loop.quit)
42 run_mainloop(loop=loop)
43 else:
44
45=== modified file 'ubuntuone/controlpanel/dbustests/test_dbus_service.py'
46--- ubuntuone/controlpanel/dbustests/test_dbus_service.py 2011-07-05 19:12:10 +0000
47+++ ubuntuone/controlpanel/dbustests/test_dbus_service.py 2011-08-25 16:11:27 +0000
48@@ -107,7 +107,7 @@
49 rs()
50 self.mocker.result(True)
51
52- mainloop = "ubuntuone.controlpanel.dbus_service.gobject.MainLoop"
53+ mainloop = "ubuntuone.controlpanel.dbus_service.GObject.MainLoop"
54 mainloop = self.mocker.replace(mainloop)
55 mainloop()
56 loop = self.mocker.mock()
57
58=== modified file 'ubuntuone/controlpanel/gui/gtk/gui.py'
59--- ubuntuone/controlpanel/gui/gtk/gui.py 2011-08-18 16:30:27 +0000
60+++ ubuntuone/controlpanel/gui/gtk/gui.py 2011-08-25 16:11:27 +0000
61@@ -32,7 +32,6 @@
62 from dbus.mainloop.glib import DBusGMainLoop
63 from ubuntu_sso import networkstate
64 # pylint: disable=E0611,F0401
65-from gi.repository import GLib
66 from ubuntuone.platform.credentials import (
67 APP_NAME as U1_APP_NAME,
68 CredentialsManagementTool,
69@@ -171,10 +170,6 @@
70
71 logger.debug('%s: started.', self.__class__.__name__)
72
73- def humanize(self, int_bytes):
74- """Return a human readble string of 'int_bytes'."""
75- return GLib.format_size_for_display(int_bytes)
76-
77 def _set_warning(self, message, label):
78 """Set 'message' as warning in 'label'."""
79 label.set_markup(WARNING_MARKUP % message)
80@@ -496,7 +491,7 @@
81 scroll_to_cell = True
82 else:
83 free_bytes_str = self.FREE_SPACE
84- free_bytes_args = {'free_space': self.humanize(free_bytes)}
85+ free_bytes_args = {'free_space': humanize(free_bytes)}
86 free_bytes = free_bytes_str % free_bytes_args
87
88 row = (self.ROW_HEADER % (name, free_bytes),
89@@ -1518,7 +1513,7 @@
90 """Backend notifies of account info."""
91 used = int(info['quota_used'])
92 total = int(info['quota_total'])
93- data = {'used': self.humanize(used), 'total': self.humanize(total),
94+ data = {'used': humanize(used), 'total': humanize(total),
95 'percentage': (used / total) * 100}
96 self._update_quota(QUOTA_LABEL % data, data)
97
98
99=== modified file 'ubuntuone/controlpanel/gui/gtk/tests/test_gui.py'
100--- ubuntuone/controlpanel/gui/gtk/tests/test_gui.py 2011-08-18 15:50:38 +0000
101+++ ubuntuone/controlpanel/gui/gtk/tests/test_gui.py 2011-08-25 16:11:27 +0000
102@@ -42,6 +42,7 @@
103 )
104 from ubuntuone.controlpanel.gui.gtk.tests.test_package_manager import (
105 SUCCESS, FAILURE)
106+from ubuntuone.controlpanel.gui import humanize
107
108
109 # Attribute 'yyy' defined outside __init__, access to a protected member
110@@ -196,7 +197,7 @@
111 treeiter = self.ui.volumes_store.get_iter_root()
112 for name, free_bytes, volumes in FAKE_VOLUMES_INFO:
113 name = "%s's" % name if name else gui.MY_FOLDERS
114- free_bytes = self.ui.humanize(int(free_bytes))
115+ free_bytes = humanize(int(free_bytes))
116 header = (name, self.ui.FREE_SPACE % {'free_space': free_bytes})
117
118 # check parent row
119@@ -277,7 +278,7 @@
120 treeiter = self.ui.volumes_store.get_iter_root()
121 for name, free_bytes, volumes in FAKE_VOLUMES_NO_FREE_SPACE_INFO:
122 name = "%s's" % name if name else gui.MY_FOLDERS
123- free_bytes = self.ui.humanize(int(free_bytes))
124+ free_bytes = humanize(int(free_bytes))
125 free_bytes = self.ui.NO_FREE_SPACE % {'free_space': free_bytes}
126
127 # check parent row
128@@ -1912,8 +1913,8 @@
129 used = int(info['quota_used'])
130 total = int(info['quota_total'])
131 percentage = round((used / total) * 100, 2)
132- expected = {'used': self.ui.humanize(used),
133- 'total': self.ui.humanize(total),
134+ expected = {'used': humanize(used),
135+ 'total': humanize(total),
136 'percentage': percentage}
137 msg = gui.QUOTA_LABEL % expected
138 self.assertEqual(self.ui.quota_label.get_text(), msg)

Subscribers

People subscribed via source and target branches