The Wayback Machine - https://web.archive.org/web/20221223193456/https://github.com/python/cpython/pull/26163
Skip to content
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

gh-84632: IDLE: fix clipboard being cleared upon exit #26163

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented May 16, 2021

This uses Tcl_FinalizeThread() rather than Tcl_Finalize(), because the latter caused memory segmentation errors on Ubuntu 20.04.

Tested locally on Windows 10 and Ubuntu 20.04.

@E-Paine
Copy link
Contributor

E-Paine commented May 16, 2021

Just a couple of quick notes:

  1. Is it worth adding this to the regular tkinter module? I suspect it would be useful for applications other than IDLE (I would personally make great use of such a method). We would have to ensure it is properly documented so people know when / not to use it.

  2. I suspect we would need to add a flag to ensure the user doesn't / cannot try to create a new Tk instance after calling the Tcl_Finalize wrapper (i.e. an exception would be raised by _tkinter.create)

@taleinat
Copy link
Contributor Author

taleinat commented May 16, 2021

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.

@taleinat
Copy link
Contributor Author

taleinat commented May 16, 2021

I'm looking into the test failure. Surprisingly, it is consistent and I can reproduce it locally on Windows 10.

Copy link
Member

@terryjreedy terryjreedy left a comment

  1. name
  2. blame

Docstring looks good. I presume C code okay since patch works for me, but better if Serhiy checks.

Lib/idlelib/pyshell.py Outdated Show resolved Hide resolved
Modules/_tkinter.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented May 16, 2021

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@taleinat
Copy link
Contributor Author

taleinat commented May 16, 2021

I'm looking into the test failure. Surprisingly, it is consistent and I can reproduce it locally on Windows 10.

It appears that merging in the main branch fixed the failing tests. 🤷

@taleinat
Copy link
Contributor Author

taleinat commented May 16, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented May 16, 2021

Thanks for making the requested changes!

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

@terryjreedy
Copy link
Member

terryjreedy commented May 16, 2021

I rebuilt branch and help(_tkinter._finalize_tcl) properly displays the new docstring. Once the blurb is fixed, I'm okay with you merging when you are.

Co-authored-by: Terry Jan Reedy <[email protected]>
@@ -1698,6 +1698,8 @@ def main():
root.mainloop()
root.destroy()
capture_warnings(False)
import _tkinter
Copy link
Member

@serhiy-storchaka serhiy-storchaka May 17, 2021

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?

Copy link
Contributor Author

@taleinat taleinat May 19, 2021

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?

Copy link
Contributor

@E-Paine E-Paine May 19, 2021

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

Copy link
Contributor Author

@taleinat taleinat May 19, 2021

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.

Lib/idlelib/pyshell.py Outdated Show resolved Hide resolved
@E-Paine
Copy link
Contributor

E-Paine commented May 17, 2021

Note: This does not work on more up-to-date Linux (e.g. Arch, Debian testing). It's a Tk issue (idk why).

@taleinat
Copy link
Contributor Author

taleinat commented May 19, 2021

Note: This does not work on more up-to-date Linux (e.g. Arch, Debian testing). It's a Tk issue (idk why).

Could you elaborate please? What exactly doesn't work, and in what way?

@E-Paine
Copy link
Contributor

E-Paine commented May 19, 2021

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:

>>> wish
% clipboard clear
% clipboard append {testing 123}
% exit

@taleinat
Copy link
Contributor Author

taleinat commented May 19, 2021

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:

>>> wish
% clipboard clear
% clipboard append {testing 123}
% exit

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.

@taleinat
Copy link
Contributor Author

taleinat commented May 19, 2021

@serhiy-storchaka, with the latest change based on your review comment, do you think this is good to go?

@github-actions
Copy link

github-actions bot commented Jun 19, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 19, 2021
@E-Paine
Copy link
Contributor

E-Paine commented Jul 6, 2021

@serhiy-storchaka are we good to merge this?

@taleinat
Copy link
Contributor Author

taleinat commented Jul 6, 2021

I just had Python segfault while running IDLE with this change. Let's hold off on merging for now.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 7, 2021
@ambv
Copy link
Contributor

ambv commented May 17, 2022

This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.

@terryjreedy terryjreedy changed the title bpo-40452: IDLE: fix clipboard being cleared upon exit gh-84632: IDLE: fix clipboard being cleared upon exit Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants