Debug => Stack Viewer (no hot key) could become more useful, aside from #23544, freezing Idle when Debugger active.

1. Don't include idlelib.run.runcode.
2. Don't duplicate globals under each function. Once for the module is enough.
3. Remove +Locals under each function and instead display locals when expanding Clicking a function should display locals without having to click anything else.
4. Add a button to expand all locals?

5. Change title to something better than 'Idle'.  Perhaps 'Idle Shell Traceback'?

5b. Add Python version to end of , as with Shell window.

This will be more than one patch. Some should be easy.

Hello, 

I'm very interested in tackling this issue but this will be my first contribution to Python and I'm quite nervous. I have gone over the developer guide and FAQ but I would appreciate if you could let me know if I should approach this in a certain way.

Welcome. I was hoping a new contributor would see the 'easy' tag. Nervous is okay as long as it leads to heighten carefulness without paralysis.  I am still nervous hitting the [Commit] button.

1. Important. Sign the contributor agreement.
https://www.python.org/psf/contrib/
https://www.python.org/psf/contrib/contrib-form/
I will not look at a patch until you say you have signed.  I will not commit until the '*' appears on your name (perhaps a week).

2. Once you sign, I will give you at least a week to produce something before I work on this by myself.  Patch needs to apply to 3.4, but StackViewer.py should be identical for 3.4/5/6.

3. Post a patch as soon as you do a complete chunk (any of the numbered items).  Start with either 1. or 2. and switch if you get stuck.

Does above answer your question?

It does indeed, thank you very much! I have signed the Contributor Agreement and will start working on this right away.

Hello again,

I have spent some time going over the code and how the PyShell brings all of this together and have formed some loose opinions on how to solve these issues but I wanted to get a much more informed opinion.

I will provide a patch for the first one first, so right now, all my questions are related to that one.

Basically, I have come up with three different places I could patch in StackViewer.py to remove "idlelib.run.runcode" from the stack viewer. 

First option is to patch the constructor of "StackTreeItem" so that if "idlelib.run.runcode" is in the traceback, it never gets pushed to the stack list. I don't like this approach very much because I would essentially be removing data from the stack. 

Second option is to patch the "GetSubList" method so that the iterated variable "info" is checked before constructing a new "FrameTreeItem". If "idlelib.run.runcode" is matched, we would just continue to the next value in the list. I think this would lead to repetitive code though. We would rewrite some of the "GetText" method of "FrameTreeItem" and then make a string comparison to identify the entry to be removed.

Third option is to call the "GetText" method after each created "FrameTreeItem" and make the string comparison then. This would add a lot of method calls if the stack is long. On the other hand, this approach doesn't repeat any code. 

It is highly possible that I have missed something very simple and elegant :) I would very much appreciate your input. I can make upload a patch once I get your approval. Which approach would be better here?

Something else that I would like to ask is the preparation of the patch file. Should switch to the 3.4 branch and then create the patch or can I do it at the dev branch? How would I check if the patch applies to 3.4/3.5/3.6 (apart from applying them manually and testing if it has the desired outcome)?

I'm sorry for the wall of text, I'm nervous and I don't want to mess something up. :)

Thanks a lot for your assistance.

StackViewer.py seem to be identical in 3.4, 3.5, and 3.6, so a patch against any applies to all.  The diff with 2.7 is

F:\Python\dev\36>hg diff -r 4884af6d3e30 f:/python/dev/36/Lib/idlelib/StackViewer.py
@@ -2,7 +2,7 @@
 import sys
 import linecache
 import re
-import Tkinter as tk
+import tkinter as tk

 from idlelib.TreeWidget import TreeNode, TreeItem, ScrolledCanvas
 from idlelib.ObjectBrowser import ObjectTreeItem, make_objecttreeitem
@@ -10,7 +10,7 @@

 def StackBrowser(root, flist=None, tb=None, top=None):
     if top is None:
-        from Tkinter import Toplevel
+        from tkinter import Toplevel
         top = Toplevel(root)
     sc = ScrolledCanvas(top, bg="white", highlightthickness=0)
     sc.frame.pack(expand=1, fill="both")
@@ -109,7 +109,7 @@
         return len(self.object) > 0

     def keys(self):
-        return self.object.keys()
+        return list(self.object.keys())

     def GetSubList(self):
         sublist = []

