Skip to content
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

Tkinter: Don't stringify callback arguments #66410

Closed
serhiy-storchaka opened this issue Aug 17, 2014 · 8 comments · Fixed by #98592
Closed

Tkinter: Don't stringify callback arguments #66410

serhiy-storchaka opened this issue Aug 17, 2014 · 8 comments · Fixed by #98592
Assignees
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir topic-tkinter type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 17, 2014

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

    @serhiy-storchaka
    Copy link
    Member Author

    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?

    @serhiy-storchaka serhiy-storchaka added topic-tkinter type-feature A feature request or enhancement labels Aug 17, 2014
    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

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

    @serhiy-storchaka serhiy-storchaka self-assigned this Jun 19, 2016
    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy terryjreedy changed the title Tkinter: Don't stringify callbacks arguments Tkinter: Don't stringify callback arguments Jun 19, 2016
    @serhiy-storchaka
    Copy link
    Member Author

    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
    Copy link
    Member

    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?

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 (EOL) end of life 3.9 only security fixes labels Oct 2, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 24, 2022
    Callbacks registered in the tkinter module now take arguments as
    various Python objects (int, float, bytes, tuple), not just str.
    To restore the previous behavior set tkinter module global wantobject to 1
    before creating the Tk object or call the wantobject() method of the Tk object
    with argument 1.
    Calling it with argument 2 restores the current default behavior.
    @serhiy-storchaka serhiy-storchaka added 3.12 bugs and security fixes and removed 3.9 only security fixes 3.8 (EOL) end of life 3.7 (EOL) end of life labels Oct 24, 2022
    @serhiy-storchaka
    Copy link
    Member Author

    PR #98592 is simpler than the initial patches. The What's New entry and the NEWS entry also were rewritten, because the behavior is now changed by default.

    Example in #66410 (comment) now fails on text.insert() on Linux, because using of UTF-8 in strict mode.

    @iritkatriel iritkatriel added the stdlib Python modules in the Lib dir label Nov 23, 2023
    @erlend-aasland erlend-aasland added the 3.13 bugs and security fixes label Jan 5, 2024
    serhiy-storchaka added a commit that referenced this issue May 7, 2024
    Callbacks registered in the tkinter module now take arguments as
    various Python objects (int, float, bytes, tuple), not just str.
    To restore the previous behavior set tkinter module global wantobject to 1
    before creating the Tk object or call the wantobject() method of the Tk object
    with argument 1.
    Calling it with argument 2 restores the current default behavior.
    SonicField pushed a commit to SonicField/cpython that referenced this issue May 8, 2024
    …nGH-98592)
    
    Callbacks registered in the tkinter module now take arguments as
    various Python objects (int, float, bytes, tuple), not just str.
    To restore the previous behavior set tkinter module global wantobject to 1
    before creating the Tk object or call the wantobject() method of the Tk object
    with argument 1.
    Calling it with argument 2 restores the current default behavior.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir topic-tkinter type-feature A feature request or enhancement
    Projects
    None yet
    Development

    Successfully merging a pull request may close this issue.

    4 participants