The Wayback Machine - https://web.archive.org/web/20250622174121/https://github.com/python/cpython/pull/19819
Skip to content

bpo-40452 #19819

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

Closed
wants to merge 10 commits into from
Closed

bpo-40452 #19819

wants to merge 10 commits into from

Conversation

E-Paine
Copy link
Contributor

@E-Paine E-Paine commented Apr 30, 2020

E-Paine added 2 commits April 30, 2020 18:26
On Windows, the tkinter clipboard gets cleared with the closure of the root. This commit forces IDLE (on Windows only) to preserve the clipboard contents even after all windows of an instance have been closed.
Imported the libraries required for the previous commit
Before, the call would occur whenever a toplevel closed, regardless of whether the root was also closing (which has now been fixed)
E-Paine added 2 commits May 15, 2020 15:29
I have addressed the issues ZackerySpytz has raised in his comments (namely reducing line-length, using a with statement and changing a broad except). The other minor changes are:
 - Clarification has been added on why we work directly with stdin rather than the Popen class
 - Root quit method has been moved to a 'finally'
Please leave a comment if errors other than 'OSError's should be ignored
When the clipboard was empty, tkinter would raise a TclError (which wasn't being caught). My point still stands about commenting if there are errors which I have missed and should be ignored.
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.

See comment about manual test on issue.
Remove trailing whitespace that caused Travis failure, and recheck with IDLE or patchcheck after other edits.

E-Paine and others added 5 commits May 17, 2020 16:09
I have committed @terryjreedy's suggested changes almost exactly, only changing '_tkinter.TclError' to 'TclError' for the new imports
Changed encoding type from UTF-8 to UTF-16 (see comment for explanation)
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.

I rearranged a bit. The patch fixes the paste-after close issue, but while exit is immediate (as signified by the Command Prompt > prompt), the patch is buggy in that it leaves the subprocess running. This is visible when running from a console.

f:\dev\3x>python -m idlelib
Running Debug|Win32 interpreter...

Warning (from warnings module):
  File "f:\dev\3x\lib\subprocess.py", line 1048
    _warn("subprocess %s is still running" % self.pid,
ResourceWarning: subprocess 12596 is still running
>>>
f:\dev\3x>

I tried sending Windows EOF ^s\n as bytes, but no effect. Ditto for adding p.close().
I tried subprocess.run('clip', input=clip) and complete exit is delayed for about 4 seconds instead of printing the warning. Neither is acceptable.
Pasting into IDLE before closing 'solves' the problem. Instead of using clip, I tried simulating this with a Toplevel and Text but so far failed.
So a change is needed but I don't know what.

Clearing the clipboard when IDLE closes strikes me as a bug somewhere. If not in IDLE's closing, it should be possible to write a short tkinter app showing the same issue when run directly, without IDLE.
@serhiy-storchaka Do you have any ideas as to what might be going on or how to better fix?

clip is meant to be used with in Command Prompt to put program output, either to stdout or a file, on the clipboard.
@eryksun Do you know what Windows system call clip uses? Can we access it via ctypes (as done near the top of pyshell for resolution settings)? Or do you know any other alternative to the current patch?

stdin=subprocess.PIPE).stdin as p:
p.write(clip)
# Popen '.communicate' & '.__exit__' call '.wait'.
except (OSError, TclError):
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe OSError is possible. In any case, we should try to write code that will not fail under anything like normal circumstances. Deleting this.

@terryjreedy
Copy link
Member

Depending on the final fix, I might want to put the Windows fix in a separate function.
I will regular and IDLE news items when the patch is acceptable.

@terryjreedy terryjreedy requested a review from taleinat May 18, 2020 03:20
@taleinat
Copy link
Contributor

I'm currently not in favor of this approach: It adds quite a bit of "fragile" code to work around an issue on just one platform, though the issue is not platform-specific.

Given our limited resources, I'd rather that we help fix the core issue at its ogirin rather than implement workarounds.

@E-Paine
Copy link
Contributor Author

E-Paine commented May 18, 2020

I'm currently not in favor of this approach: It adds quite a bit of "fragile" code to work around an issue on just one platform, though the issue is not platform-specific.

I agree, given that this is a Windows-only fix and the problem is not just on Windows (I can also reproduce the issue on my Linux machine), it may be better to use a different approach. I feel it may be worth pointing out, however, that pyperclip's "copy_wsl" method (for example) also pipes to "clip.exe" to get content onto the clipboard.

@taleinat
Copy link
Contributor

I feel it may be worth pointing out, however, that pyperclip's "copy_wsl" method (for example) also pipes to "clip.exe" to get content onto the clipboard.

That's true, thanks for pointing that out!

I'm not saying that such an approach is unreasonable in general: it obviously does work. However, while implementing such support for many different platforms is in the domain of projects such as tcl/tk and pyperclip, IMO it would not be appropriate to add such code to IDLE or tkinter, as it would be a significant ongoing maintenance burden.

Using pyperclip seems like a no-go, as that would require bringing pyperclip into the stdlib, which I don't think would be acceptable, for much the same reason as above. Therefore, we're left with working out the core issue in tkinter and/or tcl/tk.

I'm closing this PR for now, as it seems this isn't a direction we're going to pursue, but it can be re-opened later if we change our minds.

@taleinat taleinat closed this May 18, 2020
@eryksun
Copy link
Contributor

eryksun commented May 18, 2020

Do you know what Windows system call clip uses? Can we access it via ctypes (as done near the top of pyshell for resolution settings)? Or do you know any other alternative to the current patch?

For "clip.exe", set the process creation flag DETACHED_PROCESS in order to avoid allocating a console. For example:

import os
import subprocess

clip_path = os.path.join(os.environ['SystemRoot'], 'System32', 'clip.exe')
p = subprocess.run('clip', executable=clip_path,
    creationflags=subprocess.DETACHED_PROCESS,
    input='spam & eggs', encoding='utf-16le')

Alternatively, here's a function I just wrote that copies Unicode text to the clipboard using ctypes:

import time
import ctypes
from ctypes import wintypes

kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)
user32 = ctypes.WinDLL('user32', use_last_error=True)