The second and third differences are due to unneeded code; I will remove them after I submit this message, so update your repository after I do that and then patch.

---
Your research is very helpful.  The purpose of 1. *is* to remove data ;-) -- data that is only present because of Idle.  The purpose of  StackTreeItem.get_stack is to removed unneeded data while converting a linked list to a regular list.  Note that the loop drops 2 of 4 fields. I do not know if
        if tb and tb.tb_frame is None:
            tb = tb.tb_next
actually removes anything, but after this, I believe that the idlelib.run.runcode node (or idlelib.PyShell.runcode node, when Idle is started with -n) is at the top of the linked list.  So I believe adding
        tb = tb.tb_next
after the above will do what we want.  Try it, and if it works, move on.

---
6 (modeled after 3). Modules only have +Globals.  Remove +Globals under each module and instead display globals when expanding. (There will only be a module other than __main__ when there is an error in another module being imported.  We should then improve the lines to read
  + Globals for module __main__ ...
  + Locals for function xyz...

New changeset 9ad330a9d785 by Terry Jan Reedy in branch '2.7':
Issue #24790: Remove extraneous code (which also create 2 & 3 conflicts).
https://hg.python.org/cpython/rev/9ad330a9d785

New changeset 010264c9ceae by Terry Jan Reedy in branch '3.4':
Issue #24790: Remove extraneous code (which also create 2 & 3 conflicts).
https://hg.python.org/cpython/rev/010264c9ceae

7. Sorts global and local names (as with dir()).

8. Don't expand values with their class attributes, at least not for builtins.  Example

+ <dir icon> __doc__  = None

Clicking + gives all the attributes of type(None), each with its own +.  It does not do this for ints and strings.

Terry:  Was removing the public attribute keys OK?

A fair question. The API is a private implementation for Idle (see PEP 434), though I am generally avoiding breaking any public uses thereof.  I actually checked that the method is not defined on the base class.  But on the microchance that someone uses it, I will restore the methods.

Thank you very much for your thorough input, it is very much appreciated!

Actually one of the first things that I tried was to return stack[1:] to remove the target but your suggestion is much better. I did try it and it does work and I will provide a patch as soon as I get home. 

I'll start probing the second one then. :)

Alright, here goes my first patch. I just did what you have pointed out :)

I have also prepared a patch for the second item but I don't know if I should regenerate the patch after you have committed the first patch so I'm sitting on it now. 

I also didn't update the ACKS and NEWS files, should I?

Here's what I did for the second item, just in case you spot something I shouldn't be doing:

         sublist = []
         if frame.f_globals is not frame.f_locals:
             item = VariablesTreeItem("<locals>", frame.f_locals, self.flist)
-            sublist.append(item)
-        item = VariablesTreeItem("<globals>", frame.f_globals, self.flist)
+        else:
+            item = VariablesTreeItem("<globals>", frame.f_globals, self.flist)
         sublist.append(item)
         return sublist

Come to think of it, maybe I should send both the second and third items in one patch? I could just call the GetSubList method if the VariablesTreeItem is being created for locals. Is that a bad approach to take?

Sorry for the sheer number of questions :)

NEWS patches should be omitted from submitted .diff or .patch files because a) NEWS is so volatile that merge conflicts applying the .diff are likely; b) NEWS is different between versions, so that forward merge conflicts are possible (usual for Idle items, so I apply them separately) and b) committers tend to rewrite then anyway.  I will later prepare and separately commit one entry for this issue.

ACKS is more stable within and between versions, so including a acks patch is ok.  But then the patch must be prepared for 3.4 and 2.7 specifically, and I want to know ahead of time if there will be a 3.4 to 3.5 or 3.5 to 3.6 merge conflict.  You can include it or leave it out (easier).

I tested both the .patch and diff in msg248849.  When putting code or diffs in messages, please include function/class/method names or lineno.  With this patch, SubList always return a list of one item.  Note that we can always use frame.f_locals, whether it is or is not f_globals.

I am not exactly sure what you meant by your your last question, but it lead me to the answer.  When a tree item with + is clicked, the corresponding GetSubList method is called. What we want FrameTreeItem.GetSubList to do, instead of returning a list of (now 1) VariablesTreeItem, is to return what VariablesTreeItem.GetSubList would (when clicked subequently).  Replace most of the body of the former (FrameTI.GSL) with the body of the latter (VarTI.GSL), with 'self.object' replaced by 'frame.f_locals'.  Class VariablesTreeItem will then be obsolete.

