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

[Curtis Hovey]

Do not update the DiffWidget when it is being destroyed.

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.

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'diff.py'
--- diff.py	2011-09-05 03:44:26 +0000
+++ diff.py	2012-01-23 18:27:25 +0000
@@ -284,6 +284,9 @@
     """Diff widget
 
     """
+
+    SHOW_WIDGETS = True
+
     def __init__(self):
         super(DiffWidget, self).__init__()
 
@@ -292,7 +295,8 @@
         scrollwin.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC)
         scrollwin.set_shadow_type(Gtk.ShadowType.IN)
         self.pack1(scrollwin)
-        scrollwin.show()
+        if self.SHOW_WIDGETS:
+            scrollwin.show()
         
         self.model = Gtk.TreeStore(str, str)
         self.treeview = Gtk.TreeView(model=self.model)
@@ -300,7 +304,8 @@
         self.treeview.set_search_column(1)
         self.treeview.connect("cursor-changed", self._treeview_cursor_cb)
         scrollwin.add(self.treeview)
-        self.treeview.show()
+        if self.SHOW_WIDGETS:
+            self.treeview.show()
 
         cell = Gtk.CellRendererText()
         cell.set_property("width-chars", 20)
@@ -321,7 +326,8 @@
         if getattr(self, 'diff_view', None) is None:
             self.diff_view = DiffFileView()
             self.pack2(self.diff_view)
-        self.diff_view.show()
+        if self.SHOW_WIDGETS:
+            self.diff_view.show()
         for oldname, newname, patch in sections:
             self.diff_view._diffs[newname] = str(patch)
             if newname is None:
@@ -338,7 +344,8 @@
         if getattr(self, 'diff_view', None) is None:
             self.diff_view = DiffView()
             self.pack2(self.diff_view)
-        self.diff_view.show()
+        if self.SHOW_WIDGETS:
+            self.diff_view.show()
         self.diff_view.set_trees(rev_tree, parent_tree)
         self.rev_tree = rev_tree
         self.parent_tree = parent_tree
@@ -388,6 +395,8 @@
     def _treeview_cursor_cb(self, *args):
         """Callback for when the treeview cursor changes."""
         (path, col) = self.treeview.get_cursor()
+        if path is None:
+            return
         specific_files = [ self.model[path][1] ]
         if specific_files == [ None ]:
             return
 
=== modified file 'tests/__init__.py'
--- tests/__init__.py	2011-09-04 20:04:03 +0000
+++ tests/__init__.py	2012-01-23 18:27:25 +0000
@@ -31,3 +31,22 @@
     basic_tests.addTest(loader.loadTestsFromModuleNames(
             ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
     return basic_tests
+
+
+class MockMethod():
+
+    @classmethod
+    def bind(klass, test_instance, obj, method_name):
+        original_method = getattr(obj, method_name)
+        test_instance.addCleanup(setattr, obj, method_name, original_method)
+        setattr(obj, method_name, klass())
+
+    def __init__(self):
+        self.called = False
+        self.args = None
+        self.kwargs = None
+
+    def __call__(self, *args, **kwargs):
+        self.called = True
+        self.args = args
+        self.kwargs = kwargs
 
=== modified file 'tests/test_commit.py'
--- tests/test_commit.py	2012-01-22 16:18:06 +0000
+++ tests/test_commit.py	2012-01-23 18:27:25 +0000
@@ -36,6 +36,7 @@
     commitmsgs,
     )
 from bzrlib.plugins.gtk.commitmsgs import SavedCommitMessagesManager
+from bzrlib.plugins.gtk.tests import MockMethod
 
 
 # TODO: All we need is basic ancestry code to test this, we shouldn't need a
@@ -143,25 +144,6 @@
         pass # With no widgets, there are no widgets to fill out
 
 
-class MockMethod():
-
-    @classmethod
-    def bind(klass, test_instance, obj, method_name):
-        original_method = getattr(obj, method_name)
-        test_instance.addCleanup(setattr, obj, method_name, original_method)
-        setattr(obj, method_name, klass())
-
-    def __init__(self):
-        self.called = False
-        self.args = None
-        self.kwargs = None
-
-    def __call__(self, *args, **kwargs):
-        self.called = True
-        self.args = args
-        self.kwargs = kwargs
-
-
 class TestCommitDialogSimple(tests.TestCaseWithTransport):
 
     def test_init(self):
 
=== modified file 'tests/test_diff.py'
--- tests/test_diff.py	2011-09-03 20:58:34 +0000
+++ tests/test_diff.py	2012-01-23 18:27:25 +0000
@@ -18,6 +18,8 @@
 from cStringIO import StringIO
 import os
 
+from gi.repository import Gtk
+
 from bzrlib import (
     conflicts,
     errors,
@@ -32,9 +34,13 @@
 from bzrlib.plugins.gtk.diff import (
     DiffController,
     DiffView,
+    DiffWidget,
     iter_changes_to_status,
     MergeDirectiveController,
     )
+from bzrlib.plugins.gtk.tests import MockMethod
+
+
 eg_diff = """\
 === modified file 'tests/test_diff.py'
 --- tests/test_diff.py	2008-03-11 13:18:28 +0000
@@ -99,6 +105,32 @@
             )
 
 
+
+class FakeDiffWidget(DiffWidget):
+
+    SHOW_WIDGETS = False
+
+
+class TestDiffWidget(tests.TestCaseWithTransport):
+
+    def test_treeview_cursor_cb(self):
+        widget = FakeDiffWidget()
+        widget.set_diff_text_sections(
+            [('', None, 'patch1'), ('a', 'a', 'patch2')])
+        widget.treeview.set_cursor(Gtk.TreePath(path=1), None, False)
+        widget._treeview_cursor_cb(None)
+        self.assertTrue('patch2', widget.diff_view.buffer.props.text)
+
+    def test_treeview_cursor_cb_with_destroyed_treeview(self):
+        widget = FakeDiffWidget()
+        widget.set_diff_text_sections(
+            [('', None, 'patch1'), ('a', 'a', 'patch2')])
+        MockMethod.bind(self, widget.diff_view, 'show_diff')
+        widget.treeview.destroy()
+        widget._treeview_cursor_cb(None)
+        self.assertFalse(widget.diff_view.show_diff.called)
+
+
 class MockDiffWidget(object):
 
     def set_diff_text_sections(self, sections):