Merge lp:~sinzui/bzr-gtk/precise-diff-0 into lp:bzr-gtk

Proposed by Curtis Hovey on 2012-01-23
Status: Merged
Merged at revision: 772
Proposed branch: lp:~sinzui/bzr-gtk/precise-diff-0
Merge into: lp:bzr-gtk
Diff against target: 187 lines (+65/-23)
4 files modified
diff.py (+13/-4)
tests/__init__.py (+19/-0)
tests/test_commit.py (+1/-19)
tests/test_diff.py (+32/-0)
To merge this branch: bzr merge lp:~sinzui/bzr-gtk/precise-diff-0
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) 2012-01-23 Approve on 2012-01-23
Review via email: mp+89759@code.launchpad.net

Commit message

Do not update the DiffWidget when it is being destroyed.

Description of the change

This is a fix for bug 914363, which is private so I cannot see it.
It may not even be reported against bzr-gtk. Everyone can see my
bug 920525 that Apport says is a duplicate.

This is a bzr-gtk gdiff issue. The diff works, but an error happens when
the window is closed. This may be the same kind of issue I fixed in
gcommit where the callback is called while the window is being
destroyed. In that case, The treeview still existed, but it's model and
state were None.

Traceback (most recent call last):
  File "/home/curtis/.bazaar/plugins/gtk/diff.py", line 391, in _treeview_cursor_cb
    specific_files = [ self.model[path][1] ]
  File "/usr/lib/python2.7/dist-packages/gi/overrides/Gtk.py", line 730, in __getitem__
    aiter = self.get_iter(key)
  File "/usr/lib/python2.7/dist-packages/gi/overrides/Gtk.py", line 744, in get_iter
    path = TreePath(path)
  File "/usr/lib/python2.7/dist-packages/gi/overrides/Gtk.py", line 1110, in __new__
    if len(path) == 0:
TypeError: object of type 'NoneType' has no len()

--------------------------------------------------------------------

RULES

    * Do not attempt to update the diff_view if the path is None.
    * Move the MockMethod helper to a common module so that it can be used
      by any test case
    * Update the DiffWidget to guard the calls to show(), subclasses the
      widget so that tests can run without rendering.

QA

    Using Precise:
    * Branch any project
    * bzr gdiff
    * See no changes
    * close the window
    * Verify there is no traceback in the console.

