Skip to content

/cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-38943: Fix IDLE autocomplete window not always appearing #17416

Merged
merged 11 commits into from Dec 10, 2019
Merged

bpo-38943: Fix IDLE autocomplete window not always appearing #17416

merged 11 commits into from Dec 10, 2019
+6 −0

Conversation

@JohnnyNajera
Copy link
Contributor

JohnnyNajera commented Nov 29, 2019 •
edited

https://bugs.python.org/issue38943

In some systems the function winconfig_event is called in a context in which wm_geometry has no effect.
@JohnnyNajera
Fixing autocomplete window doesn't appear in ubuntu 19.10
Loading status checks…
274570d
@JohnnyNajera JohnnyNajera requested a review from terryjreedy as a code owner Nov 29, 2019
@the-knights-who-say-ni

This comment has been minimized.

Sign in to view
Copy link

the-knights-who-say-ni commented Nov 29, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@JohnnyNajera

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!
@JohnnyNajera
JohnnyNajera reviewed Nov 29, 2019
View changes
Copy link
Contributor Author

JohnnyNajera left a comment •
edited

In some systems the function winconfig_event is called in a context in which wm_geometry has no effect. Calling it through widget.after(..) solves the problem on the systems I've checked (Ubuntu 19.04 to Ubuntu 19.10), without affecting its behavior on older systems (Tested on: Ubuntu 16.04, Ubuntu 18.04).

Furthermore, the escape key never make it to the function keypress_event, without specifically binding it.
@blurb-it
📜🤖 Added by blurb_it.
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c470974
@terryjreedy
terryjreedy requested changes Nov 30, 2019
View changes
Copy link
Member

terryjreedy left a comment

Without knowing what the problem to be fixed it, I cannot judge whether the 1 millisecond delay should really fix it. I worry that calling geometry after is_configuring is set false could under some circumstances trigger another call.

My deeper problem with the patch is that it is yet another kludge on what I consider a flawed design. We want the positioning function to be done once. Yet it is bound to an event that happens multiple times. The first Configure event is from 'wm_geometry("+10000+10000")' in show_window. This is not needed (and would not work on a future 16K monitor) and is done for no other popup (window.withdraw also makes a window invisible). That event is later bound to winconfig_event and we depend on that post-event binding to trigger the function. Instead, we should just call the positioning function when needed, as we do with calltips and all dialogs.

Within the function, 'acw.update' triggers more Configure events (4 on Windows). So is_configuring is needed to avoid infinite recursion. On Windows, there are other Configure events, so the function is unbound from Configure events in the first call. Without all this nonsense, we don't seem to have no-show problems on other systems.

If we touch this stuff, we have to manually test on all three systems, so I want to refactor to make it better. Perhaps we should make this a tooltip subclass, as with calltips.
Lib/idlelib/autocomplete_w.py Outdated Show resolved Hide resolved
Lib/idlelib/autocomplete_w.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Nov 30, 2019

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
@terryjreedy terryjreedy requested a review from taleinat Nov 30, 2019
JohnnyNajera and others added 2 commits Nov 30, 2019
@JohnnyNajera @terryjreedy
Update Lib/idlelib/autocomplete_w.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
66c9c1b 
Co-Authored-By: Terry Jan Reedy <tjreedy@udel.edu>
@JohnnyNajera @terryjreedy
Update Lib/idlelib/autocomplete_w.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bdd6ddd 
Co-Authored-By: Terry Jan Reedy <tjreedy@udel.edu>
@JohnnyNajera

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

JohnnyNajera commented Nov 30, 2019

Hi,
I agree the entire design could be rewritten.

Regarding the 1-milisecond - It's not about 1-milisecond, but about adding the function to the mainloop queue, so it is called in a context in which it works.
I agree this is indeed a quick solution to a an entire class that can be rewritten, but in the meanwhile I find myself manually patching this on every Ubuntu 19.10 I'm working on.
Maybe we can merge this, while I will gladly work on a redesign?

I have made the requested changes; please review again.

P.S.
Regarding the update function, how about calling update_idletasks instead?
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Nov 30, 2019

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.
@bedevere-bot bedevere-bot requested a review from terryjreedy Nov 30, 2019
@JohnnyNajera
Remove excessive whitespace
Loading status checks…
eeba78f
@JohnnyNajera
Delete unused wrapper function
Loading status checks…
4c025b0
@terryjreedy

This comment has been minimized.

Sign in to view
Copy link
Member

terryjreedy commented Nov 30, 2019

I appears that you have not yet signed the CLA. Please do so.
@taleinat

This comment has been minimized.

Sign in to view
Copy link
Contributor

taleinat commented Dec 1, 2019

Maybe we can merge this, while I will gladly work on a redesign?

If we can't easily find the root cause for this issue, I tend to agree.

I'll try testing on my Ubuntu 18.10 machine to see if this reproduces. (Ideally we'd find a better fix for this which doesn't require a rewrite.)
@taleinat
taleinat reviewed Dec 1, 2019
View changes
Lib/idlelib/autocomplete_w.py Outdated
@@ -256,7 +256,7 @@ def winconfig_event(self, event):
else:
# place acw above current line
new_y -= acw_height
acw.wm_geometry("+%d+%d" % (new_x, new_y))
self.widget.after(1, acw.wm_geometry, "+%d+%d" % (x, y))

