classification
Title: Tkinter: Don't stringify callback arguments
Type: enhancement Stage: patch review
Components: Tkinter Versions: Python 3.6
process
Status: open Resolution:
Dependencies: 21585 Superseder:
Assigned To: serhiy.storchaka Nosy List: gpolo, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2014-08-17 14:24 by serhiy.storchaka, last changed 2016-06-20 03:25 by terry.reedy.

Files
File name Uploaded Description Edit
tkinter_obj_cmd.patch serhiy.storchaka, 2014-08-17 14:24 review
tkinter_obj_cmd_2.patch serhiy.storchaka, 2014-08-18 14:52 review
tkinter_obj_cmd_3.patch serhiy.storchaka, 2016-06-19 08:56
tkinter_obj_cmd_4.patch serhiy.storchaka, 2016-06-19 21:31 review
Messages (7)
msg225446 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-17 14:24
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?
msg225480 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-08-18 02:25
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.
msg225494 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-18 14:52
> 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.
msg268847 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-19 08:56
Updated patch. Added NEWS and What's New entries.
msg268868 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-19 20:26
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.
msg268871 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-19 21:31
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.
msg268883 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-06-20 03:25
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?
History
Date User Action Args
2016-06-20 03:25:20terry.reedysetmessages: + msg268883
2016-06-19 21:31:23serhiy.storchakasetfiles: + tkinter_obj_cmd_4.patch

messages: + msg268871
2016-06-19 20:26:36terry.reedysetmessages: + msg268868
title: Tkinter: Don't stringify callbacks arguments -> Tkinter: Don't stringify callback arguments
2016-06-19 08:56:07serhiy.storchakasetfiles: + tkinter_obj_cmd_3.patch
assignee: serhiy.storchaka
messages: + msg268847

versions: + Python 3.6, - Python 3.5
2014-08-18 14:52:45serhiy.storchakasetfiles: + tkinter_obj_cmd_2.patch

messages: + msg225494
2014-08-18 02:25:50terry.reedysetmessages: + msg225480
2014-08-17 14:26:32serhiy.storchakasetdependencies: + Run Tkinter tests with wantobjects=False
2014-08-17 14:24:09serhiy.storchakacreate