Ctrl-D eats a character on IDLE

[serhiy-storchaka]

BPO	29910
Nosy	@terryjreedy, @serhiy-storchaka, @mlouielu
PRs	bpo-29910: IDLE no longer delete character after commenting out a region #825 [3.6] bpo-29910: IDLE no longer deletes a character after commenting … #2429 bpo-29910: IDLE - revert breaks that disabled calltip close. #2997 [3.6] bpo-29910: IDLE - revert breaks that disabled calltip close. … #3017

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = None
created_at = <Date 2017-03-26.17:25:01.898>
labels = ['expert-IDLE', 'type-bug', '3.7']
title = 'Ctrl-D eats a character on IDLE'
updated_at = <Date 2017-08-08.04:46:37.919>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2017-08-08.04:46:37.919>
actor = 'louielu'
assignee = 'terry.reedy'
closed = False
closed_date = None
closer = None
components = ['IDLE']
creation = <Date 2017-03-26.17:25:01.898>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 29910
keywords = []
message_count = 12.0
messages = ['290539', '290549', '290568', '296663', '296995', '297009', '297010', '299730', '299855', '299859', '299894', '299895']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'serhiy.storchaka', 'louielu']
pr_nums = ['825', '2429', '2997', '3017']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29910'
versions = ['Python 3.6', 'Python 3.7']

[serhiy-storchaka]

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.

[terryjreedy]

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.

[serhiy-storchaka]

(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.

[terryjreedy]

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.

[terryjreedy]

New changeset 213ce12 by terryjreedy (Serhiy Storchaka) in branch 'master':
bpo-29910: IDLE no longer deletes a character after commenting out a region (#825)
213ce12

[terryjreedy]

New changeset 8bdc3bd by terryjreedy in branch '3.6':
[3.6] bpo-29910: IDLE no longer deletes a character after commenting out a region (GH-825) (bpo-2429)
8bdc3bd

[terryjreedy]

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.

[terryjreedy]

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.

[terryjreedy]

New changeset 8922587 by Terry Jan Reedy in branch 'master':
bpo-29910: IDLE - revert breaks that disabled calltip close. (bpo-2997)
8922587

[terryjreedy]

New changeset b61de2d by Terry Jan Reedy in branch '3.6':
[3.6] bpo-29910: IDLE - revert breaks that disabled calltip close. (GH-2997) (bpo-3017)
b61de2d

[mlouielu]

After bisecting, commit 213ce12 cause calltip can't close when type to ) at the end.

for example:

    print('blablalbalba')
         ^ show         ^ didn't
           tip            close tip

[mlouielu]

da..., Terry reverts this soon.

[erlend-aasland]

ISTM, the OP issue was fixed. A small part of it was reverted, but AFAICS, the main issue is resolved. I would suggest closing this.

If there are further problems, we should open new, narrow issues for those.