To post a comment you must log in.
Jelmer Vernooij (jelmer) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'diff.py'
2--- diff.py 2011-09-05 03:44:26 +0000
3+++ diff.py 2012-01-23 18:27:25 +0000
4@@ -284,6 +284,9 @@
5 """Diff widget
6
7 """
8+
9+ SHOW_WIDGETS = True
10+
11 def __init__(self):
12 super(DiffWidget, self).__init__()
13
14@@ -292,7 +295,8 @@
15 scrollwin.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC)
16 scrollwin.set_shadow_type(Gtk.ShadowType.IN)
17 self.pack1(scrollwin)
18- scrollwin.show()
19+ if self.SHOW_WIDGETS:
20+ scrollwin.show()
21
22 self.model = Gtk.TreeStore(str, str)
23 self.treeview = Gtk.TreeView(model=self.model)
24@@ -300,7 +304,8 @@
25 self.treeview.set_search_column(1)
26 self.treeview.connect("cursor-changed", self._treeview_cursor_cb)
27 scrollwin.add(self.treeview)
28- self.treeview.show()
29+ if self.SHOW_WIDGETS:
30+ self.treeview.show()
31
32 cell = Gtk.CellRendererText()
33 cell.set_property("width-chars", 20)
34@@ -321,7 +326,8 @@
35 if getattr(self, 'diff_view', None) is None:
36 self.diff_view = DiffFileView()
37 self.pack2(self.diff_view)
38- self.diff_view.show()
39+ if self.SHOW_WIDGETS:
40+ self.diff_view.show()
41 for oldname, newname, patch in sections:
42 self.diff_view._diffs[newname] = str(patch)
43 if newname is None:
44@@ -338,7 +344,8 @@
45 if getattr(self, 'diff_view', None) is None:
46 self.diff_view = DiffView()
47 self.pack2(self.diff_view)
48- self.diff_view.show()
49+ if self.SHOW_WIDGETS:
50+ self.diff_view.show()
51 self.diff_view.set_trees(rev_tree, parent_tree)
52 self.rev_tree = rev_tree
53 self.parent_tree = parent_tree
54@@ -388,6 +395,8 @@
55 def _treeview_cursor_cb(self, *args):
56 """Callback for when the treeview cursor changes."""
57 (path, col) = self.treeview.get_cursor()
58+ if path is None:
59+ return
60 specific_files = [ self.model[path][1] ]
61 if specific_files == [ None ]:
62 return
63
64=== modified file 'tests/__init__.py'
65--- tests/__init__.py 2011-09-04 20:04:03 +0000
66+++ tests/__init__.py 2012-01-23 18:27:25 +0000
67@@ -31,3 +31,22 @@
68 basic_tests.addTest(loader.loadTestsFromModuleNames(
69 ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
70 return basic_tests
71+
72+
73+class MockMethod():
74+
75+ @classmethod
76+ def bind(klass, test_instance, obj, method_name):
77+ original_method = getattr(obj, method_name)
78+ test_instance.addCleanup(setattr, obj, method_name, original_method)
79+ setattr(obj, method_name, klass())
80+
81+ def __init__(self):
82+ self.called = False
83+ self.args = None
84+ self.kwargs = None
85+
86+ def __call__(self, *args, **kwargs):
87+ self.called = True
88+ self.args = args
89+ self.kwargs = kwargs
90
91=== modified file 'tests/test_commit.py'
92--- tests/test_commit.py 2012-01-22 16:18:06 +0000
93+++ tests/test_commit.py 2012-01-23 18:27:25 +0000
94@@ -36,6 +36,7 @@
95 commitmsgs,
96 )
97 from bzrlib.plugins.gtk.commitmsgs import SavedCommitMessagesManager
98+from bzrlib.plugins.gtk.tests import MockMethod
99
100
101 # TODO: All we need is basic ancestry code to test this, we shouldn't need a
102@@ -143,25 +144,6 @@
103 pass # With no widgets, there are no widgets to fill out
104
105
106-class MockMethod():
107-
108- @classmethod
109- def bind(klass, test_instance, obj, method_name):
110- original_method = getattr(obj, method_name)
111- test_instance.addCleanup(setattr, obj, method_name, original_method)
112- setattr(obj, method_name, klass())
113-
114- def __init__(self):
115- self.called = False
116- self.args = None
117- self.kwargs = None
118-
119- def __call__(self, *args, **kwargs):
120- self.called = True
121- self.args = args
122- self.kwargs = kwargs
123-
124-
125 class TestCommitDialogSimple(tests.TestCaseWithTransport):
126
127 def test_init(self):
128
129=== modified file 'tests/test_diff.py'
130--- tests/test_diff.py 2011-09-03 20:58:34 +0000
131+++ tests/test_diff.py 2012-01-23 18:27:25 +0000
132@@ -18,6 +18,8 @@
133 from cStringIO import StringIO
134 import os
135
136+from gi.repository import Gtk
137+
138 from bzrlib import (
139 conflicts,
140 errors,
141@@ -32,9 +34,13 @@
142 from bzrlib.plugins.gtk.diff import (
143 DiffController,
144 DiffView,
145+ DiffWidget,
146 iter_changes_to_status,
147 MergeDirectiveController,
148 )
149+from bzrlib.plugins.gtk.tests import MockMethod
150+
151+
152 eg_diff = """\
153 === modified file 'tests/test_diff.py'
154 --- tests/test_diff.py 2008-03-11 13:18:28 +0000
155@@ -99,6 +105,32 @@
156 )
157
158
159+
160+class FakeDiffWidget(DiffWidget):
161+
162+ SHOW_WIDGETS = False
163+
164+
165+class TestDiffWidget(tests.TestCaseWithTransport):
166+
167+ def test_treeview_cursor_cb(self):
168+ widget = FakeDiffWidget()
169+ widget.set_diff_text_sections(
170+ [('', None, 'patch1'), ('a', 'a', 'patch2')])
171+ widget.treeview.set_cursor(Gtk.TreePath(path=1), None, False)
172+ widget._treeview_cursor_cb(None)
173+ self.assertTrue('patch2', widget.diff_view.buffer.props.text)
174+
175+ def test_treeview_cursor_cb_with_destroyed_treeview(self):
176+ widget = FakeDiffWidget()
177+ widget.set_diff_text_sections(
178+ [('', None, 'patch1'), ('a', 'a', 'patch2')])
179+ MockMethod.bind(self, widget.diff_view, 'show_diff')
180+ widget.treeview.destroy()
181+ widget._treeview_cursor_cb(None)
182+ self.assertFalse(widget.diff_view.show_diff.called)
183+
184+
185 class MockDiffWidget(object):
186
187 def set_diff_text_sections(self, sections):

Subscribers

People subscribed via source and target branches

to all changes: