classification
Title: Ctrl-D eats a character on IDLE
Type: behavior Stage: resolved
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: open Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: louielu, serhiy.storchaka, terry.reedy
Priority: normal Keywords:

Created on 2017-03-26 17:25 by serhiy.storchaka, last changed 2017-08-08 04:46 by louielu.

Pull Requests
URL Status Linked Edit
PR 825 merged serhiy.storchaka, 2017-03-26 17:31
PR 2429 merged terry.reedy, 2017-06-27 05:21
PR 2997 merged terry.reedy, 2017-08-04 04:55
PR 3017 merged terry.reedy, 2017-08-07 17:50
Messages (12)
msg290539 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-26 17:25
Ctrl-D is binded to commenting out a block in IDLE. But additionally it deletes the first character of the line after the block. The default binding of Ctrl-D in Text widget is deleting a character under cursor. IDLE first comment out selected block, and after that runs the default handler.

Proposed patch fixes this issue and presumably other potential conflicts with default bindings. It just adds `return "break"` at the end of most event handlers.
msg290549 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-03-26 22:17
Checking, I see that the default ^D binding to DEL is documented in IDLE Help, section 2, Editing and Navigation, along with other Text defaults.  (I need to determine if the list is complete.  Is it the same on all systems?)  

I question whether IDLE should allow these to be changed, or whether at least some should be fixed.  The information should certainly be made available somehow in the key customization UI. The UI should inform users when a proposed customization creates a conflict with existing bindings.  (This last is more or less covered by other issues.)

In Shell, ^D is treated as EOF and closes the window (but not IDLE if another window is open), even (recently) on Windows.

> Ctrl-D is binded to commenting out a block in IDLE.

This is only true in the IDLE Modern UNIX binding that you wrote for 3.6 (and possibly in user customizations).  All other builtin bindings use Alt-3 for 'comment-region'.  If I had noticed the conflict before pushing, I would have brought it up.  Too late.

As for the patch.  Adding "return 'break'" in general looks good, but I have not checked each case yet.  (Not having side by side diffs as with Rietveld makes this much harder.)  In some places, you add "return None" instead.  I am not sure why the difference.  If there is a masked binding, having 'key-x' do one thing sometimes and something else other times would seem disconcerting. 

Removing "assertIsNone(<method call>)" in test_parenmatch.py appears valid since the assert did not really test much for calls that always returned None (and now always 'break').  Improving that test file would be a new issue.

Testing the change in wrapper functions of the form
    def xyz_event(event):
        xyz()
        return 'break'
will be easy: check that a mock xyz is called and that the wrapper returns 'break'.  Testing xyz itself can be deferred.  Testing event handlers that do the work internally, as in (un)comment_region_event, will require some fixture setup.  As before, I expect to work on adding tests.

Since idelib essentially identical in 3.6 and 3.7, I think your original cherry-pick 3.6 tag was correct in that the patch should work as is.  It would have to be changed at least as far as file names is concerned for other versions, but since the Modern Unix keyset and associated changes were new in 3.6 and not backported, I am not inclined to backport this either.
msg290568 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-27 08:21
> (Not having side by side diffs as with Rietveld makes this much harder.)

Click on the "Files changed" tab on the PR page: https://github.com/python/cpython/pull/825/files . You can add inline comments when click on the "+" button that follows your mouse cursor. After entering the comment press the GREEN "Start a review" button rather than "Add single comment". This will allow to send all your comments at once rather than sending them as separate emails, and will allow to edit or remove comments before sending them. After reviewing all changes press the GREEN button at the top right corner of the page for sending your comments.

Yes, it is my fault that I missed the conflict. But the user can add conflicting shortcuts for other events, so it would be better to make them safe even if there re not conflicts in standard configuration.

> In some places, you add "return None" instead.

PEP 8: "Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None , and an explicit return statement should be present at the end of the function (if reachable)."

> If there is a masked binding, having 'key-x' do one thing sometimes and something else other times would seem disconcerting. 

I think this is okay. In the specific context one this is done, but if this context does not exist fall back to doing other thing.

I don't think special tests are needed. There are too much event handlers, and testing them with monkey-patching will just complicate the code and will not check anything beside the fact that that event handlers are written in special style.
msg296663 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-23 03:17
I have since discovered the [unified][split] buttons in the diff windows.
I have also starting applying the return run elsewhere in idlelib.

