Bazaar GTK+ Frontends

Merge lp:~sinzui/bzr-gtk/fix-threads-icons into lp:bzr-gtk/gtk2

  1. fix-threads-icons
  2. Merge into gtk2
Proposed by Curtis Hovey
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: 740
Merged at revision: 738
Proposed branch: lp:~sinzui/bzr-gtk/fix-threads-icons
Merge into: lp:bzr-gtk/gtk2
Diff against target: 268 lines (+110/-21)
6 files modified
avatarproviders.py (+29/-16)
avatarsbox.py (+11/-4)
tests/__init__.py (+1/-0)
tests/test_annotate_config.py (+8/-1)
tests/test_avatarsbox.py (+47/-0)
tests/test_revisionview.py (+14/-0)
To merge this branch: bzr merge lp:~sinzui/bzr-gtk/fix-threads-icons
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Approve
Review via email: mp+73975@code.launchpad.net

Description of the change

Fix running threads and missing images reported by test suite.

added:
  tests/test_avatarsbox.py
modified:
  avatarproviders.py
  avatarsbox.py
  tests/__init__.py
  tests/test_annotate_config.py
  tests/test_revisionview.py

Setup the window before the working directory is changed, or patch the window
to use a fake implementation of icon_path that know where the icons are.
  tests/test_annotate_config.py
  tests/test_revisionview.py

AvatarDownloaderWorker and its queue were leaving thread behind.
* I refactored the code to just use __stop and removed __end_thread.
* The fix for the worker was to ensure that stop was called during the
  widget's destroy event.
* The queue was troublesome because the worker's run() method was blocking on
  it--join() would never return.
* The fix was to not start the thread until something was queued,
* The stop() method dequeues the avatars so that the queue knows it
  is finished.
* The run() method uses a non-blocking call to something from the queue.
  The loop recovered from the Queue.Empty error.
  avatarproviders.py
  avatarsbox.py
  tests/test_annotate_config.py
  tests/test_revisionview.py

ADDENDUM: The __eq__ method was checking the widget's name attr
(which is always None) instead of the username attr.
  tests/__init__.py
  tests/test_avatarsbox.py
  avatarsbox.py

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Curtis,

