python / cpython

The Python programming language
https://www.python.org
Other
62.15k stars 29.86k forks source link

Tkinter/IDLE: preserve clipboard on closure #84632

Open 919475b3-36a2-4cd7-997c-9c38f05f93c7 opened 4 years ago

919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago
BPO 40452
Nosy @terryjreedy, @taleinat, @serhiy-storchaka, @eryksun, @pablogsal, @E-Paine
PRs
  • python/cpython#19819
  • python/cpython#26163
  • Files
  • tkinter-clipboard-on-exit.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/terryjreedy' closed_at = None created_at = labels = ['type-bug', 'expert-tkinter', '3.9', '3.10', '3.8', 'expert-IDLE'] title = 'Tkinter/IDLE: preserve clipboard on closure' updated_at = user = 'https://github.com/E-Paine' ``` bugs.python.org fields: ```python activity = actor = 'primexx' assignee = 'terry.reedy' closed = False closed_date = None closer = None components = ['IDLE', 'Tkinter'] creation = creator = 'epaine' dependencies = [] files = ['49173'] hgrepos = [] issue_num = 40452 keywords = ['patch'] message_count = 45.0 messages = ['367765', '369002', '369135', '369156', '369165', '369199', '369287', '369304', '369322', '369334', '369338', '369383', '369387', '369389', '369391', '369392', '369395', '369406', '369440', '369444', '369453', '369456', '369601', '376354', '380199', '382635', '393742', '393743', '393750', '393753', '393755', '393756', '393757', '393770', '393788', '393790', '393794', '393795', '393797', '393798', '393804', '393805', '393806', '393817', '393818'] nosy_count = 8.0 nosy_names = ['terry.reedy', 'taleinat', 'serhiy.storchaka', 'eryksun', 'pablogsal', 'epaine', 'Spacekiwigames', 'primexx'] pr_nums = ['19819', '26163'] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue40452' versions = ['Python 3.8', 'Python 3.9', 'Python 3.10'] ```

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    Currently, on a Windows machine, the clipboard contents is lost when the root is closed. This, therefore requires you to keep the IDLE instance open until after the copy has been complete (particularly annoying when copying between different IDLE instances). The solution is to pipe the tkinter clipboard contents to "clip.exe" (built-in to Windows) which will preserve it after root closure.

    terryjreedy commented 4 years ago

    Please write out a manual test example (steps 1, 2, ..., N) that fails now and passes with the patch.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    Test example:

    1) Open IDLE (shell or editor) 2) Copy some text in the IDLE window 3) Close the IDLE window (instance should end) 4) Paste into application of choice Without the patch, the clipboard is cleared when the instance ends so nothing is pasted. With the patch, the content remains in the clipboard as expected.

    Note: This is not a problem if clipboard history is turned on or the content is pasted before closure, however the order of events above is fairly common for me.

    Encoding:

    In the latest commit to the pull, I have changed the encoding from UTF-8 to UTF-16 and my reasoning is as follows:

    1) Most importantly, using UTF-8, characters with a unicode value >=2^8 are incorrectly copied as multiple characters. UTF-8 does support these characters but uses a different number of bytes per character (which is presumably what is causing these issues - for example, "AĀ" is encoded as "\x41\xc4\x80", which pastes as "A─Ç") UTF-16, however, correctly works for all characters supported by Tcl (see next point).

    2) "Strings in Tcl are encoded using 16-bit Unicode characters" (https://www.tcl.tk/man/tcl8.2.3/TclCmd/encoding.htm). Therefore, the encoding we choose should have at least 16 bits allocated per character (meaning either UTF-16 or UTF-32).

    3) Windows' "Unicode-enabled functions [...] use UTF-16 (wide character) encoding, which is [...] used for native Unicode encoding on Windows operating systems" (https://docs.microsoft.com/en-us/windows/win32/intl/unicode). For me, this was what decided the new encoding (between the two given in the previous point).

    terryjreedy commented 4 years ago

    Now that I think about it, I have run into enough problems with ^V not pasting something copied with ^C that I always leave source windows open until successful. I had not noticed that there is only a problem between windows (but this may be true) or after closing IDLE or that it only occurs when copying *from* IDLE. In fact, the last might not be true if other apps have the same bug. (I will start paying more attention after this is fixed.)

    terryjreedy commented 4 years ago

    eryksun, Piping to clip.exe is not working well. On the patch, I asked if you know what Windows system call it uses, but I cannot request your review there.

    taleinat commented 4 years ago

    I can reproduce this behavior on macOS as well, so this doesn't seem to be Windows-specific.

    This does not happen with other apps in general, so it is not normal behavior for apps.

    Testing with a minimal tkinter app (see code below) gives similar behavior, so this appears to be an issue with tkinter and/or tcl/tk.

    import tkinter
    text = tkinter.Text()
    text.pack()
    tkinter.mainloop()
    taleinat commented 4 years ago

    I closed the PR but IMO this issue should remain open.

    I am changing the title, though, since this is not actually Windows-specific.

    terryjreedy commented 4 years ago

    Thanks Tal. I should have inquired about behavior on *nix before putting so much effort into a Windows-only workaround, and not try to rush something into the beta coming out today or tomorrow. I will return to trying to do an invisible paste into a bare Text widget.

    taleinat commented 4 years ago

    Looking into this further, this seems to be an issue in tkinter rather than with tcl/tk. Running wish a.tcl with the following simple script does leave its text in the clipboard:

    pack [text .t] .t insert 1.0 "Test text" clipboard clear clipboard append -- [.t get 1.0 end] exit

    On the other hand, the following equivalent Python script leaves the clipboard empty:

    import tkinter
    text = tkinter.Text()
    text.pack()
    text.clipboard_clear()
    text.clipboard_append("Testing again")
    terryjreedy commented 4 years ago

    I ran your tkinter code on Windows, without IDLE, and the clipboard was clear thereafter. I added 'test.mainloop()' and observed the following.

    1. Close immediately, clipboard is clear, as before.

    2. Paste 'Testing again' into 'text' with ^V and close, and clipboard is still clear.

    3. Paste elsewhere (here, Command Prompt, or IDLE), close, and 'Testing again' can be pasted anywhere.

    4. When I run from an IDLE editor, paste into the editor (which in running in the IDLE process rather than the user process where the test code is running, and close, 'Testing again' remains on the clipboard.

    It appears that either a. tkinter clear the clipboard unless pasted into another process, or b. tkinter (or tk) always clears the clipboard on exit and it is gone unless pulled out of the process first by pasting elsewhere. The best option is for this to be fixed.

    The example codes are not quite equivalent. Tkinter clipboard_clear and _append call self.tk.call with the equivalent arguments *plus*, self._options({'displayof':text._w}) == ('-displayof', '.!text') However, commenting out the addition had no visible effect.

    taleinat commented 4 years ago

    Note that Tcl/Tk once had exactly the same issue on Windows, and they added specific code to "render" to the clipboard upon exit if the current app is the "clipboard owner".

    https://core.tcl-lang.org/tk/tktview/732662

    This was fixed here:

    https://core.tcl-lang.org/tk/tktview/939389

    Note that Tcl/Tk has separate OS-specific implementations of "TkClipCleanup" for Windows, macOS and "Unix".

    https://github.com/tcltk/tk/search?q=TkClipCleanup

    This only appears to be called by TkCloseDisplay, which is turn is never called elsewhere in the Tk codebase. tkinter never appears to call either of those functions. Perhaps this is the core issue?

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    I'm (sadly) not particularly familiar with C, though I have tried to trace the calls of TkClipCleanup. As @taleinat mentioned, it is called by the TkCloseDisplay, though, in turn, I believe this method is called by the DeleteWindowsExitProc method. The name "DeleteWindowsExitProc" is passed, in the Initialize method, to the TkCreateThreadExitHandler method (I assume method references work in a similar-enough way to Python?) along with the tsdPtr object.

    I stopped tracing it at this point, and instead worked from the Python end. The Tkapp_New method calls the Tcl_CreateInterp method and I have three questions about this (each only applicable depending on the answer to the previous one):

    1. Is the Tkapp_New method the correct one to be looking at for tcl/tk initialisation (I can't find where _tkinter.create is implemented)?
    2. Is there a reason why the call is to tcl rather than tk?
    3. Would changing this to tk cause the fix in tk to be applied?
    taleinat commented 4 years ago

    Indeed, you've got that pretty much correct. The call chain is:

    Tk.__init__ _tkinter_create_impl (called as _tkinter.create() from Python) Tkapp_New

    Tkapp_New does a lot of things. In particular, it calls Tcl_CreateInterp and later Tcl_AppInit. Tcl_AppInit (apparently a local copy is usually used!) calls Tcl_Init and later Tk_Init.

    At this point the call chain enters tcl/tk code. Tk_Init calls Initialize (except on cygwin - I'm ignoring this). The inline comment for Initialize says "The core of the initialization code for Tk, called from Tk_Init and Tk_SafeInit." And at the very end of Initialize, we find the call TkCreateThreadExitHandler(DeleteWindowsExitProc, tsdPtr).

    taleinat commented 4 years ago

    During finalization, TkFinalizeThread would call DeleteWindowsExitProc (registered via TkCreateThreadExitHandler). This in turn is set as a thread-exit handler via Tcl_CreateThreadExitHandler upon the first call to TkCreateThreadExitHandler.

    Now we're out of Tk and into Tcl itself. TkFinalizeThread would be called by Tcl's Tck_FinalizeThread. This, in turn, is called by Tcl_Finalize (for thread number zero?). And _tkinter sets an atexit handler to call Tcl_Finalize.

    Ah, but no! That handler is actually not set, due to the "#if 0" before it! So the Tcl/Tk finalization is actually skipped, causing this issue.

    taleinat commented 4 years ago

    Indeed, adding a simple _tkinter.destroy() method which invokes Tcl_Finalize, and calling it in the Tk.destroy() method, makes copied text remain available after closing IDLE!

    I did the above to test my hypothesis, but I'm not sure it would be a proper fix.

    taleinat commented 4 years ago

    epaine, if you'd like to create a new PR based on these findings, I'd be happy to review it!

    terryjreedy commented 4 years ago

    After reporting my experiments above, msg369334, I made further failing efforts to simulate pasting into another process, as in 3 and 4. It might be that a concrete key event is needed. So I strongly suspect that the solution for IDLE is indeed a tkinter solution, and it seems that patching _tkinter.c is needed. (And a solution only for IDLE would not help other tkinter users.) Please continue.

    terryjreedy commented 4 years ago

    The #if 0 was added by Guido in 43ff8683fe68424b9c179ee4970bb865e11036d6 in 1998, before the tcl/tk clip fix for Windows.

    The code has a comment that was part of a multifile svn merge, so author unknown.

    /* This was not a good idea; through <Destroy> bindings,
       Tcl_Finalize() may invoke Python code but at that point the
       interpreter and thread state have already been destroyed! */

    Calling Tcl_Finalize() within Tk.destroy avoids this as .destroy is called while python is running, either explicitly or when the associated window is closed.

    However, it is possible to have more than 1 Tk instance, either accidentally or on purpose*, each with its own .tk, which I presume is the 'associated tcl interpreter' instance. So Tk.destroy may be called more than once and each call must not disable other Tk instances. To test: Create 2 roots and two Tk windows, destroy 1. Does the other window disappear? Does root2.withdraw raise? Does adding widgets raise?

    If yes, we would either need to count working Tk instances or try calling less shutdown stuff.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    Multiple Tk instances are already recommended against, but what would be the implications of preventing multiple roots?

    A simple check could be added to the Tk class init which ensures _default_root is None (if it is not None, an error is raised). In this case, I think it would be much easier for the proposed changes to _tkinter, but also make future maintenance of tkinter easier.

    I am currently investigating potential solutions based on what Tal has found, and will come back with details if I succeed (and thank you Tal for offering to review a PR).

    Separately, should we change the issue from IDLE to tkinter, as that the fix we are looking at applying?

    taleinat commented 4 years ago

    Regarding multiple Tk instances, IMO we should try the straightforward solution first: Tcl/Tk has its own mechanisms for handling per-interpreter state, so we may not actually need to handle any of this ourselves.

    Regarding the title of this issue, it is indeed a bug in Tkinter rather than IDLE, but it does affect IDLE significantly, and someone searching for this might search for IDLE rather than Tkinter. I suggest leaving the title as it is.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    After some initial testing, I have found that while calling Tcl_Finalize on window closure keeps the clipboard contents (as expected), it also finishes the Python interpreter.

    The solution was to instead use the Tcl_FinalizeThread method, "which you can call if you just want to clean up per-thread state" (https://www.tcl.tk/man/tcl8.4/TclLib/Exit.htm).

    Reading this, I was expecting it to stop further Tk instances from being created after the original one was closed, however some initial testing has found this to not be true.

    I feel it is too early to create a PR for this yet (I need to do more research and properly understand the calls), but it is quite possible calling this method on root "destroy" is the only thing required to fix this issue.

    taleinat commented 4 years ago

    Attaching the changes I made while testing as a patch file.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 4 years ago

    Unfortunately, after lots of testing/experimenting, I cannot find a way to make the correct call/s at the correct time. The methods that call the exit handlers directly or through InvokeExitHandlers are Tcl_Exit, Tcl_Finalize & FinalizeThread (Tcl_FinalizeThread) – Tcl_ExitThread calls Tcl_FinalizeThread.

    We want to call the exit handlers (to access TkClipCleanup) with as little else as possible and both Tcl_Exit & Tcl_Finalize call FinalizeThread, so I concluded the method we should call is Tcl_FinalizeThread. Tcl_Finalize also stops the Python interpreter (not quite sure why), so this should only be called when the interpreter is stopping/stopped.

    The problem with Tcl_FinalizeThread, is that it also finalises some of the mutexs (most notably asyncMutex - part of the async system). Once the mutexs are finalised, I can't find a way of creating them again (I think they are a global variables created on load), meaning that a new tcl interpreter cannot be created in the same thread (and even if it can, any calls to it cause the Python interpreter to crash).

    This means we cannot call the Tcl_FinalizeThread method either when the root is destroyed or when the Tkapp object is deleted, as the user could still request a new tcl interpreter. This leaves us with only one option: call it when the Python interpreter is closing. For this, which call is made doesn’t really matter, and so to test, I commented out the "#if 0", to see if this call fixed the clipboard issue (if it had worked, we could have deleted all bindings before calling Tcl_Finalize). Unfortunately, this did not fix the clipboard issue and so I did not deem it worth developing an "unbinder" (possibly because the GC had already destroyed the interpreter?).

    I did not even get onto the issue of multiple, simultaneous interpreters but unless someone has an idea that could solve any of the issues above (or we ask the Tcl team to make TkClipCleanup a public method!), I can’t see how we can patch this issue.

    terryjreedy commented 4 years ago

    I closed duplicate issue bpo-41709, Linux Mint 19.3, python 3.6.8

    taleinat commented 3 years ago

    Perhaps we could get a Tcl/Tk dev to help with this?

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago

    Removed Alex from nosy because the link was an advertisement for an essay writer.

    taleinat commented 3 years ago

    Okay, so it appears that a generic solution for tkinter is non-trivial, as it is not clear when to call Tcl_Finalize() or Tcl_FinalizeThread().

    However, giving tkinter apps the ability to call those when appropriate is possible. Exposing a thin wrapper around Tcl_Finalize in _tkinter, and making IDLE call that when exiting, resolves the clipboard preservation issue.

    For some reason, calling Tcl_Finalize causes a memory segmentation issue for me on Ubuntu 20.04. However, calling Tcl_FinalizeThread also resolves the clipboard issue without causing problems. On Windows 10, calling either function works.

    taleinat commented 3 years ago

    See PR python/cpython#70351.

    terryjreedy commented 3 years ago

    Serhiy: what do you know of tcl finalization?

    Tal: why does your patch only expose the option that you said crashes ubuntu.

    Spacekiwigames: if you have a cpython fork and local repository, you could try the patch on your linux mint.

    terryjreedy commented 3 years ago

    Tal: cancel comment above. I was confused because FinalizeThread is exposed as just finalize.

    On Windows, PR fixes issue whether Shell or editor is closed last.

    terryjreedy commented 3 years ago

    Can we backport? The change to _tkinter, not directly exposed in tkinter, could be considered an enhancement, but is necessary to fix what I consider a real bug in tkinter, and hence in IDLE.

    Pablo, would you allow the _tkinter change in .0b2?

    If we cannot backport the _tkinter change, can the tcl function be accessed through root.tk.call('????')? I am guessing that 'exit' exits the process from tcl, which is probably not what we want. https://www.tcl.tk/man/tcl8.6/TclLib/Exit.htm suggests that when tcl is loaded as an extention, Tcl_Finalize should be called instead of Tcl_Exit to *not* exit the process.

    terryjreedy commented 3 years ago

    As Tal notes on the PR, the _tkinter patch is the minimum needed to fix the problem for IDLE, not the undetermined patch to tkinter that should be done to properly expose the fix to all tkinter users.

    If the tcl function were exposed with a presumably temporary private name, '_finalize_tcl', rather than a public name, 'finalize_tcl', then I presume we should be able to backport without question.

    taleinat commented 3 years ago

    Can we backport? The change to _tkinter, not directly exposed in tkinter, could be considered an enhancement, but is necessary to fix what I consider a real bug in tkinter, and hence in IDLE.

    I'm +1 for backporting the latest PR. It only adds the clearly internal and undocumented _tkinter.finalize_tcl(), which is only used by IDLE.

    pablogsal commented 3 years ago

    Pablo, would you allow the _tkinter change in .0b2?

    I checked the change and I think is scoped enough and doesn't change the public API (and it solves a bug). Is fine with me, but please make sure of the following:

    Thanks for checking!

    serhiy-storchaka commented 3 years ago

    What if register Tcl_Finalize as an atexit callback?

    taleinat commented 3 years ago

    What if register Tcl_Finalize as an atexit callback?

    Precisely that was tried a long time ago (2002?) and decided against, as can be seen by the comment left behind at the end of _tkinter.c:

    #if 0
        /* This was not a good idea; through <Destroy> bindings,
           Tcl_Finalize() may invoke Python code but at that point the
           interpreter and thread state have already been destroyed! */
        Py_AtExit(Tcl_Finalize);
    #endif
    serhiy-storchaka commented 3 years ago

    That comment may be outdated. The atexit callbacks are called very early at the interpreter shutdown stage. The interpreter is still entirely intact at this point.

    taleinat commented 3 years ago

    Wow, I'm very surprised! Simply uncommenting that works perfectly on Ubuntu 20.04 and Windows 10.

    I'll make a new PR...

    serhiy-storchaka commented 3 years ago

    It was commented out in

    commit 43ff8683fe68424b9c179ee4970bb865e11036d6 Author: Guido van Rossum \guido@python.org\ Date: Tue Jul 14 18:02:13 1998 +0000

    Temporarily get rid of the registration of Tcl_Finalize() as a
    low-level Python exit handler.  This can attempt to call Python code
    at a point that the interpreter and thread state have already been
    destroyed, causing a Bus Error.  Given the intended use of
    Py_AtExit(), I'm not convinced that it's a good idea to call it
    earlier during Python's finalization sequence...  (Although this is
    the only use for it in the entire distribution.)

    Maybe Guido remember more details about reproducing a Bus Error. Was it an issue from the other side, when the Tcl/Tk interpreter was used after calling Tcl_Finalize()?

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago

    Simply uncommenting that works perfectly on Ubuntu 20.04 and Windows 10.

    I can't remember the details, but I don't think this worked if the Tcl interpreter had been garbage collected before the Python interpreter shutdown. Therefore, Tcl_Finalize (or similar) would preferably be called at Tkapp dealloc, but we cannot do that because the user may create more interpreters later. I will try and give a reproducible example when I have time.

    terryjreedy commented 3 years ago

    Tcl_Finalize can be called more than once, says the tcl doc. It also says it, or T_FThread should be called when unloading. If the tcl implementation on some system does not match the doc, then there is a bug that should be reported.

    If the registration is done by tkinter, rather than IDLE, then all tkinter apps benefit.

    taleinat commented 3 years ago

    Tcl_Finalize can be called more than once, says the tcl doc.

    I take that to mean that when calling Tcl_Finalize more than once, the second and following calls will do nothing rather than crash the process, corrupt memory etc.

    The Tcl docs are quite clear that Tcl is no longer usable after calling Tcl_Finalize. Therefore, E. Paine's latest comment appears to be correct in the sense that we can't call Tcl_Finalize when cleaning up a tkinter.Tk instance. The same goes for Tcl_FinalizeThread.

    taleinat commented 3 years ago

    After thinking about this a bit more, it seems to me that using an atexit handler is definitely worth pursuing, but not for 3.10.0.

    I'm in favor of merging the existing PR as an immediate fix for IDLE, and following up with a general solution for tkinter using atexit, which should also include some tests so that we can see that it works on our CI and buildbots.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago

    I don't think this worked if the Tcl interpreter had been garbage collected

    Turns out I was wrong, it appears it *does* work. The only thing we need to change is ensuring the GIL is held when calling finalise (because, as Guido's comment says, this process can call Python code).

    gvanrossum commented 3 years ago

    I don't recall anything about that, sorry.

    Viamo commented 1 month ago

    I have the same problem on Windows. If you copy text to the clipboard and close the application, the clipboard is lost. If you copy text to the clipboard and then use the copied data in any other application before closing the Tkinter application and then close the Tkinter application, the clipboard is not lost. It looks like the data being written belongs to the application and not to the clipboard, so this data is cleared by the resource collector on closing. But if you use the clipboard before closing the application, Windows makes a copy of the clipboard in its memory and clearing resources by the application no longer affects the contents of the clipboard.

    Are there any solutions available at this time?