-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-84632: IDLE: fix clipboard being cleared upon exit #26163
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
base: main
Are you sure you want to change the base?
gh-84632: IDLE: fix clipboard being cleared upon exit #26163
Conversation
Just a couple of quick notes:
|
I'm suggesting this as an immediate fix for the reported issue as it relates to IDLE. Making this more generally available would require figuring out how it best fits into tkinter's set of abstractions, documenting it, testing it thoroughly, etc. I'm all for it, but IMO we should open a separate issue for that and not have it further delay this fix for IDLE. |
I'm looking into the test failure. Surprisingly, it is consistent and I can reproduce it locally on Windows 10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name
- blame
Docstring looks good. I presume C code okay since patch works for me, but better if Serhiy checks.
When you're done making the requested changes, leave the comment: |
It appears that merging in the main branch fixed the failing tests. 🤷 |
I have made the requested changes; please review again. |
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
I rebuilt branch and |
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Lib/idlelib/pyshell.py
Outdated
@@ -1698,6 +1698,8 @@ def main(): | |||
root.mainloop() | |||
root.destroy() | |||
capture_warnings(False) | |||
import _tkinter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if add del root
and two gc.collect()
? Would it crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would adding those cause a crash? Sorry, I don't understand the reasoning here, would you care to expand on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't cause a crash, but the check Serhiy suggested is to ensure that the interpreter isn't required for finalisation (i.e. if the root gets garbage collected, things are not going to go horribly wrong - at least that's how I understood it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT nothing else in our codebase calls Tcl_Finalize
, Tcl_FinalizeThread
, or anything else that triggers Tcl's finalization.
In addition, calling these multiple times shouldn't be an issue according to the Tcl docs:
Tcl_Finalize
can be safely called more than once.
Note: This does not work on more up-to-date Linux (e.g. Arch, Debian testing). It's a Tk issue (idk why). |
Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
Could you elaborate please? What exactly doesn't work, and in what way? |
On the two distros named (I haven't tested on e.g. Fedora), Tk clipboard content is cleared on exit regardless of the new call. I proved it was a Tk issue because the following also results in an empty clipboard:
|
Oh, then that's different from what we've been trying to fix up until now, since on all platforms tested so far the above worked as expected; it only didn't work via tkinter. That should be reported as a Tcl/Tk bug, and shouldn't affect this issue and PR. |
@serhiy-storchaka, with the latest change based on your review comment, do you think this is good to go? |
This PR is stale because it has been open for 30 days with no activity. |
@serhiy-storchaka are we good to merge this? |
I just had Python segfault while running IDLE with this change. Let's hold off on merging for now. |
This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today. |
The way the clipboard works on X11, is that it only stores a reference to the data when copying, and only retrieves the actual data when a paste is requested. Thus, the source application must provide the data at paste time, otherwise the clipboard will be empty. The clipboard content will only persist on X11 if there's a clipboard manager running, that constantly "harvests" clipboard selections. |
I smoke-tested ( |
This uses
Tcl_FinalizeThread()
rather thanTcl_Finalize()
, because the latter caused memory segmentation errors on Ubuntu 20.04.Tested locally on Windows 10 and Ubuntu 20.04.