Tkinter: Don't stringify callback arguments

[serhiy-storchaka]

BPO	22214
Nosy	@terryjreedy, @serhiy-storchaka
PRs	bpo-13153: Use OS native encoding for converting between Python and Tcl. #16545
Dependencies	bpo-21585: Run Tkinter tests with wantobjects=False
Files	tkinter_obj_cmd.patch tkinter_obj_cmd_2.patch tkinter_obj_cmd_3.patch tkinter_obj_cmd_4.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:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = None
created_at = <Date 2014-08-17.14:24:09.210>
labels = ['3.7', '3.8', 'type-feature', 'expert-tkinter', '3.9']
title = "Tkinter: Don't stringify callback arguments"
updated_at = <Date 2019-10-02.17:57:31.798>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2019-10-02.17:57:31.798>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = False
closed_date = None
closer = None
components = ['Tkinter']
creation = <Date 2014-08-17.14:24:09.210>
creator = 'serhiy.storchaka'
dependencies = ['21585']
files = ['36393', '36406', '43468', '43476']
hgrepos = []
issue_num = 22214
keywords = ['patch']
message_count = 7.0
messages = ['225446', '225480', '225494', '268847', '268868', '268871', '268883']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'gpolo', 'serhiy.storchaka']
pr_nums = ['16545']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22214'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

Linked PRs

• gh-66410: Do not stringify arguments of Tkinter callback #98592

[serhiy-storchaka]

Currently Tkinter supports two modes:

• When wantobjects=1 (default), all results of Tcl/Tk commands are converted to appropriate Python objects (int, float, str, bytes, tuple) if it is possible or to Tcl_Obj for unknown Tcl types.
• When wantobjects=0 (legacy mode), all results of Tcl/Tk commands are converted to strings.

But arguments of a callback are always converted to str, even when wantobjects=1. This can lost information ("42" is not distinguished from 42 or (42,)) and may be less efficient. Unfortunately we can't just change Tkinter to pass callback arguments as Python objects, this is backward incompatible change. Proposed patch introduces third mode wantobjects=2. It is the same as wantobjects=1, but callbacks now received . Default mode is still wantobjects=1, it can be changed in future. But in Tkinter applications (IDLE, Turtledemo, Pynche) we can use wantobjects=2 right now.

The only problem left is documenting it. The wantobjects parameter is not documented at all except an entry in "What's New in Python 2.3". Does anyone want to document this feature?

[terryjreedy]

I think I requested elsewhere that public _tkinter/tkinter names be documented. What you wrote already seems clear enough.

"Default mode is still wantobjects=1," The patch appears to change it to 2 in tkinter/init.py.