I had to think about paren_match_event.  It is bound to KeyRelease-parenright, etc, via config-extentions.def and should never be changed by the user.  Nor should those binding be duplicated.  But they could be, at least now.  So always returning 'break' is probably safest.
msg296995 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-27 04:02
New changeset 213ce12adfc9281c6f381bb45d132e9de8ffd450 by terryjreedy (Serhiy Storchaka) in branch 'master':
bpo-29910: IDLE no longer deletes a character after commenting out a region (#825)
https://github.com/python/cpython/commit/213ce12adfc9281c6f381bb45d132e9de8ffd450
msg297009 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-27 05:53
New changeset 8bdc3bd3d66fefdc07d32bd19c41c6f902f16111 by terryjreedy in branch '3.6':
[3.6] bpo-29910: IDLE no longer deletes a character after commenting out a region (GH-825) (#2429)
https://github.com/python/cpython/commit/8bdc3bd3d66fefdc07d32bd19c41c6f902f16111
msg297010 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-27 05:57
Thanks for the fix.  I am now comfortable enough with the new workflow to begin making a dent in the 100+ patch backlog waiting for review.
msg299730 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-08-04 05:06
Adding 'break' to parenmatch.paren_closed_event prevented ')' from closing calltips.  Removing it reverts the regression.

I am leaving this open to re-review the other optional changes to see if ')' is unique or if anything else is being caught that needs to pass   I am thinking that 'break' should perhaps be limited to events that are associated with user-settable control sequences (those with modifiers other than Shift).

I am still looking at calltip_w.py to see whether, if the bindings were reversed, ')' could close a calltip but not pass on to parenmatch.
msg299855 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-08-07 17:37
New changeset 89225871d314fa675ea9ac292e7bc75320f1aef5 by Terry Jan Reedy in branch 'master':
bpo-29910: IDLE - revert `break`s that disabled calltip close. (#2997)
https://github.com/python/cpython/commit/89225871d314fa675ea9ac292e7bc75320f1aef5
msg299859 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-08-07 18:23
New changeset b61de2d46596ac00dcf2d33bbce2023d0bf4207b by Terry Jan Reedy in branch '3.6':
[3.6] bpo-29910: IDLE - revert `break`s that disabled calltip close. (GH-2997) (#3017)
https://github.com/python/cpython/commit/b61de2d46596ac00dcf2d33bbce2023d0bf4207b
msg299894 - (view) Author: Louie Lu (louielu) * Date: 2017-08-08 04:45
After bisecting, commit 213ce12adfc9281c6f381bb45d132e9de8ffd450 cause calltip can't close when type to `)` at the end.

for example:

    print('blablalbalba')
         ^ show         ^ didn't
           tip            close tip
msg299895 - (view) Author: Louie Lu (louielu) * Date: 2017-08-08 04:46
da..., Terry reverts this soon.
History
Date User Action Args
2017-08-08 04:46:37louielusetmessages: + msg299895
2017-08-08 04:45:20louielusetnosy: + louielu
messages: + msg299894
2017-08-07 18:23:09terry.reedysetmessages: + msg299859
2017-08-07 17:50:53terry.reedysetpull_requests: + pull_request3050
2017-08-07 17:37:12terry.reedysetmessages: + msg299855
2017-08-04 05:06:58terry.reedysetstatus: closed -> open

messages: + msg299730
2017-08-04 04:55:12terry.reedysetpull_requests: + pull_request3034
2017-06-27 17:05:03terry.reedysetpull_requests: - pull_request2504
2017-06-27 16:42:18wohlgangersetpull_requests: + pull_request2504
2017-06-27 05:57:36terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg297010

stage: test needed -> resolved
2017-06-27 05:53:42terry.reedysetmessages: + msg297009
2017-06-27 05:21:42terry.reedysetpull_requests: + pull_request2480
2017-06-27 04:02:34terry.reedysetmessages: + msg296995
2017-06-23 03:17:45terry.reedysetmessages: + msg296663
versions: - Python 2.7, Python 3.5
2017-03-27 08:21:04serhiy.storchakasetmessages: + msg290568
2017-03-26 22:18:00terry.reedysetmessages: + msg290549
stage: patch review -> test needed
2017-03-26 17:31:10serhiy.storchakasetpull_requests: + pull_request731
2017-03-26 17:25:01serhiy.storchakacreate