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

tk busy command #72684

Closed
tkinter mannequin opened this issue Oct 21, 2016 · 43 comments
Closed

tk busy command #72684

tkinter mannequin opened this issue Oct 21, 2016 · 43 comments
Assignees
Labels
3.13 bugs and security fixes stdlib Python modules in the Lib dir topic-tkinter type-feature A feature request or enhancement

Comments

@tkinter
Copy link
Mannequin

tkinter mannequin commented Oct 21, 2016

BPO 28498
Nosy @ned-deily, @serhiy-storchaka, @csabella
Files
  • tk_busy.diff: Patch that add tk_busy commands to tkinter
  • tk_busy.diff: Patch for tk_busy commands against default branch
  • tk_busy.patch: Regenerated for review
  • tk_busy.patch: Regenerated for review
  • tk_busy_3.diff
  • tk_busy_4.diff
  • tk_busy_5.diff
  • tk_busy_6.diff
  • 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 2016-10-21.17:03:43.066>
    labels = ['expert-tkinter', '3.8', 'type-feature', 'library']
    title = 'tk busy command'
    updated_at = <Date 2018-02-22.01:06:47.241>
    user = 'https://bugs.python.org/tkinter'

    bugs.python.org fields:

    activity = <Date 2018-02-22.01:06:47.241>
    actor = 'ned.deily'
    assignee = 'serhiy.storchaka'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'Tkinter']
    creation = <Date 2016-10-21.17:03:43.066>
    creator = 'tkinter'
    dependencies = []
    files = ['45190', '45193', '45199', '45200', '45230', '45231', '45277', '45337']
    hgrepos = []
    issue_num = 28498
    keywords = ['patch']
    message_count = 43.0
    messages = ['279140', '279168', '279171', '279196', '279210', '279216', '279223', '279226', '279232', '279244', '279246', '279247', '279252', '279262', '279272', '279274', '279277', '279280', '279282', '279283', '279286', '279287', '279289', '279290', '279291', '279292', '279293', '279294', '279295', '279300', '279302', '279322', '279323', '279505', '279506', '279507', '279510', '279675', '279726', '279738', '279746', '279990', '312521']
    nosy_count = 6.0
    nosy_names = ['klappnase', 'ned.deily', 'serhiy.storchaka', 'tkinter', 'GNJ', 'cheryl.sabella']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28498'
    versions = ['Python 3.8']

    Linked PRs

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 21, 2016

    tcl tk 8.6.6 has a new busy command. The new tkinter library doesn't provide an interface for this command.

    https://www.tcl.tk/man/tcl/TkCmd/busy.htm

    The solution is to add to the class Misc of tkinter these methods:

    def tk_busy(self, *args, **kw):
        self.tk_busy_hold(*args, **kw)
        
    def tk_buy_hold(self, cnf=None, **kw)
        self.tk.call(('tk', 'busy', 'hold', self._w) + self._options(cnf, kw))
    
    def tk_buy_configure(self, cnf=None, **kw):
        self.tk.call(('tk', 'busy', 'configure', self._w) + self._options(cnf, kw))
        
    def tk_cget(self, option):
        return self.tk.call('tk', 'busy', 'cget', self._w, option)
    
    def tk_busy_forget(self):
        self.tk_call('tk', 'busy', 'forget', self._w)
    
    def tk_busy_current(self, pattern=None):
        if pattern is None:
            self.tk.call('tk', 'busy', 'current')
        else:
            self.tk.call('tk', 'busy', 'current', pattern)
    
    def tk_busy_status(self):
        return self.tk.call('tk', 'busy', 'status', self._w)

    @tkinter tkinter mannequin added topic-tkinter 3.7 (EOL) end of life labels Oct 21, 2016
    @serhiy-storchaka serhiy-storchaka added the stdlib Python modules in the Lib dir label Oct 21, 2016
    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 21, 2016
    @serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Oct 21, 2016
    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 21, 2016

    Funny thing, just today I came across the same issue.
    So far I came up with an a little bit refined version of the OP's suggestions. Until now I have tested this with Python-2.7.9 and tk-8.6.0 on debian Jessie, I think the same code should work for Python-3 , too.
    The busy_current() function here always reports only the root window (the same when called from within wish, apparently a bug in this early version of Tk-8.6) so I was not able to figure out how to use and test the "pattern" option.

        def busy(self, **kw):
            '''Shortcut for the busy_hold() command.'''
            self.tk.call(('tk', 'busy', self._w) + self._options(kw))
    
        def busy_cget(self, option):
            '''Queries the busy command configuration options for
            this window.
            The window must have been previously made busy by
            the busy_hold() command. Returns the present value
            of the specified option. Option may have
            any of the values accepted by busy_hold().'''
            return(self.tk.call('tk', 'busy', 'cget', self._w, '-'+option))
    
        def busy_configure(self, cnf=None, **kw):
            '''Queries or modifies the tk busy command configuration
            options. The window must have been previously made busy by
            busy_hold(). Option may have any of the values accepted by
            busy_hold().
            Please note that the option database is referenced by the
            window. For example, if a Frame widget is to be made busy,
            the busy cursor can be specified for it by :
                w.option_add("*Frame.BusyCursor", "gumby")'''
            if kw:
                cnf = _cnfmerge((cnf, kw))
            elif cnf:
                cnf = _cnfmerge(cnf)
            if cnf is None:
                return(self._getconfigure(
                            'tk', 'busy', 'configure', self._w))
            if type(cnf) is StringType:
                return(self._getconfigure1(
                            'tk', 'busy', 'configure', self._w, '-'+cnf))
            self.tk.call((
                'tk', 'busy', 'configure', self._w) + self._options(cnf))
        busy_config = busy_configure
    
        def busy_current(self, pattern=None):
            '''Returns the widget objects of all widgets that are
            currently busy.
            If a pattern is given, only the path names of busy widgets
            matching PATTERN are returned. '''
            return([self._nametowidget(x) for x in
                    self.tk.splitlist(self.tk.call(
                       'tk', 'busy', 'current', self._w, pattern))])
    
        def busy_forget(self):
            '''Releases resources allocated by the busy() command for
            this window, including the transparent window. User events will
            again be received by the window. Resources are also released
            when the window is destroyed. The window must have been
            specified in the busy_hold() operation, otherwise an
            exception is raised.'''
            self.tk.call('tk', 'busy', 'forget', self._w)
    
        def busy_hold(self, **kw):
            '''Makes this window (and its descendants in the Tk window
            hierarchy) appear busy. A transparent window is put in front
            of the specified window. This transparent window is mapped
            the next time idle tasks are processed, and the specified
            window and its descendants will be blocked from user
            interactions. Normally update() should be called immediately
            afterward to insure that the hold operation is in effect before
            the application starts its processing. The following
            configuration options are valid:
                -cursor cursorName
                    Specifies the cursor to be displayed when the widget
                    is made busy. CursorName can be in any form accepted
                    by Tk_GetCursor. The default cursor is "wait" on
                    Windows and "watch" on other platforms.'''
            self.tk.call((
                    'tk', 'busy', 'hold', self._w) + self._options(kw))
    
        def busy_status(self):
            '''Returns the busy status of this window.
            If the window presently can not receive user interactions,
            True is returned, otherwise False.'''
            return((self.tk.getboolean(self.tk.call(
                    'tk', 'busy', 'status', self._w)) and True) or False)

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 21, 2016

    Oops, just made a quick test with Python3, and found that I had missed that types.StringType is no longer present. So to be compatible with Python3 the busy_configure() func ought to look like:

        def busy_configure(self, cnf=None, **kw):
            if kw:
                cnf = _cnfmerge((cnf, kw))
            elif cnf:
                cnf = _cnfmerge(cnf)
            if cnf is None:
                return(self._getconfigure(
                            'tk', 'busy', 'configure', self._w))
            if isinstance(cnf, str):
                return(self._getconfigure1(
                            'tk', 'busy', 'configure', self._w, '-'+cnf))
            self.tk.call((
                'tk', 'busy', 'configure', self._w) + self._options(cnf))

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 22, 2016

    Maybe it's better to add also these methods:
    busy = tk_busy
    busy_cget = tk_busy_cget
    busy_configure = tk_busy_configure
    busy_current = tk_busy_current
    busy_forget = tk_busy_forget
    busy_hold = tk_busy_hold
    busy_status = tk_busy_status

    Many other methods in tkinter module follow the same pattern. For example:

        iconbitmap = wm_iconbitmap
        iconify = wm_iconify
        group = wm_group
        geometry = wm_geometry
        focusmodel = wm_focusmodel
        frame = wm_frame

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 22, 2016

    Ok, I investigated this a little further. First I noticed another bug with the code from my first post, the "self._w" must be omitted from the call to busy_current(), so the func should look like:

        def busy_current(self, pattern=None):
            return([self._nametowidget(x) for x in
                    self.tk.splitlist(self.tk.call(
                       'tk', 'busy', 'current', pattern))])

    I did then some more testing, the code now seems to work flawlessly both with Python-3.5.2 on windows 10 and with Python 2.7 on debian Jessie. So I finally prepared a patch (against the __init__.py file from the 3.5.2 windows installer). Following Miguel's suggestion I also added aliases prefixed with tk_ to the various busy_... methods.
    Since I am not familiar with Python's "official" test mechanisms, for now I wrote a simple script that does a number of calls to the tk_busy_... functions to test if everything works as expected:

    try:
    import Tkinter
    except:
    import tkinter as Tkinter
    #Tkinter.wantobjects = False

    root = Tkinter.Tk()
    f = Tkinter.Frame(root, name='f')
    f.pack(fill='both', expand=1)
    b=Tkinter.Button(f, name='b', text='hi', command=root.bell)
    b.pack(padx=100, pady=100)
    
    top = Tkinter.Toplevel(root, name='top')
    
    def test1():
        root.tk_busy()
    def test2():
        root.tk_busy_forget()
    def test3():
        root.tk_busy_hold(cursor='gumby')
    def test4():
        root.tk_busy_forget()
    def test5():
        root.tk_busy_hold()
        top.tk_busy(cursor='gumby')
        print(root.tk_busy_current())
        print(root.tk_busy_current(pattern='*t*'))
    def test6():
        print(root.tk_busy_current())
    def test7():
        root.tk_busy_configure(cursor='gumby')
    def test8():
        print(root.tk_busy_configure())
        print(root.tk_busy_configure('cursor'))
        print(root.tk_busy_cget('cursor'))
    def test9():
        print(root.tk_busy_status())
    def test10():
        root.tk_busy_forget()
        print(root.tk_busy_status())
        print(root.tk_busy_current())
    
    delay = 0
    for test in (test1, test2, test3, test4, test5, test6, test7,
                    test8, test9, test10):
        delay += 1000
        root.after(delay, test)
    
    root.mainloop()

    @serhiy-storchaka
    Copy link
    Member

    Could you provide a patch against the default branch? See https://docs.python.org/devguide/ for help. The patch should include not just changes to the tkinter module, but tests for new methods (add new class in Lib/tkinter/test/test_tkinter/test_widgets.py). Unfortunately there is no appropriate place for documenting this feature in the module documentation, but new methods should have docstrings. Would be nice to add an entry in Doc/whatsnew/3.7.rst.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 22, 2016

    Thanks klappnase for your collaboration.

    I dont understand this function:
    def busy_status(self):
    '''Returns the busy status of this window.
    If the window presently can not receive user interactions,
    True is returned, otherwise False.'''
    return((self.tk.getboolean(self.tk.call(
    'tk', 'busy', 'status', self._w)) and True) or False)

    This pattern is not used in other functions that make use of self.tk.getboolean. These functions simply returns the value of self.tk.getboolean directly.

    The code of your function busy_configure() is very similar to Misc._configure(). I think that you are duplicating code. Other functions related to configuration like pack_configure() and place_configure() simply use self._options(). For example:
    def pack_configure(self, cnf={}, **kw):
    self.tk.call(
    ('pack', 'configure', self._w)
    + self._options(cnf, kw))

        def place_configure(self, cnf={}, **kw):
            self.tk.call(
                  ('place', 'configure', self._w)
                  + self._options(cnf, kw))

    I think that my proposal can do the job well. It follows the same pattern than the other functions:
    def tk_busy_configure(self, cnf=None, **kw):
    self.tk.call(('tk', 'busy', 'configure', self._w) + self._options(cnf, kw))

    But I am not totally sure whether it's better to call directly Misc._configure or Misc._options in this situation.

    Also if we follow the naming convention used in tkinter, it seems that we have to define first tk_busy_configure and then make this assignation:
    busy_configure = tk_busy_configure

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 22, 2016

    Misc._configure is only used when the first Tcl command is the name of the widget.

    Very probably my proposal for tk_busy_configure is a better candidate because it follows the conventions used in tkinter (it's similar to pack_configure and place_configure):
    def tk_busy_configure(self, cnf=None, **kw):
    self.tk.call(('tk', 'busy', 'configure', self._w) + self._options(cnf, kw))

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 22, 2016

    @miguel
    About tk_busy_configure():
    I strongly suggest that my version of tk_busy_configure() is better, since it behaves exactly like the usual configure() methods in Tkinter (which includes e.g. Canvas.itemconfigure() or Text.tag_configure()), i.e. you can not only change the values of the configuration options, but also query available options and their default values. I think *this* follows the usual Tkinter conventions.
    As you pointed out in your last post, I could not use _configure() directly for this, but had to "duplicate" the code with a slight modification, because of the order of arguments for tk_busy_configure().
    The reason that this construct is not used for pack_configure() is simply that widget.pack_configure() does not return anything if called without arguments (in fact it appears to be the very same as widget.pack()).

    About busy_status():
    the construct I use here is to work around the broken _getboolean() which does not always return True or False, but often returns None instead of False (if wantobjects==True). Therefore I used (unlike most Tkinter methods) tkapp.getboolean() directly, however quite a while ago I noticed that this was broken, too, and sometimes returned 0 or 1 instead of True or False. However I admit that I did not test right now if this is still the case, in fact I just copy'n'pasted it from code I already use and that works perfectly well (BTW, I suggested the same method to fix these issue with _getboolean() here some time ago, and of course I agree that it would be preferrable to fix _getboolean() and possibly tkapp.getboolean() in the first place).

    @serhiy
    Thanks for the link, I did not find the source tree on the new python web site (I admit I did not look too hard :) .
    I now added a patch against the default branch'es __init__.py . Doc strings are (still :) included. I also changed the function names according to Miguel's suggestion.
    I'll see if I find the time to add a proper test class one of these days (you guys make it really hard for outsiders to help out ;)

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Hi klappnase,
    you are right with the function tk_busy_configure(). Maybe there is a little bit of code duplicated. I think that it's better to directly change the Tkinter function _configure() to make it more general:
    def _configure(self, cmd, cnf, kw):
    """Internal function."""
    if kw:
    cnf = _cnfmerge((cnf, kw))
    elif cnf:
    cnf = _cnfmerge(cnf)
    if cnf is None:
    return self._getconfigure(cmd)
    if isinstance(cnf, str):
    return self._getconfigure1(cmd + ('-'+cnf,))
    self.tk.call(cmd + self._options(cnf))

    I think that it's interesting to do this change because the function is more general and maybe can be used again in the future for new features in Tcl Tk. This way, we avoid code duplication (Dont Repeat Yourself is one of the philosophes of Python). This is the new version of tk_busy_configure using the mentioned change:
    def tk_busy_configure(self, cnf=None, **kw):
    return self._configure(('tk', 'busy', 'configure', self._w), cnf, kw)

    It's more concise.
    I disagree with tk_busy_status(). It's better to be more predictable an return the same than the other methods that uses getboolean(). If there is a bug, it's better to solve that bug. Could you please guive me an code example that replicates the bug?

    The _getboolean function is implemented in _tkinter.c. This is the implementation:
        static PyObject *
        Tkapp_GetBoolean (self, args)
             PyObject *self;
             PyObject *args;
        {
          char *s;
          int v;
      if (!PyArg_Parse (args, "s", &s))
        return NULL;
      if (Tcl_GetBoolean (Tkapp_Interp (self), s, &v) == TCL_ERROR)
        return Tkinter_Error (self);
      return Py_BuildValue ("i", v);
    }
    

    A priori , I can't appreciate any bug in this function. If there is some bug, it's in another place of the C code.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Tcl_GetBoolean() converts a boolean string to an integer 0 or 1:
    https://www.tcl.tk/man/tcl8.6/TclLib/GetInt.htm

    and then Py_BuildValue() converts the integer to a Python object:
    https://docs.python.org/2/c-api/arg.html

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Ok. Maybe the bug is here:

       Misc.getboolean()

    This is the required change:
    def getboolean(self, s):
    """Return a boolean value for Tcl boolean values true and false given as parameter."""
    return bool(self.tk.getboolean(s))

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 23, 2016

    As far as I can see, most internal Tkinter methods use _getboolean(), which currently looks like:

        def _getboolean(self, string):
            """Internal function."""
            if string:
                return self.tk.getboolean(string)

    I am not 100% sure about this, but I figure that when this was written it was intentional that if "string" is an empty string, it should return None instead of (back then) 0 or 1. Today this would probably translate into something like:

        def _getboolean(self, value):
            if not value in ('', None):
                return bool(self.tk.getboolean(value))

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 23, 2016

    Hi Miguel,
    about _configure() :
    the code you suggest:

        def _configure(self, cmd, cnf, kw):
            """Internal function."""
            if kw:
                cnf = _cnfmerge((cnf, kw))
            elif cnf:
                cnf = _cnfmerge(cnf)
            if cnf is None:
                return self._getconfigure(cmd)
            if isinstance(cnf, str):
                return self._getconfigure1(cmd + ('-'+cnf,))
            self.tk.call(cmd + self._options(cnf))

    will break all other methods that use configure() and friends. A possible way to work around this might be:

        def _configure(self, cmd, cnf, kw):
            """Internal function."""
            if kw:
                cnf = _cnfmerge((cnf, kw))
            elif cnf:
                cnf = _cnfmerge(cnf)
            if not self._w in cmd:
                cmd = _flatten((self._w, cmd))
            if cnf is None:
                return self._getconfigure(cmd)
            if isinstance(cnf, str):
                return self._getconfigure1(cmd + ('-'+cnf,))
            self.tk.call(cmd + self._options(cnf))

    Not sure if this is smart, though, it might require some thorough testing.

    About getboolean() :

    it seems like tkapp.getboolean() returns 1 or 0 when it is passed an integer value, at least this is still true here for Python-3.4.2:

    $ python3
    Python 3.4.2 (default, Oct  8 2014, 10:45:20) 
    [GCC 4.9.1] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from tkinter import *
    >>> r=Tk()
    >>> getboolean(1)
    1
    >>> getboolean('1')
    True
    >>> getboolean('yes')
    True
    >>> getboolean(True)
    True
    >>> 

    Probably the best thing to do would be to fix this in the C code. The next best thing I think is to change tkinter.getboolean(), tkinter.Misc.getboolean() and tkinter.Misc._getboolean(). This however leaves a number of Tkinter methods like e.g. Text.compare() which directly use self.tk.getboolean() unresolved.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Yes, sure. It will break code. Maybe it's better to be explicit than implicit (another Python motto). It's also necessary to change configure(). This is the code for my version of _configure() and configure():
    def _configure(self, cmd, cnf, kw):
    """Internal function."""
    if kw:
    cnf = _cnfmerge((cnf, kw))
    elif cnf:
    cnf = _cnfmerge(cnf)
    if cnf is None:
    return self._getconfigure(cmd)
    if isinstance(cnf, str):
    return self._getconfigure1(cmd + ('-'+cnf,))
    self.tk.call(cmd + self._options(cnf))

        # These used to be defined in Widget:
        def configure(self, cnf=None, **kw):
            """Configure resources of a widget.
        The values for resources are specified as keyword
        arguments. To get an overview about
        the allowed keyword arguments call the method keys.
        """
        return self._configure((self._w, 'configure'), cnf, kw)
    

    The semantics of getboolean is clear for me: Transform a true and false value in *Tcl* to an integer value: 1 or 0:

    def getboolean(s):
        """Convert true and false to integer values 1 and 0."""
        return _default_root.tk.getboolean(s)

    I think that the C implementation of getboolean is right.
    The true values for Tcl are: 1, true, yes.
    And the false values are: 0, false, no

    >>> tk.getboolean("true")
    True
    >>> tk.getboolean("false")
    False
    >>> tk.getboolean("0")
    False
    >>> tk.getboolean("1")
    True
    >>> tk.getboolean("yes")
    True
    >>> tk.getboolean("no")
    False

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    It's also necessary in the same way to adapt these functions:
    itemconfigure(), entryconfigure(), image_configure(), tag_configure() and window_configure().

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Your proposal also makes an extra computation: an item (not) belongs to a list

       if not self._w in cmd:
          cmd = _flatten((self._w, cmd))

    Also it's more efficient to use this than the function _flaten() in that situation:

    if not self._w in cmd:
    cmd = (self._w,) + cmd

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 23, 2016

    Your changed _configure() will also break Canvas/Listbox.itemconfigure(), Menu.entryconfigure() and a number of other methods, Tix is also affected. It will also break third party extensions that use _configure(), like pybwidget.
    As another python motto says "Special cases aren't special enough to break the rules." :) I believe that breaking existing code is not justified by the "special case" of the tk_busy_configure() syntax, resp. the desire to avoid 10 extra lines of code.

    The change to _configure() I suggested otoh leaves all the existing configure()-like methods intact, and it seems at least very unlikely that some third party module uses a configure()-like method that adds the window path name to the cmd-tuple (which indeed would break my _configure() example.

    However, following the "explicit is better than implicit" motto, I believe the best idea, if _configure() should be changed at all, is to add a new option to let the programmer decide if the window path should be added to the cmd tuple, which defaults to a value that keeps the old behavior intact, as in this example:

        def _configure(self, cmd, cnf, kw, usewinpath=True):
            """Internal function."""
            if kw:
                cnf = _cnfmerge((cnf, kw))
            elif cnf:
                cnf = _cnfmerge(cnf)
            if usewinpath:
                cmd = _flatten((self._w, cmd))
            else:
                cmd = _flatten(cmd)
            if cnf is None:
                return self._getconfigure(cmd)
            if isinstance(cnf, str):
                return self._getconfigure1(cmd + ('-'+cnf,))
            self.tk.call(cmd + self._options(cnf))

    Then busy_configure might look like:

        def busy_configure(self, cnf=None, **kw):
            return self._configure(('tk', 'busy', 'configure', self._w),
                                    cnf, kw, usewinpath=False)

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 23, 2016

    "Also it's more efficient to use this than the function _flaten() in that situation:

    if not self._w in cmd:
    cmd = (self._w,) + cmd
    "

    The construct with _flatten() was not my invention, it's probably something to discuss with Guido ;)
    The advantage of _flatten() may be, that it will also handle nested tuples properly, though I don't know if there is a real-life situation whee nested tuples might show up there. Anyway, thinking about optimisations on that level seems somewhat pointless to me, no one will probably iterate over configure() calls, and if yes, the bottleneck is clearly the self.tk.call() part.

    @serhiy-storchaka
    Copy link
    Member

    Use "hg diff" command for creating a patch.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    This is my point of view:
    These functions are easy to change:
    itemconfigure(), entryconfigure(), image_configure(), tag_configure() and window_configure()

    It's only to add self._w in the proper place. Only one line per method

    Other third party extensions should not rely on _configure() because it's an internal method (it starts with underscore). We have rights to change the semantics of this internal method in any moment without notification.

    @tkinter tkinter mannequin added topic-installation and removed stdlib Python modules in the Lib dir topic-tkinter labels Oct 23, 2016
    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Hi Serhiy,
    I totally disagree of this change on your patch:

    + def tk_busy_status(self):
    + '''Returns the busy status of this window.
    + If the window presently can not receive user interactions,
    + True is returned, otherwise False.'''
    + return((self.tk.getboolean(self.tk.call(
    + 'tk', 'busy', 'status', self._w)) and True) or False)

    tk_busy_status should return the returned value of self.tk.getboolean directly like other methods in Tkinter using self.tk.getboolean.

    There is no test that shows that self.tk.getboolean is buggy.

    This is the right implementation:
    def tk_busy_status(self):
    '''Returns the busy status of this window.
    If the window presently can not receive user interactions,
    True is returned, otherwise False.'''
    return self.tk.getboolean(self.tk.call('tk', 'busy', 'status', self._w))

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    Hi,
    I think that it's behaving well. Where is the bug here?
    root.tk.getboolean(root.tk_strictMotif())

    getboolean() converts Tcl strings to Boolean Python values according to the definition of True and False in Tcl.

    getboolean is only used for converting strings to boolean Python values. It's undefined the behaviour for other things different than strings.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 23, 2016

    It's not defined the semantics for things different than strings.

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 24, 2016

    The bug is that tk_strictMotif (which uses self.tk.getboolean() itself) returns 0 instead of False. I used the "nested" command to point out, that self.tk.getboolean() is broken when used with wantobjects=True, because it does *not* return proper boolean values , i.e. True or False. It is probably the reduction to handling strings only that you mentioned that causes this wrong behavior, because with wantobjects=True self.tk.call() will convert the "0" tk_strictMotif returns into 0 which is not handled properly. With wantobjects=False, it will retunr True/False as expected. Its behavior is not consistent, *that* is the bug.

    But to be honest, personally I don't give a dime if these calls return True/False or 1/0, I just wanted to make clear why I explicitely converted the output of self.tk.getboolean() in my tk_busy_status function. I believed that proper boolean values (True/False) were the desired thing.
    I personally agree with you about the busy_status function, my construct is not necessary, if someone does not like it to return 0/1 , tkapp.getboolean() should be fixed instead.
    And again I admit that I don't remember why I used that construct instead of just passing the result to bool(). I am quite sure I had a reason for this when I started to use this construct first some years ago, but back then tkapp.getboolean() behaved differently (and worse), some of which was apparently fixed in the meantime.

    And one more time about _configure():

    "I like elegancy..."

    So do I, but can't expanding the functionality of a method by adding a new option be considered a more elegant solution than entirely changing its behavior with the effect of having to change a number of other things, too? (though I admit that the name for this option I picked first should seriously be reconsidered if it's supposed to be called "elegant", I confess that I am notoriously dumb in inventing names :)

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 24, 2016

    In the C source code that I am reading of tkinter: _tkinter.c. It seems that these parameters are not used:

    • wantobjects
    • useTk
    • syn
    • use

    It seems that it's dead code. I hope that somebody can tell me whether I am right. I suppose that now Python returns always Python objects instead of strings. For this reason, wantobjects is not any more necessary. This is the C code of Tkinter_Create that I am reading:

        static PyObject *
        Tkinter_Create (self, args)
             PyObject *self;
             PyObject *args;
        {
          char *screenName = NULL;
          char *baseName = NULL;
          char *className = NULL;
          int interactive = 0;
          baseName = strrchr (Py_GetProgramName (), '/');
          if (baseName != NULL)
            baseName++;
          else
            baseName = Py_GetProgramName ();
          className = "Tk";
          
          if (!PyArg_ParseTuple (args, "|zssi",
                     &screenName, &baseName, &className, &interactive))
            return NULL;
      return (PyObject *) Tkapp_New (screenName, baseName, className, 
                     interactive);
    }
    

    And this is the call to Tkinter_Create in Tkinter.py:
    self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 24, 2016

    At least with python 3.4 here wantobjects still is valid, and personally I really hope that it remains this way, because I use to set wantobjects=False in my own code to avoid having to deal with errors because of some method or other unexpectedly returning TclObjects instead of Python objects (which has been happening here occasionally ever since they were invented).
    I am practically illiterate with C, so I cannot tell what this code does, but at least I believe I see clearly here that it appears to be still used:

    static TkappObject *
    Tkapp_New(const char *screenName, const char *className,
              int interactive, int wantobjects, int wantTk, int sync,
              const char *use)
    {
        TkappObject *v;
        char *argv0;
    
        v = PyObject_New(TkappObject, (PyTypeObject *) Tkapp_Type);
        if (v == NULL)
            return NULL;
        Py_INCREF(Tkapp_Type);
    v-\>interp = Tcl_CreateInterp();
    v-\>wantobjects = wantobjects;
    v-\>threaded = Tcl_GetVar2Ex(v-\>interp, "tcl_platform", "threaded",
                                TCL_GLOBAL_ONLY) != NULL;
    v-\>thread_id = Tcl_GetCurrentThread();
    v-\>dispatching = 0;
    

    (...)

    static PyObject*
    Tkapp_CallResult(TkappObject *self)
    {
        PyObject *res = NULL;
        Tcl_Obj *value = Tcl_GetObjResult(self->interp);
        if(self->wantobjects) {
            /* Not sure whether the IncrRef is necessary, but something
               may overwrite the interpreter result while we are
               converting it. */
            Tcl_IncrRefCount(value);
            res = FromObj((PyObject*)self, value);
            Tcl_DecrRefCount(value);
        } else {
            res = unicodeFromTclObj(value);
        }
        return res;
    }

    @serhiy-storchaka
    Copy link
    Member

    First, thank you Miguel and klappnase for your patches. But they should be provided in different way, as described in Python Developer’s Guide [1].

    I totally disagree of this change on your patch:

    This is not my patch. This is regenerated klappnase's. I expected this will allow to use the Rietveld Code Review Tool for reviewing, but unfortunately Rietveld don't accept these patches [2]. Below I added comments to the patch.

    I agree, that all these complications are not needed. Just use getboolean(). It always returns bool in recent Python.

    And don't bother about _configure(). Just duplicate the code. It can be refactored later, in separate issue.

    Since this is a new feature, it can added only in developing version (future 3.7). Forgot about 2.7 and 3.5. If make the patch fast, there is a chance to get it in 3.6 (if release manager accept this). But tests are needed.

    At least with python 3.4 here wantobjects still is valid, and personally I really hope that it remains this way, because I use to set wantobjects=False in my own code to avoid having to deal with errors because of some method or other unexpectedly returning TclObjects instead of Python objects (which has been happening here occasionally ever since they were invented).

    There was an attempt to deprecate wantobjects=False (bpo-3015), but it is useful for testing and I think third-party program still can use it. If you encounter an error because some standard method return Tcl_Object, please file a bug. This is considered as a bug, and several similar bugs was fixed in last years.

    Here are comments to the last klappnase's patch. Please add a serial number to your next patch for easier referring patches.

    + def tk_busy(self, **kw):

    Shouldn't it be just an alias to tk_busy_hold?

    + '''Queries the busy command configuration options for
    + this window.

    PEP-257: "Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description."

    + any of the values accepted by busy_hold().'''

    PEP-257: "Unless the entire docstring fits on a line, place the closing quotes on a line by themselves."

    + return(self.tk.call('tk', 'busy', 'cget', self._w, '-'+option))

    Don't use parentheses around the return value.

    + the busy cursor can be specified for it by :

    Remove a space before colon, add an empty line after it.

    + def tk_busy_hold(self, **kw):

    Since the only supported option is cursor, just declare it.

        def tk_busy_hold(self, cursor=None):

    + -cursor cursorName

    "-cursor cursorName" is not Python syntax for passing arguments.

    + return((self.tk.getboolean(self.tk.call(
    + 'tk', 'busy', 'status', self._w)) and True) or False)

    Just return the result of self.tk.getboolean().

    [1] https://docs.python.org/devguide/
    [2] http://bugs.python.org/review/28498/

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 24, 2016

    Hi Serhiy,

    thanks for the feedback.

    "+ def tk_busy(self, **kw):

    Shouldn't it be just an alias to tk_busy_hold?"

    Not sure, I figured since it is a separate command in tk I would make it a separate command in Python, too.

    "+ def tk_busy_hold(self, **kw):

    Since the only supported option is cursor, just declare it.

        def tk_busy_hold(self, cursor=None):
    "

    I thought so at first, too, but is this really wise, since if future versions of tk add other options another patch would be required?

    @serhiy-storchaka
    Copy link
    Member

    Shouldn't it be just an alias to tk_busy_hold?"

    Not sure, I figured since it is a separate command in tk I would make it a
    separate command in Python, too.

    place is just an alias to place_configure despites the fact that in Tk "place"
    and "place configure" are different commands. This is Tk sugar, we have Python
    sugar.

    I thought so at first, too, but is this really wise, since if future
    versions of tk add other options another patch would be required?

    Okay, let keep general kwarg.

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 26, 2016

    Ok, I hope I applied all the required changes now.
    The tests seemed to me to belong to test_misc, so I just added another function to the MiscTest class, I hope that is ok. The test runs well here, so I hope I did everything correctly.
    Thanks for your patience!

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 26, 2016

    Oops, I guess I violated PEP-257 again, sorry, corrected version #4 attached.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 26, 2016

    Using the same reasoning applied to tk_busy_hold(), we have to change also these methods:
    selection_set(self, first, last=None) -> selection_set(self, **kw)
    coords(self, value=None) -> coords(self, **kw)
    identify(self, x, y) -> identity(self, **kw)
    delete(self, index1, index2=None) -> delete(self, **kw)
    mark_set(self, markName, index) -> ...
    mark_unset(self, *markNames) -> ...
    ....

    and many other to adapt to future changes.

    @klappnase
    Copy link
    Mannequin

    klappnase mannequin commented Oct 26, 2016

    This is something entirely different, since the commands you list here in tk do not accept keyword arguments at all.

    @tkinter
    Copy link
    Mannequin Author

    tkinter mannequin commented Oct 29, 2016

    Why dont we do the job well from the beginning and refactor _configure() and adapt other dependent code? It's not so complicated to change the dependent code. Many people around the world use Tkinter.

    @serhiy-storchaka
    Copy link
    Member

    Added a number of style comments on Rietveld. I believe klappnase could address them, but since there are too little time to beta3 and I have a weak hope to push this in 3.6, I addressed them myself. Rewritten docstrings and test.

    Ned, is it good to add this feature in 3.6? The patch just adds a couple of new methods, it doesn't affect existing programs.

    @ned-deily
    Copy link
    Member

    With ActiveState 8.6.4.1 (the most recent version available) on macOS:

    ======================================================================
    ERROR: test_tk_busy (tkinter.test.test_tkinter.test_misc.MiscTest)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/tkinter/test/test_tkinter/test_misc.py", line 33, in test_tk_busy
        f.tk_busy_hold(cursor='gumby')
      File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/tkinter/__init__.py", line 849, in tk_busy_hold
        self.tk.call('tk', 'busy', 'hold', self._w, *self._options(kw))
    _tkinter.TclError: unknown option "-cursor"

    @serhiy-storchaka
    Copy link
    Member

    Ah, yes, this feature is not supported on OSX/Aqua.

    @serhiy-storchaka
    Copy link
    Member

    Updated patch skips tests with the cursor option on OSX/Aqua.

    @csabella
    Copy link
    Contributor

    There was a lot of work put into this patch, but I don't think it's been committed yet. Are any of the original authors interested in converting this to a Github pull request on the master branch? Thanks!

    @ned-deily ned-deily added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Feb 22, 2018
    @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 Aug 6, 2023
    Add tkinter.Misc methods: tk_busy_hold(), tk_busy_configure(), tk_busy_cget(),
    tk_busy_forget(), tk_busy_current(), and tk_busy_status().
    @serhiy-storchaka serhiy-storchaka added 3.13 bugs and security fixes and removed 3.8 only security fixes labels Aug 6, 2023
    serhiy-storchaka added a commit that referenced this issue Aug 19, 2023
    …7684)
    
    Add tkinter.Misc methods: tk_busy_hold(), tk_busy_configure(), tk_busy_cget(),
    tk_busy_forget(), tk_busy_current(), and tk_busy_status().
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    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

    No branches or pull requests

    3 participants