bpo-1612262: IDLE: Class Browser shows nested functions, classes (#2573)

Original patches for code and tests by Guilherme Polo and
Cheryl Sabella, respectively.

3 files changed, 326 insertions, 157 deletions

diff --git a/Lib/idlelib/browser.py b/Lib/idlelib/browser.py
index 4cf4744..1fc04d8 100644
--- a/Lib/idlelib/browser.py
+++ b/Lib/idlelib/browser.py
@@ -19,14 +19,49 @@ from idlelib import pyshell
 from idlelib.tree import TreeNode, TreeItem, ScrolledCanvas
 from idlelib.windows import ListedToplevel
 
+
 file_open = None  # Method...Item and Class...Item use this.
 # Normally pyshell.flist.open, but there is no pyshell.flist for htest.
 
+
+def transform_children(child_dict, modname=None):
+    """Transform a child dictionary to an ordered sequence of objects.
+
+    The dictionary maps names to pyclbr information objects.
+    Filter out imported objects.
+    Augment class names with bases.
+    Sort objects by line number.
+
+    The current tree only calls this once per child_dic as it saves
+    TreeItems once created.  A future tree and tests might violate this,
+    so a check prevents multiple in-place augmentations.
+    """
+    obs = []  # Use list since values should already be sorted.
+    for key, obj in child_dict.items():
+        if modname is None or obj.module == modname:
+            if hasattr(obj, 'super') and obj.super and obj.name == key:
+                # If obj.name != key, it has already been suffixed.
+                supers = []
+                for sup in obj.super:
+                    if type(sup) is type(''):
+                        sname = sup
+                    else:
+                        sname = sup.name
+                        if sup.module != obj.module:
+                            sname = f'{sup.module}.{sname}'
+                    supers.append(sname)
+                obj.name += '({})'.format(', '.join(supers))
+            obs.append(obj)
+    return sorted(obs, key=lambda o: o.lineno)
+
+
 class ClassBrowser:
     """Browse module classes and functions in IDLE.
     """
+    # This class is the base class for pathbrowser.PathBrowser.
+    # Init and close are inherited, other methods are overriden.
 
-    def __init__(self, flist, name, path, _htest=False):
+    def __init__(self, flist, name, path, _htest=False, _utest=False):
         # XXX This API should change, if the file doesn't end in ".py"
         # XXX the code here is bogus!
         """Create a window for browsing a module's structure.
@@ -47,11 +82,12 @@ class ClassBrowser:
                 the tree and subsequently in the children.
         """
         global file_open
-        if not _htest:
+        if not (_htest or _utest):
             file_open = pyshell.flist.open
         self.name = name
         self.file = os.path.join(path[0], self.name + ".py")
         self._htest = _htest
+        self._utest = _utest
         self.init(flist)
 
     def close(self, event=None):
@@ -80,8 +116,9 @@ class ClassBrowser:
         sc.frame.pack(expand=1, fill="both")
         item = self.rootnode()
         self.node = node = TreeNode(sc.canvas, None, item)
-        node.update()
-        node.expand()
+        if not self._utest:
+            node.update()
+            node.expand()
 
     def settitle(self):
         "Set the window title."
@@ -92,6 +129,7 @@ class ClassBrowser:
         "Return a ModuleBrowserTreeItem as the root of the tree."
         return ModuleBrowserTreeItem(self.file)
 
+
 class ModuleBrowserTreeItem(TreeItem):
     """Browser tree for Python module.
 
@@ -115,16 +153,8 @@ class ModuleBrowserTreeItem(TreeItem):
         return "python"
 
     def GetSubList(self):
-        """Return the list of ClassBrowserTreeItem items.
-
-        Each item returned from listclasses is the first level of
-        classes/functions within the module.
-        """
-        sublist = []
-        for name in self.listclasses():
-            item = ClassBrowserTreeItem(name, self.classes, self.file)
-            sublist.append(item)
-        return sublist
+        "Return ChildBrowserTreeItems for children."
+        return [ChildBrowserTreeItem(obj) for obj in self.listchildren()]
 
     def OnDoubleClick(self):
         "Open a module in an editor window when double clicked."
@@ -132,89 +162,44 @@ class ModuleBrowserTreeItem(TreeItem):
             return
         if not os.path.exists(self.file):
             return
-        pyshell.flist.open(self.file)
+        file_open(self.file)
 
     def IsExpandable(self):
         "Return True if Python (.py) file."
         return os.path.normcase(self.file[-3:]) == ".py"
 
-    def listclasses(self):
-        """Return list of classes and functions in the module.
-
-        The dictionary output from pyclbr is re-written as a
-        list of tuples in the form (lineno, name) and
-        then sorted so that the classes and functions are
-        processed in line number order.  The returned list only
-        contains the name and not the line number.  An instance
-        variable self.classes contains the pyclbr dictionary values,
-        which are instances of Class and Function.
-        """
+    def listchildren(self):
+        "Return sequenced classes and functions in the module."
         dir, file = os.path.split(self.file)
         name, ext = os.path.splitext(file)
         if os.path.normcase(ext) != ".py":
             return []
         try:
-            dict = pyclbr.readmodule_ex(name, [dir] + sys.path)
+            tree = pyclbr.readmodule_ex(name, [dir] + sys.path)
         except ImportError:
             return []
-        items = []
-        self.classes = {}
-        for key, cl in dict.items():
-            if cl.module == name:
-                s = key
-                if hasattr(cl, 'super') and cl.super:
-                    supers = []
-                    for sup in cl.super:
-                        if type(sup) is type(''):
-                            sname = sup
-                        else:
-                            sname = sup.name
-                            if sup.module != cl.module:
-                                sname = "%s.%s" % (sup.module, sname)
-                        supers.append(sname)
-                    s = s + "(%s)" % ", ".join(supers)
-                items.append((cl.lineno, s))
-                self.classes[s] = cl
-        items.sort()
-        list = []
-        for item, s in items:
-            list.append(s)
-        return list
-
-class ClassBrowserTreeItem(TreeItem):
-    """Browser tree for classes within a module.
+        return transform_children(tree, name)
 
-    Uses TreeItem as the basis for the structure of the tree.
-    """
 
-    def __init__(self, name, classes, file):
-        """Create a TreeItem for the class/function.
+class ChildBrowserTreeItem(TreeItem):
+    """Browser tree for child nodes within the module.
 
-        Args:
-            name: Name of the class/function.
-            classes: Dictonary of Class/Function instances from pyclbr.
-            file: Full path and module name.
+    Uses TreeItem as the basis for the structure of the tree.
+    """
 
-        Instance variables:
-            self.cl: Class/Function instance for the class/function name.
-            self.isfunction: True if self.cl is a Function.
-        """
-        self.name = name
-        # XXX - Does classes need to be an instance variable?
-        self.classes = classes
-        self.file = file
-        try:
-            self.cl = self.classes[self.name]
-        except (IndexError, KeyError):
-            self.cl = None
-        self.isfunction = isinstance(self.cl, pyclbr.Function)
+    def __init__(self, obj):
+        "Create a TreeItem for a pyclbr class/function object."
+        self.obj = obj
+        self.name = obj.name
+        self.isfunction = isinstance(obj, pyclbr.Function)
 
     def GetText(self):
         "Return the name of the function/class to display."
+        name = self.name
         if self.isfunction:
-            return "def " + self.name + "(...)"
+            return "def " + name + "(...)"
         else:
-            return "class " + self.name
+            return "class " + name
 
     def GetIconName(self):
         "Return the name of the icon to display."
@@ -224,95 +209,34 @@ class ClassBrowserTreeItem(TreeItem):
             return "folder"
 
     def IsExpandable(self):
-        "Return True if this class has methods."
-        if self.cl:
-            try:
-                return not not self.cl.methods
-            except AttributeError:
-                return False
-        return None
+        "Return True if self.obj has nested objects."
+        return self.obj.children != {}
 
     def GetSubList(self):
-        """Return Class methods as a list of MethodBrowserTreeItem items.
-
-        Each item is a method within the class.
-        """
-        if not self.cl:
-            return []
-        sublist = []
-        for name in self.listmethods():
-            item = MethodBrowserTreeItem(name, self.cl, self.file)
-            sublist.append(item)
-        return sublist
+        "Return ChildBrowserTreeItems for children."
+        return [ChildBrowserTreeItem(obj)
+                for obj in transform_children(self.obj.children)]
 
     def OnDoubleClick(self):
-        "Open module with file_open and position to lineno, if it exists."
-        if not os.path.exists(self.file):
-            return
-        edit = file_open(self.file)
-        if hasattr(self.cl, 'lineno'):
-            lineno = self.cl.lineno
-            edit.gotoline(lineno)
-
-    def listmethods(self):
-        "Return list of methods within a class sorted by lineno."
-        if not self.cl:
-            return []
-        items = []
-        for name, lineno in self.cl.methods.items():
-            items.append((lineno, name))
-        items.sort()
-        list = []
-        for item, name in items:
-            list.append(name)
-        return list
-
-class MethodBrowserTreeItem(TreeItem):
-    """Browser tree for methods within a class.
-
-    Uses TreeItem as the basis for the structure of the tree.
-    """
-
-    def __init__(self, name, cl, file):
-        """Create a TreeItem for the methods.
-
-        Args:
-            name: Name of the class/function.
-            cl: pyclbr.Class instance for name.
-            file: Full path and module name.
-        """
-        self.name = name
-        self.cl = cl
-        self.file = file
-
-    def GetText(self):
-        "Return the method name to display."
-        return "def " + self.name + "(...)"
-
-    def GetIconName(self):
-        "Return the name of the icon to display."
-        return "python"
-
-    def IsExpandable(self):
-        "Return False as there are no tree items after methods."
-        return False
+        "Open module with file_open and position to lineno."
+        try:
+            edit = file_open(self.obj.file)
+            edit.gotoline(self.obj.lineno)
+        except (OSError, AttributeError):
+            pass
 
-    def OnDoubleClick(self):
-        "Open module with file_open and position at the method start."
-        if not os.path.exists(self.file):
-            return
-        edit = file_open(self.file)
-        edit.gotoline(self.cl.methods[self.name])
 
 def _class_browser(parent): # htest #
     try:
+        file = sys.argv[1]  # If pass file on command line
+        # If this succeeds, unittest will fail.
+    except IndexError:
         file = __file__
-    except NameError:
-        file = sys.argv[0]
-        if sys.argv[1:]:
-            file = sys.argv[1]
-        else:
-            file = sys.argv[0]
+        # Add objects for htest
+        class Nested_in_func(TreeNode):
+            def nested_in_class(): pass
+        def closure():
+            class Nested_in_closure: pass
     dir, file = os.path.split(file)
     name = os.path.splitext(file)[0]
     flist = pyshell.PyShellFileList(parent)
@@ -321,5 +245,7 @@ def _class_browser(parent): # htest #
     ClassBrowser(flist, name, [dir], _htest=True)
 
 if __name__ == "__main__":
+    from unittest import main
+    main('idlelib.idle_test.test_browser', verbosity=2, exit=False)
     from idlelib.idle_test.htest import run
     run(_class_browser)
diff --git a/Lib/idlelib/idle_test/test_browser.py b/Lib/idlelib/idle_test/test_browser.py
new file mode 100644
index 0000000..025e290
--- /dev/null
+++ b/Lib/idlelib/idle_test/test_browser.py
@@ -0,0 +1,242 @@
+""" Test idlelib.browser.
+
+Coverage: 88%
+(Higher, because should exclude 3 lines that .coveragerc won't exclude.)
+"""
+
+import os.path
+import unittest
+import pyclbr
+
+from idlelib import browser, filelist
+from idlelib.tree import TreeNode
+from test.support import requires
+from unittest import mock
+from tkinter import Tk
+from idlelib.idle_test.mock_idle import Func
+from collections import deque
+
+
+class ClassBrowserTest(unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        requires('gui')
+        cls.root = Tk()
+        cls.root.withdraw()
+        cls.flist = filelist.FileList(cls.root)
+        cls.file = __file__
+        cls.path = os.path.dirname(cls.file)
+        cls.module = os.path.basename(cls.file).rstrip('.py')
+        cls.cb = browser.ClassBrowser(cls.flist, cls.module, [cls.path], _utest=True)
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.cb.close()
+        cls.root.destroy()
+        del cls.root, cls.flist, cls.cb
+
+    def test_init(self):
+        cb = self.cb
+        eq = self.assertEqual
+        eq(cb.name, self.module)
+        eq(cb.file, self.file)
+        eq(cb.flist, self.flist)
+        eq(pyclbr._modules, {})
+        self.assertIsInstance(cb.node, TreeNode)
+
+    def test_settitle(self):
+        cb = self.cb
+        self.assertIn(self.module, cb.top.title())
+        self.assertEqual(cb.top.iconname(), 'Class Browser')
+
+    def test_rootnode(self):
+        cb = self.cb
+        rn = cb.rootnode()
+        self.assertIsInstance(rn, browser.ModuleBrowserTreeItem)
+
+    def test_close(self):
+        cb = self.cb
+        cb.top.destroy = Func()
+        cb.node.destroy = Func()
+        cb.close()
+        self.assertTrue(cb.top.destroy.called)
+        self.assertTrue(cb.node.destroy.called)
+        del cb.top.destroy, cb.node.destroy
+
+
+# Same nested tree creation as in test_pyclbr.py except for super on C0.
+mb = pyclbr
+module, fname = 'test', 'test.py'
+f0 = mb.Function(module, 'f0', fname, 1)
+f1 = mb._nest_function(f0, 'f1', 2)
+f2 = mb._nest_function(f1, 'f2', 3)
+c1 = mb._nest_class(f0, 'c1', 5)
+C0 = mb.Class(module, 'C0', ['base'], fname, 6)
+F1 = mb._nest_function(C0, 'F1', 8)
+C1 = mb._nest_class(C0, 'C1', 11, [''])
+C2 = mb._nest_class(C1, 'C2', 12)
+F3 = mb._nest_function(C2, 'F3', 14)
+mock_pyclbr_tree = {'f0': f0, 'C0': C0}
+
+# transform_children(mock_pyclbr_tree, 'test') mutates C0.name.
+
+class TransformChildrenTest(unittest.TestCase):
+
+    def test_transform_children(self):
+        eq = self.assertEqual
+        # Parameter matches tree module.
+        tcl = list(browser.transform_children(mock_pyclbr_tree, 'test'))
+        eq(tcl[0], f0)
+        eq(tcl[1], C0)
+        eq(tcl[1].name, 'C0(base)')
+        # Check that second call does not add second '(base)' suffix.
+        tcl = list(browser.transform_children(mock_pyclbr_tree, 'test'))
+        eq(tcl[1].name, 'C0(base)')
+        # Nothing to traverse if parameter name isn't same as tree module.
+        tn = browser.transform_children(mock_pyclbr_tree, 'different name')
+        self.assertEqual(list(tn), [])
+        # No name parameter.
+        tn = browser.transform_children({'f1': f1, 'c1': c1})
+        self.assertEqual(list(tn), [f1, c1])
+
+
+class ModuleBrowserTreeItemTest(unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.mbt = browser.ModuleBrowserTreeItem(fname)
+
+    def test_init(self):
+        self.assertEqual(self.mbt.file, fname)
+
+    def test_gettext(self):
+        self.assertEqual(self.mbt.GetText(), fname)
+
+    def test_geticonname(self):
+        self.assertEqual(self.mbt.GetIconName(), 'python')
+
+    def test_isexpandable(self):
+        self.assertTrue(self.mbt.IsExpandable())
+
+    def test_listchildren(self):
+        save_rex = browser.pyclbr.readmodule_ex
+        save_tc = browser.transform_children
+        browser.pyclbr.readmodule_ex = Func(result=mock_pyclbr_tree)
+        browser.transform_children = Func(result=[f0, C0])
+        try:
+            self.assertEqual(self.mbt.listchildren(), [f0, C0])
+        finally:
+            browser.pyclbr.readmodule_ex = save_rex
+            browser.transform_children = save_tc
+
+    def test_getsublist(self):
+        mbt = self.mbt
+        mbt.listchildren = Func(result=[f0, C0])
+        sub0, sub1 = mbt.GetSubList()
+        del mbt.listchildren
+        self.assertIsInstance(sub0, browser.ChildBrowserTreeItem)
+        self.assertIsInstance(sub1, browser.ChildBrowserTreeItem)
+        self.assertEqual(sub0.name, 'f0')
+        self.assertEqual(sub1.name, 'C0')
+
+
+    def test_ondoubleclick(self):
+        mbt = self.mbt
+        fopen = browser.file_open = mock.Mock()
+
+        with mock.patch('os.path.exists', return_value=False):
+            mbt.OnDoubleClick()
+            fopen.assert_not_called()
+
+        with mock.patch('os.path.exists', return_value=True):
+            mbt.OnDoubleClick()
+            fopen.assert_called()
+            fopen.called_with(fname)
+
+        del browser.file_open
+
+
+class ChildBrowserTreeItemTest(unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        CBT = browser.ChildBrowserTreeItem
+        cls.cbt_f1 = CBT(f1)
+        cls.cbt_C1 = CBT(C1)
+        cls.cbt_F1 = CBT(F1)
+
+    @classmethod
+    def tearDownClass(cls):
+        del cls.cbt_C1, cls.cbt_f1, cls.cbt_F1
+
+    def test_init(self):
+        eq = self.assertEqual
+        eq(self.cbt_C1.name, 'C1')
+        self.assertFalse(self.cbt_C1.isfunction)
+        eq(self.cbt_f1.name, 'f1')
+        self.assertTrue(self.cbt_f1.isfunction)
+
+    def test_gettext(self):
+        self.assertEqual(self.cbt_C1.GetText(), 'class C1')
+        self.assertEqual(self.cbt_f1.GetText(), 'def f1(...)')
+
+    def test_geticonname(self):
+        self.assertEqual(self.cbt_C1.GetIconName(), 'folder')
+        self.assertEqual(self.cbt_f1.GetIconName(), 'python')
+
+    def test_isexpandable(self):
+        self.assertTrue(self.cbt_C1.IsExpandable())
+        self.assertTrue(self.cbt_f1.IsExpandable())
+        self.assertFalse(self.cbt_F1.IsExpandable())
+
+    def test_getsublist(self):
+        eq = self.assertEqual
+        CBT = browser.ChildBrowserTreeItem
+
+        f1sublist = self.cbt_f1.GetSubList()
+        self.assertIsInstance(f1sublist[0], CBT)
+        eq(len(f1sublist), 1)
+        eq(f1sublist[0].name, 'f2')
+
+        eq(self.cbt_F1.GetSubList(), [])
+
+    def test_ondoubleclick(self):
+        fopen = browser.file_open = mock.Mock()
+        goto = fopen.return_value.gotoline = mock.Mock()
+        self.cbt_F1.OnDoubleClick()
+        fopen.assert_called()
+        goto.assert_called()
+        goto.assert_called_with(self.cbt_F1.obj.lineno)
+        del browser.file_open
+        # Failure test would have to raise OSError or AttributeError.
+
+
+class NestedChildrenTest(unittest.TestCase):
+    "Test that all the nodes in a nested tree are added to the BrowserTree."
+
+    def test_nested(self):
+        queue = deque()
+        actual_names = []
+        # The tree items are processed in breadth first order.
+        # Verify that processing each sublist hits every node and
+        # in the right order.
+        expected_names = ['f0', 'C0',  # This is run before transform test.
+                          'f1', 'c1', 'F1', 'C1()',
+                          'f2', 'C2',
+                          'F3']
+        CBT = browser.ChildBrowserTreeItem
+        queue.extend((CBT(f0), CBT(C0)))
+        while queue:
+            cb = queue.popleft()
+            sublist = cb.GetSubList()
+            queue.extend(sublist)
+            self.assertIn(cb.name, cb.GetText())
+            self.assertIn(cb.GetIconName(), ('python', 'folder'))
+            self.assertIs(cb.IsExpandable(), sublist != [])
+            actual_names.append(cb.name)
+        self.assertEqual(actual_names, expected_names)
+
+
+if __name__ == '__main__':
+    unittest.main(verbosity=2)
diff --git a/Lib/idlelib/pathbrowser.py b/Lib/idlelib/pathbrowser.py
index 6c19508..598dff8 100644
--- a/Lib/idlelib/pathbrowser.py
+++ b/Lib/idlelib/pathbrowser.py
@@ -9,11 +9,12 @@ from idlelib.tree import TreeItem
 
 class PathBrowser(ClassBrowser):
 
-    def __init__(self, flist, _htest=False):
+    def __init__(self, flist, _htest=False, _utest=False):
         """
         _htest - bool, change box location when running htest
         """
         self._htest = _htest
+        self._utest = _utest
         self.init(flist)
 
     def settitle(self):