GMEM_MOVEABLE = 0x0002
CF_UNICODETEXT = 13

kernel32.GlobalAlloc.restype = wintypes.HGLOBAL
kernel32.GlobalFree.argtypes = (wintypes.HGLOBAL,)
kernel32.GlobalLock.restype = wintypes.LPVOID
kernel32.GlobalLock.argtypes = (wintypes.HGLOBAL,)
kernel32.GlobalUnlock.argtypes = (wintypes.HGLOBAL,)
user32.SetClipboardData.restype = wintypes.HANDLE
user32.SetClipboardData.argtypes = (wintypes.UINT, wintypes.HANDLE)

def text2clip(text):
    """Copy text to the clipboard."""
    # OpenClipboard fails with ERROR_ACCESS_DENIED if the clipboard
    # is owned by a window, so retry every 100 ms for a second.
    i = 0
    while not user32.OpenClipboard(None):
        i += 1
        if i > 9:
            raise ctypes.WinError(ctypes.get_last_error())
        time.sleep(0.1)
    try:
        if not user32.EmptyClipboard():
            raise ctypes.WinError(ctypes.get_last_error())
        text_array_t = ctypes.c_wchar * (len(text) + 1)
        hMem = kernel32.GlobalAlloc(GMEM_MOVEABLE, 
            ctypes.sizeof(text_array_t))
        if not hMem:
            raise ctypes.WinError(ctypes.get_last_error())
        pMem = kernel32.GlobalLock(hMem)
        text_array_t.from_address(pMem).value = text
        kernel32.GlobalUnlock(hMem)
        if not user32.SetClipboardData(CF_UNICODETEXT, hMem):
            kernel32.GlobalFree(hMem)
            raise ctypes.WinError(ctypes.get_last_error())
        # now the system owns hMem
    finally:
        user32.CloseClipboard()

@terryjreedy
Copy link
Member

@eryksun Thank you. It turns out that the problem also exists on Linux and macOS, so I will try harder with a cross-platform solution using tkinter.

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.

7 participants