bpo-17942: major rework of debugger UI #22947
Conversation
|
Is it possible to do without using preset image sizes and font? (if for some reason the default font is larger than normal, the difference is very obvious). Even if we cannot change the image and font sizes dynamically, I think we should avoid fixing the family. |
- updated to use ttk widgets throughout - simplified controls, changed go/step/etc. to image buttons - overall layout is a two-pane horizontal window (controls, stack, status on left, variables on right) - treeview widget used to replace custom scrolled-listbox stackviewer and custom canvas-based namespaceviewer - integrated locals/globals into a single treeview on right; can collapse/expand each independently instead of having checkboxes to control whether shown or not
was being used by stack trace in debugger
- button captions now use TkIconFont (a bit bigger than before) - status line now uses default font, plus derives italic variation for errors
|
Oops, sorry about the committed merge (can you tell I'm not using git every day)? In any case, I've changed the hard-coded fonts as per Elisha's comment. |
Massive visual improvement over the original! We have a few methods which don't do anything (add_varheader, mouse_moved_vars and leave_vars): I assume there's a plan for them but for now should we add a TODO to them?
I only reviewed the comments, not the whole patch.
I verified that scrolled list is only used here and can go if not used here.
What is the replacement?
Lib/idlelib/debugger.py
Outdated
| func = match.group(1) + '.' + func | ||
| else: | ||
| func = selfval.__class__.__name__ + '.' + func | ||
| except Exception: |
There was a problem hiding this comment.
I strongly prefer that we not add more bare exceptions. Certainly not without a justification comment.
Lib/idlelib/debugger.py
Outdated
| # we've probably got the string representation of the | ||
| # object sent from the remote debugger, see if we can | ||
| # parse it into something useful | ||
| match = re.match('^<(?:.*)\.([^\.]*) object at 0x[0-9a-f]+>$', |
There was a problem hiding this comment.
Special chars are normal inside [] without \, so I think [^\.] and [^.] have the same effect (I prefer the latter) so that [^\\.] is needed to match 3 all 3 chars. Prefix r suppress strings escapes, but there are none, so I think it has no effect, though some prefer to use it for all REs. Outside the brackets, '\.' matches literal period, which is what want, whereas '\\.' matches multiple backslashes? I am not sure why later periods are excluded. CPython now prints addresses with A-F.
In any case, non-trivial REs should be defined at the top of the file so that they can be tested.
Lib/idlelib/debugger.py
Outdated
| # we've probably got the string representation of the | ||
| # object sent from the remote debugger, see if we can | ||
| # parse it into something useful | ||
| match = re.match('^<(?:.*)\.([^\.]*) object at 0x[0-9a-f]+>$', |
There was a problem hiding this comment.
I suspect this is correct. See previous comment about testing REs
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
All REs with '' should be prefixed with 'r' because uses like . and \d not recognized by standard string processing are deprecated and will become errors in the future. |
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: E-Paine <63801254+E-Paine@users.noreply.github.com>
|
cleaned up the code and documentation in add_stackframee... not a shock but it was actually not doing anything as is because the 're' module hadn't been imported, which the bare exception oh-so-helpfully hid.. |
|
Just a general thing, is there a way to make the panedwindow more obvious? (when I first looked at the new design, I mistook it for padding until I looked at the code) |
Follow PEP-8 in comments and my comment on clarifying debugger separation.
The IDLE Doc says of debugger "This feature is still incomplete and somewhat experimental. " Would you consider this still true after this patch?
|
Ubuntu failure related to issue 42142 (I have noted this there) |
|
@E-Paine Thank you. GH will not let me download the failure log. |
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
|
|
related to issue #14111... i note that the 'stop' button in the in-progress debugger ui is actually doing 'quit' (and doesn't in fact work when in the middle of a busy loop). i think it would make sense to add a 'pause' button beside go, which can be used to pause the running program, allowing it to be continued. i'm wondering if the stop button should be renamed or icon changed to make it more clear that it will terminate the running program altogether. should probably add tool tips on the debugger controls as well |
|
This PR is stale because it has been open for 30 days with no activity. |
https://bugs.python.org/issue17942