"/* Create argument list (argv1, ..., argvN) */
if (!(arg ..."

/arg/args/ is definitely less confusing. Add /list/tuple/ to old and new functions.

One thing slightly puzzles me: the current PythonCmd is used in a call to Tcl_CreateCommand. (Since the latter is not in _tkinter, I presume it is 'imported' in one of the includes.) The new PythonObjCmd is passed to Tcl_CreateObjCommand. Does that already exist, just waiting for us to add PythonObjCmd?

I near as I can tell, the only differences between PythonCmd and PythonOjbCmd are /argv/objv/ and the following.
  
      PyObject *s = unicodeFromTclString(argv[i + 1]);
      PyObject *s = FromObj(data->self, objv[i + 1]);

I think I would make the name substitution either in both or neither. It would be nice to reuse the rest of the code.

[serhiy-storchaka]

The patch appears to change it to 2 in tkinter/init.py.

This is for development only. You can apply the patch and test how new mode
affects IDLE or other applications.

One thing slightly puzzles me: the current PythonCmd is used in a call to
Tcl_CreateCommand. (Since the latter is not in _tkinter, I presume it is
'imported' in one of the includes.) The new PythonObjCmd is passed to
Tcl_CreateObjCommand. Does that already exist, just waiting for us to add
PythonObjCmd?

All Tcl_* functions are Tcl C API functions. Old functions work with strings
(char *). Modern functions (have "Obj" in a name) work with specialized Tcl
objects (Tcl_Obj *).

I near as I can tell, the only differences between PythonCmd and
PythonOjbCmd are /argv/objv/ and the following.

  PyObject \*s = unicodeFromTclString(argv[i + 1]);
  PyObject \*s = FromObj(data-\>self, objv[i + 1]);

I think I would make the name substitution either in both or neither. It
would be nice to reuse the rest of the code.

I'm not sure that I understand all you mean, but in updated patch the common
code are extracted to separate function.

[serhiy-storchaka]

Updated patch. Added NEWS and What's New entries.

[terryjreedy]

There is no review button for the new patch (don't know why), so: in the News and What's New entries, change "if call" to "if one calls" and "or set" to "or sets".

More importantly, question your use of 'callback' in the entries. To me, a callback is a function that is passed to a recipient to be called *by the recipient* at some later time. For tkinter, there are command callbacks, which get no arguments, validatecommand callbacks, which get string arguments, event callbacks, which get a tkinter event object as argument, and after callbacks, which get as arguments the Python objects given to the after call. In all cases, there is no visible issue of passing non-strings as strings to the callback. The following duplicates the testfunc and interp.call in test_tcl.py as closely as I know how, but with a different result.

import tkinter as tk
root = tk.Tk()
def test(*args):
    for arg in args:
        print(type(arg), '', repr(arg))
root.after(1, test, True, 1, '1')
root.mainloop()

To me, the revised test does not involve a callback. The test registers a name for the python function so it can be passes by name to Tk().call, as required by the .call signature. So I would replace 'callback' with 'Python function registered with tk'.

Registered functions can, of course, be used as callback if they have the proper signature. ConfigDialog registers an 'is_int' function so it can be passed to an Entry as a validatecommand callback. The patch does not affect registered validate commands because they receive string args.

After grepping idlelib for "register(", I believe this is the only function IDLE registers with tk (two of its classes have non-tk .register methods). Hence, IDLE should not be affected by the patch and seems not as far as I tested.

I can't properly review the _tkinter patch as it requires too much knowledge of tcl/tk interfaces.

[serhiy-storchaka]

Thank you Terry for your review. Here is updated patch fith fixed wording as you suggested. Hope it will be available to review on Rietveld.

The original purpose of this patch was to help fixing some IDLE "crashes". Percolator is registered in text widgets for highlighting pasted text. If you paste invalid string containing a lone surrogate on Windows (clipboard uses UTF-8), it first converted by Tcl to char*, and then Tkinter should convert it to Python string for passing to registered Python function, but fails. Since the information is lost during conversion in Tcl, we can't use say "surrogatepass" error handler fo representing non-well-formed data. With this patch we can decode just from Tcl unicode string.

[terryjreedy]

Review works. "to Python function" should be either "to a Python function" or "to Python functions".

Preventing crashes is a justifying use case. Percolator registers its 'insert' and 'delete' methods with a WidgetRedirector. It then patches them into to text instance, thereby inducing tk to call its replacements without their being registered with tk. Can you add a test case based on this?

I tried the following, based on what Redirector does, without and with the patch and got the same result each time.
import tkinter as tk
root = tk.Tk()
text = tk.Text(root)
s = '\ud800'
text.insert('1.0', s)
print(text.get('1.0'))
def insert(index, s):
    Text.insert(text, index, s)
text.insert('1.1', s)
print(text.get('1.1'))
root.clipboard_append(s)
text.insert('1.2', text.clipboard_get())
print(text.get('1.2'))
### output
�
�
Traceback (most recent call last):
  File "F:\Python\mypy\tem2.py", line 12, in <module>
    text.insert('1.2', text.clipboard_get())
  File "F:\Python\dev\36\lib\tkinter\__init__.py", line 730, in clipboard_get
    return self.tk.call(('clipboard', 'get') + self._options(kw))
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xed in position 0: invalid continuation byte

Can the above be changed to fail, then succeed?