Skip to content

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

Open
taleinat wants to merge 9 commits into
python:mainfrom
taleinat:bpo-40452/IDLE-preserve-clipboard-upon-exit
Open

gh-84632: IDLE: fix clipboard being cleared upon exit#26163
taleinat wants to merge 9 commits into
python:mainfrom
taleinat:bpo-40452/IDLE-preserve-clipboard-upon-exit

Conversation

@taleinat

@taleinat taleinat commented May 16, 2021

Copy link
Copy Markdown
Contributor

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

E-Paine commented May 16, 2021

Copy link
Copy Markdown
Contributor

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

taleinat commented May 16, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Misc/NEWS.d/next/IDLE/2021-05-16-16-24-31.bpo-40452.Nayfdf.rst Outdated
Comment thread Lib/idlelib/pyshell.py Outdated
Comment thread Modules/_tkinter.c Outdated
@bedevere-bot

Copy link
Copy Markdown

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

@taleinat

taleinat commented May 16, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@terryjreedy

Copy link
Copy Markdown
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>
Comment thread Lib/idlelib/pyshell.py Outdated
root.mainloop()
root.destroy()
capture_warnings(False)
import _tkinter

Copy link
Copy Markdown
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
Copy Markdown
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?

@E-Paine E-Paine May 19, 2021

Copy link
Copy Markdown
Contributor

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

@taleinat taleinat May 19, 2021

Copy link
Copy Markdown
Contributor Author

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.

Comment thread Lib/idlelib/pyshell.py Outdated
@E-Paine

E-Paine commented May 17, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

E-Paine commented May 19, 2021

Copy link
Copy Markdown
Contributor

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

taleinat commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

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.

@github-actions

Copy link
Copy Markdown

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

E-Paine commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

@serhiy-storchaka are we good to merge this?

@taleinat

taleinat commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

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 commented May 17, 2022

Copy link
Copy Markdown
Contributor

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

rdbende commented Apr 7, 2023

Copy link
Copy Markdown

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
Copy Markdown
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

python-cla-bot Bot commented Apr 18, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label May 8, 2025
@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown

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 Apr 9, 2026
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 30, 2026
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting change review DO-NOT-MERGE needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.