I just realized that there is a slight but in the patch so far.  When a non-empty program runs, it necessarily executes at least one statement in the main module.  It will only execute anything else when there is a call. Therefore, the stack trace always begins in the main module and that line can be used to display globals, making the globals item under functions redundant.

However, if there is a call to a function in another module, there need not be a module level statement.  By removing the globals items for functions in other modules, the current patch as is will not allow viewing of globals in other modules. A possible solution is to add an expandable module item every time the module changes.

Mark and I would like to combine the StackViewer and Debugger UIs.  Essentially, a revised stackviewer pane would eventually become part of the debugger.  The Debugger avoids the intermediate +locals and +globals items of StackViewer by expanding both when an item is clicked.  However, the expansion is not added to the tree, but to the one locals box and the one (optional) globals box, both separate from and below the tree.

An alternate solution to the globals problem is to expand locals under a function when clicked on, as currently done with stackviewer, and expand globals to a globals box below the tree, as currently done with stackviewer.  I do not see much need to see the globals of different functions simultaneously (one can switch, as one must do not with Debugger).  I see more reason to see multiple locals of at the same time, and it also make sense to put locals right under each function.

Debugger already does item 8.

We should take a look as the stackviewer part of the debugger ui.

First of all, sorry for the late reply.

I did try the solution you have proposed which removes the need to click to expand the items under globals/locals but as I understand it, you have something more in mind for the whole stackviewer UI. Should I still submit a patch for this?

As far as the bug is concerned I will look into that next.

Please post a 'checkpoint' patch with all changes so far, even though it has the 'other globals' problem.

Then look at the UI parts of Debugger.py. It has a Stackviewer class that subclasses ScrolledLIst instead of TreeWidget and a Namespaceviewer class currently used for both locals and globals.

The Namespace class is a frame containing a label (a labeled frame would work as well), a scrollbar, and a canvas containing a frame. The inner frame has a two-column grid: labels with names and entry boxes with values.  Currently, the entry boxes do nothing other than make values look different from names, as editing the entry does nothing. (Ignored entries are a bad UI design.)  Perhaps there was once an intention to propagate changes back to the running program. This would not apply to post-mortem stack viewing.

Starting with the checkpoint patch, I would like to modify Stackviewer to display globals for any item clicked in a separate frame, like Debugger.  This would removing the bug in the checkpoint patch and result in a patch that I might commit.  It would also be a step toward merging (in a new issue).

One possibility is a tree in a new frame.  Another is to import and reuse Debugger.Namespaceviewer.  The issue is whether users should be able to view the attributes of value objects (as in Stackviewer) or not (as in Debugger).  I previously suggested not, because it makes the interface look busy, but I just rediscovered a note based on debugger experience suggesting that it might be a good idea.  Lets start with a second tree.

When waiting for me to respond, you could look at #17942, about improving the command button part of debugger.

New changeset 14ac7deba91f by Terry Jan Reedy in branch '2.7':
Issue #24790: Restore unused function.
https://hg.python.org/cpython/rev/14ac7deba91f

New changeset eec5e81b38d1 by Terry Jan Reedy in branch '3.4':
Issue #24790: Restore unused function.
https://hg.python.org/cpython/rev/eec5e81b38d1

New changeset 514f5d610175 by Terry Jan Reedy in branch '2.7':
Issue #24790: correct typo noticed by Eric Smith
https://hg.python.org/cpython/rev/514f5d610175

New changeset 213ae5626493 by Terry Jan Reedy in branch '3.4':
Issue #24790: correct typo noticed by Eric Smith
https://hg.python.org/cpython/rev/213ae5626493

Deleted .keys in 8.6.

9. As of now, values are not named, whereas I believe they are in debugger.  IE given def f(a,b,c): 1/0; f(1,2,3), viewer prints unlaabelled 1, 2, 3 on separate lines.

Also, the representation of a value may not fit on a line.  I believe a current debugger patch addresses this issue.  Clearly, there should be one function for displaying locals and globals.  And in debugger, one should be able to display them for previous frames.