Skip to content

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

Open
wants to merge 9 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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. name
  2. blame

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

@bedevere-bot
Copy link

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

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@terryjreedy
Copy link
Member

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 <tjreedy@udel.edu>
@@ -1698,6 +1698,8 @@ def main():
root.mainloop()
root.destroy()
capture_warnings(False)
import _tkinter
Copy link
Member

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

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.

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

Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
@taleinat
Copy link
Contributor Author

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

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

@github-actions
Copy link

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 ambv removed the needs backport to 3.9 only security fixes label May 17, 2022
@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
@rdbende
Copy link

rdbende commented Apr 7, 2023

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

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.

@hugovk hugovk removed the needs backport to 3.10 only security fixes label Apr 8, 2023
@arhadthedev
Copy link
Member

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

I smoke-tested (python -m idlelib-select-Ctrl+C-close) IDLE from the head of this PR and couldn't reproduce the crash. @taleinat could you recheck if the underlying reason was fixed during last two years, please?

@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed needs backport to 3.11 only security fixes labels May 9, 2024
@tomasr8 tomasr8 removed the needs backport to 3.12 only security fixes label Apr 10, 2025
@python-cla-bot
Copy link

python-cla-bot bot commented Apr 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

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.