Thanks for fixing this. You should be able to land this yourself, now that you're in the ~bzr-gtk team.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'avatarproviders.py'
2--- avatarproviders.py 2011-04-06 14:50:40 +0000
3+++ avatarproviders.py 2011-09-03 22:41:25 +0000
4@@ -46,7 +46,7 @@
5
6 def __init__(self, provider_method):
7 """Constructor
8-
9+
10 :param provider_method: Provider method that returns fields
11 to send with the request.
12 """
13@@ -55,12 +55,18 @@
14 self.__queue = Queue.Queue()
15
16 self.__provider_method = provider_method
17- self.__end_thread = False
18
19 def stop(self):
20 """ Stop this worker """
21- self.__end_thread = True
22 self.__stop.set()
23+ while self.__queue.qsize() > 0:
24+ self.__queue.get_nowait()
25+ self.__queue.task_done()
26+ self.__queue.join()
27+
28+ @property
29+ def is_running(self):
30+ return not self.__stop.is_set()
31
32 def set_callback_method(self, method):
33 """ Fire the given callback method when treatment is finished """
34@@ -71,20 +77,27 @@
35
36 This id_field is for example with Gravatar the email address.
37 """
38- self.__queue.put(id_field)
39+ if self.is_running:
40+ self.__queue.put(id_field)
41+ if not self.is_alive():
42+ self.start()
43
44 def run(self):
45 """Worker core code. """
46- while not self.__end_thread:
47- id_field = self.__queue.get()
48- # Call provider method to get fields to pass in the request
49- url = self.__provider_method(id_field)
50- # Execute the request
51- response = urllib2.urlopen(url)
52- # Fire the callback method
53- if not self.__callback_method is None:
54- self.__callback_method(response, id_field)
55- self.__queue.task_done()
56+ while self.is_running:
57+ try:
58+ id_field = self.__queue.get_nowait()
59+ # Call provider method to get fields to pass in the request
60+ url = self.__provider_method(id_field)
61+ # Execute the request
62+ response = urllib2.urlopen(url)
63+ # Fire the callback method
64+ if not self.__callback_method is None:
65+ self.__callback_method(response, id_field)
66+ self.__queue.task_done()
67+ except Queue.Empty:
68+ # There is no more work to do.
69+ pass
70
71
72 class AvatarProviderGravatar(AvatarProvider):
73@@ -97,6 +110,6 @@
74 """Return a gravatar URL for an email address.."""
75 return self.get_base_url() + \
76 urllib.urlencode({
77- 'gravatar_id':hashlib.md5(email.lower()).hexdigest(),
78- 'size':str(self.size)
79+ 'gravatar_id': hashlib.md5(email.lower()).hexdigest(),
80+ 'size': str(self.size)
81 })
82
83=== modified file 'avatarsbox.py'
84--- avatarsbox.py 2011-04-10 18:44:39 +0000
85+++ avatarsbox.py 2011-09-03 22:41:25 +0000
86@@ -38,7 +38,7 @@
87
88 def __eq__(self, other):
89 return (self.apparent_username == other.apparent_username and
90- self.name == other.name and
91+ self.username == other.username and
92 self.email == other.email)
93
94 def show_spinner(self):
95@@ -184,13 +184,19 @@
96 # This callback method should be fired by all workers when a request
97 # is done.
98 self.__worker.set_callback_method(self._update_avatar_from_response)
99- self.__worker.start()
100+ self.connect('destroy', self.on_destroy)
101+
102+ def on_destroy(self, widget):
103+ self.__worker.stop()
104+ if self.__worker.is_alive():
105+ self.__worker.join()
106
107 def add(self, username, role):
108 """Add the given username in the role box and add in the worker queue.
109 """
110 avatar = Avatar(username)
111- if (role == "author" and not self._role_box_for("committer").showing(avatar)) or role == "committer":
112+ is_shown = self._role_box_for("committer").showing(avatar)
113+ if (role == "author" and not is_shown) or role == "committer":
114 if self._role_box_for(role).append_avatars_with(avatar):
115 self.__worker.queue(avatar.email)
116
117@@ -234,7 +240,8 @@
118 loader.close()
119
120 for role in ["committer", "author"]:
121- self._role_box_for(role).and_avatar_email(email).update_avatar_image_from_pixbuf_loader(loader)
122+ self._role_box_for(role).and_avatar_email(
123+ email).update_avatar_image_from_pixbuf_loader(loader)
124
125 def _role_box_for(self, role):
126 """ Return the gtk.HBox for the given role """
127
128=== modified file 'tests/__init__.py'
129--- tests/__init__.py 2011-02-18 13:01:03 +0000
130+++ tests/__init__.py 2011-09-03 22:41:25 +0000
131@@ -18,6 +18,7 @@
132 def load_tests(basic_tests, module, loader):
133 testmod_names = [
134 'test_annotate_config',
135+ 'test_avatarsbox',
136 'test_commit',
137 'test_diff',
138 'test_history',
139
140=== modified file 'tests/test_annotate_config.py'
141--- tests/test_annotate_config.py 2009-05-07 11:48:03 +0000
142+++ tests/test_annotate_config.py 2011-09-03 22:41:25 +0000
143@@ -25,11 +25,18 @@
144 gannotate,
145 )
146
147+
148 class TestConfig(tests.TestCaseInTempDir):
149
150 def setUp(self):
151+ # Create an instance before the env is changed so that
152+ # icon lookups work.
153+ self.window = gannotate.GAnnotateWindow()
154 super(TestConfig, self).setUp()
155- self.window = gannotate.GAnnotateWindow()
156+
157+ def tearDown(self):
158+ self.window.destroy()
159+ super(TestConfig, self).tearDown()
160
161 def test_create_initial_config(self):
162 """We can create a config even without a prior conf file"""
163
164=== added file 'tests/test_avatarsbox.py'
165--- tests/test_avatarsbox.py 1970-01-01 00:00:00 +0000
166+++ tests/test_avatarsbox.py 2011-09-03 22:41:25 +0000
167@@ -0,0 +1,47 @@
168+# Copyright (C) 2011 Curtis Hovey <sinzui.is@verizon.net>
169+#
170+# This program is free software; you can redistribute it and/or modify
171+# it under the terms of the GNU General Public License as published by
172+# the Free Software Foundation; either version 2 of the License, or
173+# (at your option) any later version.
174+#
175+# This program is distributed in the hope that it will be useful,
176+# but WITHOUT ANY WARRANTY; without even the implied warranty of
177+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
178+# GNU General Public License for more details.
179+#
180+# You should have received a copy of the GNU General Public License
181+# along with this program; if not, write to the Free Software
182+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
183+
184+"""Test the AvatarsBox functionality."""
185+
186+from bzrlib import (
187+ tests,
188+ )
189+
190+from bzrlib.plugins.gtk.avatarsbox import Avatar
191+
192+
193+class TestAvatar(tests.TestCase):
194+
195+ def test__init(self):
196+ apparent_username = 'Random J User <randomjuser@eg.dom>'
197+ avatar = Avatar(apparent_username)
198+ self.assertEqual(apparent_username, avatar.apparent_username)
199+ self.assertEqual('Random J User', avatar.username)
200+ self.assertEqual('randomjuser@eg.dom', avatar.email)
201+ self.assertEqual(None, avatar.image)
202+
203+ def test_eq_true(self):
204+ apparent_username = 'Random J User <randomjuser@eg.dom>'
205+ avatar_1 = Avatar(apparent_username)
206+ avatar_2 = Avatar(apparent_username)
207+ self.assertTrue(avatar_1 == avatar_2)
208+
209+ def test_eq_false(self):
210+ apparent_username = 'Random J User <randomjuser@eg.dom>'
211+ avatar_1 = Avatar(apparent_username)
212+ avatar_2 = Avatar(apparent_username)
213+ avatar_2.username = 'changed'
214+ self.assertFalse(avatar_1 == avatar_2)
215
216=== modified file 'tests/test_revisionview.py'
217--- tests/test_revisionview.py 2011-07-28 15:34:17 +0000
218+++ tests/test_revisionview.py 2011-09-03 22:41:25 +0000
219@@ -16,6 +16,8 @@
220
221 """Test the RevisionView functionality."""
222
223+import os
224+
225 from bzrlib import (
226 tests,
227 )
228@@ -27,8 +29,17 @@
229 from bzrlib.plugins.gtk import revisionview
230
231
232+def fake_icon_path(*args):
233+ return os.path.join(
234+ os.path.dirname(__file__), '..', 'icons', *args)
235+
236+
237 class TestPendingRevisions(tests.TestCaseWithMemoryTransport):
238
239+ def setUp(self):
240+ super(TestPendingRevisions, self).setUp()
241+ self.overrideAttr(revisionview, 'icon_path', new=fake_icon_path)
242+
243 def assertBufferText(self, text, buffer):
244 """Check the text stored in the buffer."""
245 self.assertEqual(text, buffer.get_text(buffer.get_start_iter(),
246@@ -41,6 +52,7 @@
247 b = builder.get_branch()
248
249 rv = revisionview.RevisionView(b)
250+ self.addCleanup(rv.destroy)
251 rev = b.repository.get_revision('A')
252 rv.set_revision(rev)
253 self.assertEqual(rev.committer, rv.committer.get_text())
254@@ -61,6 +73,7 @@
255 b = tree.branch
256
257 rv = revisionview.RevisionView(b)
258+ self.addCleanup(rv.destroy)
259 rev = b.repository.get_revision('A')
260 rv.set_revision(rev)
261
262@@ -81,6 +94,7 @@
263 b = tree.branch
264
265 rv = revisionview.RevisionView(b)
266+ self.addCleanup(rv.destroy)
267 rev = b.repository.get_revision('A')
268 rv.set_revision(rev)
269

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: