bpo-37903: IDLE: Shell sidebar with prompts (take #2) #22682
Conversation
Specifically, avoid EditorWindow.ResetColorizer() moving the undo and colorizer delegators to the top of the percolator, by removing and inserting them.
This avoids ColorDelegator removing unrelated tags, specifically "stdin", when re-colorizing.
Prompts are now marked in the text by adding the "console" tag to
the newline character ('\n') before the beginning of the line. This
works around issues with the prompt itself usually no longer being
written in the text widget.
With another small fix to the PyShell.recall() method, input lines
can now be recognized consistently by checking for the "stdin" tag.
This is much better than attempting to separately track the prompt
and input lines, since those are difficult to keep up to date after
undo/redo actions and squeezing/unsqueezing of previous output.
The background color still uses that configured for line numbers.
The fix needed was for History to properly set the "stdin" tag on text it inserts.
|
I verified that the double return bug is gone. I will recheck other stuff tomorrow. And also look at the test failure. If the difference of behavior on Windows does not affect Shell's behavior, then maybe the test is not needed. With the main blocker I know of gone, I am hopeful we can get this in the May 3 beta. It would not have to be perfect because I do not expect to immediately backport for the 3.9.5 and 3.8.10 releases on the same day. (Hence, I removed the automatic backport labels.) At this point, I don't mind not putting this in 3.8 (May 4 is its last maintenance release), or delaying putting it in 3.9 (next is 3.9.6 on June 26) until tested and improved. In particular, I would want tab indents gone so users see an immediate benefit that merits having their visual comfort initially disrupted. If you pull a couple of things into separate issues and PRs, then I will deal with news and merging, once satisfied, and baby-sitting the backports. My thinking now is that even if a bug is discovered during and its quick fix is motivated by another issue, it should be done as a separate issue and PR as long as it is applicable to and an upgrade to IDLE as it is. (In otherwords, if we would want it even without the main issue.) Then merge it into the original PR. I should have said something more earlier. |
|
@terryjreedy, I suggest that you wait and let me take another look at the tests. I should find the time for that in the next few days, and hopefully will find a solution that works. If I don't, we can indeed temporarily disable the offending tests, since the issue is entirely with the testing method being fragile (rather than an actual problem with IDLE.) AFAICT after reviewing everything here and the latest fixes and cleanups, there are no other outstanding issues, and there is no need for separate PRs. |
| @@ -370,5 +391,347 @@ def assert_colors_are_equal(colors): | |||
| assert_colors_are_equal(orig_colors) | |||
|
|
|||
|
|
|||
| def test_coroutine(test_method): | |||
There was a problem hiding this comment.
I have two issues with the name. First, I am conditioned to see test_x as a test function. Second, in usual Python parlance, the generators returned by the test generator functions (called semi-coroutines by Tim Peters) are neither 'coroutines' (defined with 'async def', https://docs.python.org/3/glossary.html) nor are they being used as such (in an async context). I prefer 'wrap_generator' and 'self.run_generator' and believe that this would be clearer to others also.
I think a doc string is also needed explaining why and what. I believe 'why' is that while some previous tests required root, to create and update widgets, these are the first tests requiring mainloop in order for widgets to properly respond to events not generated by test code but by the tested code.
I presume we can use the same machinery for shell tests that do not involve the side bar. I appreciate you 'pioneering' this.
There was a problem hiding this comment.
I've renamed this to run_in_tk_mainloop() and extracted it to a separate tkinter_testing_utils module.
There was a problem hiding this comment.
I've also changed the wording to reference "generator functions" rather than "coroutines", and added more info in a doc-string and comments.
| try: | ||
| orig_use_subprocess = idlelib.pyshell.use_subprocess | ||
| except AttributeError: | ||
| orig_use_subprocess = None | ||
| idlelib.pyshell.use_subprocess = False | ||
| def cleanup_use_subprocess(): | ||
| if orig_use_subprocess is not None: | ||
| idlelib.pyshell.use_subprocess = orig_use_subprocess | ||
| else: | ||
| del idlelib.pyshell.use_subprocess | ||
| cls.addClassCleanup(cleanup_use_subprocess) | ||
|
|
There was a problem hiding this comment.
| try: | |
| orig_use_subprocess = idlelib.pyshell.use_subprocess | |
| except AttributeError: | |
| orig_use_subprocess = None | |
| idlelib.pyshell.use_subprocess = False | |
| def cleanup_use_subprocess(): | |
| if orig_use_subprocess is not None: | |
| idlelib.pyshell.use_subprocess = orig_use_subprocess | |
| else: | |
| del idlelib.pyshell.use_subprocess | |
| cls.addClassCleanup(cleanup_use_subprocess) |
Instead, add the following to pyshell, maybe line 62.
use_subprocess = False # Default for testing; defaults to True in main() for running.
| text.event_generate('<<newline-and-indent>>') | ||
| text.insert('insert', line, 'stdin') | ||
|
|
||
| def run_test_coroutine(self, coroutine): |
There was a problem hiding this comment.
See comment above about 'coroutine'.
| self.assert_sidebar_lines_end_with(['>>>', '>>>']) | ||
|
|
||
| @test_coroutine | ||
| def test_single_line_command(self): |
There was a problem hiding this comment.
Python REPLs execute statements, not OS command lines ;-).
| def test_single_line_command(self): | |
| def test_single_line_statement(self): |
| self.assert_sidebar_lines_end_with(['>>>', None, '>>>']) | ||
|
|
||
| @test_coroutine | ||
| def test_multi_line_command(self): |
There was a problem hiding this comment.
| def test_multi_line_command(self): | |
| def test_multi_line_statement(self): |
|
|
||
| @test_coroutine | ||
| def test_multi_line_command(self): | ||
| # Block statements are not indented because IDLE auto-indents. |
There was a problem hiding this comment.
| # Block statements are not indented because IDLE auto-indents. | |
| # Suite is not indented because IDLE auto-indents. |
There was a problem hiding this comment.
What's a "suite" in this context? I'm not familiar with this use of the word.
There was a problem hiding this comment.
The indented statements after the header of a compound statement. A Python grammar term used throughout https://docs.python.org/3/reference/compound_stmts.html#compound-statements.
| self.assert_sidebar_lines_synced() | ||
|
|
||
| @test_coroutine | ||
| def test_squeeze_single_line_command(self): |
There was a problem hiding this comment.
| def test_squeeze_single_line_command(self): | |
| def test_squeeze_single_line_statement(self): | |
| # Test that statement is not squeezed. |
Since not squeezed, there is not button. So why test button call below?
There was a problem hiding this comment.
Yes, this isn't testing much. I've improved this in the latest commit.
|
CONFIGDIALOG is disabled because it is still calling update_colors. That line must not be covered in its test. (probably should use some mock_funcs). |
|
@terryjreedy |
|
I am about to get more sleep before continuing. Ignoring the issue of making this work with #25678, discussed on the tracker, do you consider this PR ready to commit for a beta release? |
Yes! I'd like to follow up quickly with implementing mouse interaction with the sidebar (similar to the line-numbers sidebar), including specifically a context-menu (i.e. right-click) option to copy the contents with prompts, which would be useful for doctests for example. |
|
I checked this out and played with it, but while the prompt in the sidebar looks nice, indentation is still with tabs, which gives it a decidedly 1990s feel. :-) |
|
@gvanrossum, thanks for giving this a whirl! Changing indentation from tabs to spaces is a quick followup that @terryjreedy has begun working on (see PR GH-25678). |
|
@terryjreedy, I've got a local branch which refactors |
|
#25678 works to change indents when applied on top of this. I have done a thorough manual testing of the combination. While I want to review these changes more, I am merging this now so we can move on. I will then merge master in #25678 and merge that. Given Tal preference, Guido's comments, and the fact that the changes here are, from a bug-finding viewpoint, more in need of user testing than those in #25678, the sidebar should be in place first. I will look at the new sidebar code after Tal posts a new PR. While trying to make the sidebar optional after adding a 'usesidebar' attribute, I will look at the pyshell changes. Tal, if I have trouble, I will ask for help. |
|
(Take 2 of this PR, following up PR GH-15474, just to have a cleaner history and conversation, as per @terryjreedy's request.)
This is a working implementation of a sidebar for IDLE's shell, removing the prompts from the main text widget (except for special prompts such as the one used by the debugger.) It works very well in my testing.
Currently known to be missing:
Notes on the implementation:
recall()method.https://bugs.python.org/issue37903