This comment has been minimized.

Sign in to view
Copy link
@taleinat

taleinat Dec 1, 2019

Contributor

This should use new_x and new_y, rather than x and y.

This comment has been minimized.

Sign in to view
Copy link
@JohnnyNajera

JohnnyNajera Dec 3, 2019

Author Contributor

Of course, thank you. This happened in the last change suggestion, I didn't notice.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 1, 2019

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
@taleinat

This comment has been minimized.

Sign in to view
Copy link
Contributor

taleinat commented Dec 1, 2019 •
edited

Regarding the update function, how about calling update_idletasks instead?

Assuming you are referring to the code in the same function: It requires that the auto-complete window to be fully positioned in order to query its geometry.

According to this SO answer, update_idletasks should indeed be preferred, especially inside event handlers as in this case. We could try changing this, but as you've seen this is a fragile piece of code, so that would need to be tested extensively on many different OSes.
@JohnnyNajera
Fix variable names
Loading status checks…
34fc56c
@JohnnyNajera JohnnyNajera changed the title bpo-38943: Fixing autocomplete window doesn't appear in ubuntu 19.10 bpo-38943: Fix autocomplete window doesn't appear in ubuntu 19.10 Dec 3, 2019
@taleinat

This comment has been minimized.

Sign in to view
Copy link
Contributor

taleinat commented Dec 4, 2019

A different solution can be just calling update_idletasks right after wm_geometry which works just as well, which might be slightly preferred aesthetically.

That seems like a more appropriate solution to me. (And the difference isn't just aesthetic.)
@JohnnyNajera
Call update_idletasks instead
Loading status checks…
bb78bdf
@JohnnyNajera

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

JohnnyNajera commented Dec 4, 2019 •
edited

(And the difference isn't just aesthetic.)

It is in the sense we don't quite understand both of them. But fair enough, I have committed it.

I have made the requested changes; please review again.

P.S. Can you review my other PR? #17419
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 4, 2019

Thanks for making the requested changes!

@terryjreedy, @taleinat: please review the changes made to this pull request.
@JohnnyNajera

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

JohnnyNajera commented Dec 9, 2019

What do you say? should we merge this? @taleinat
@terryjreedy
terryjreedy approved these changes Dec 9, 2019
View changes
@terryjreedy terryjreedy dismissed taleinat’s stale review Dec 9, 2019

Requested changes were done and made obsolete by further change, which Tal also approved.

@terryjreedy
Merge remote-tracking branch 'origin/master' into pr_17416
e7408a1
@terryjreedy

This comment has been minimized.

Sign in to view
Copy link
Member

terryjreedy commented Dec 9, 2019

The tooltip event handler, used for calltips, uses .update_idletasks and seems to be working fine. I check all 3 current completions both auto and manual on windows and all continue to work. So I am comfortable with this -- and will check on Mac Mohave when the .rcs are released tomorrow or so.
@terryjreedy terryjreedy changed the title bpo-38943: Fix autocomplete window doesn't appear in ubuntu 19.10 bpo-38943: Fix IDLE autocomplete window not always appearing Dec 9, 2019
terryjreedy added 2 commits Dec 9, 2019
@terryjreedy
Merge remote-tracking branch 'origin/master' into pr_17416
1a35978
@terryjreedy
News item.
Loading status checks…
cbed971
@terryjreedy terryjreedy merged commit bbc4162 into python:master Dec 10, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191210.1 succeeded
Details
bedevere/issue-number Issue number 38943 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 10, 2019

Thanks @JohnnyNajera for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 10, 2019
@JohnnyNajera @miss-islington
bpo-38943: Fix IDLE autocomplete window not always appearing (pythonG…
ec643ad 
…H-17416)

This has happened on some versions of Ubuntu.
(cherry picked from commit bbc4162)

Co-authored-by: JohnnyNajera <58344607+JohnnyNajera@users.noreply.github.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 10, 2019

GH-17548 is a backport of this pull request to the 3.8 branch.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 10, 2019

GH-17549 is a backport of this pull request to the 3.7 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 10, 2019
@JohnnyNajera @miss-islington
bpo-38943: Fix IDLE autocomplete window not always appearing (pythonG…
e657362 
…H-17416)

This has happened on some versions of Ubuntu.
(cherry picked from commit bbc4162)

Co-authored-by: JohnnyNajera <58344607+JohnnyNajera@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 10, 2019
@miss-islington @JohnnyNajera
bpo-38943: Fix IDLE autocomplete window not always appearing (GH-17416)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1b0e88d 
This has happened on some versions of Ubuntu.
(cherry picked from commit bbc4162)

Co-authored-by: JohnnyNajera <58344607+JohnnyNajera@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 10, 2019
@miss-islington @JohnnyNajera
bpo-38943: Fix IDLE autocomplete window not always appearing (GH-17416)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
34d5d5e 
This has happened on some versions of Ubuntu.
(cherry picked from commit bbc4162)

Co-authored-by: JohnnyNajera <58344607+JohnnyNajera@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryjreedy terryjreedy

@taleinat taleinat

Assignees
No one assigned
Labels
CLA signed type-bugfix
Projects
None yet
Milestone
No milestone
6 participants
@JohnnyNajera @the-knights-who-say-ni @bedevere-bot @terryjreedy @taleinat @miss-islington
You can’t perform that action at this time.