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

Faulty override of tkinter.Misc.configure in tkinter.ttk.Scale.configure #83333

Closed
GiovaLomba mannequin opened this issue Dec 29, 2019 · 11 comments
Closed

Faulty override of tkinter.Misc.configure in tkinter.ttk.Scale.configure #83333

GiovaLomba mannequin opened this issue Dec 29, 2019 · 11 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-tkinter type-bug An unexpected behavior, bug, or error

Comments

@GiovaLomba
Copy link
Mannequin

GiovaLomba mannequin commented Dec 29, 2019

BPO 39152
Nosy @terryjreedy, @serhiy-storchaka, @miss-islington, @GiovaLomba
PRs
  • bpo-39152: add missing ttk.Scale.configure return value #17815
  • [3.7] bpo-39152: add missing ttk.Scale.configure return value (GH-17815) #17839
  • [3.8] bpo-39152: add missing ttk.Scale.configure return value (GH-17815) #17840
  • 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 = None
    closed_at = <Date 2020-01-05.17:19:13.306>
    created_at = <Date 2019-12-29.13:05:33.887>
    labels = ['3.7', '3.8', 'type-bug', 'expert-tkinter', '3.9']
    title = 'Faulty override of tkinter.Misc.configure in tkinter.ttk.Scale.configure'
    updated_at = <Date 2020-01-05.17:19:13.297>
    user = 'https://github.com/GiovaLomba'

    bugs.python.org fields:

    activity = <Date 2020-01-05.17:19:13.297>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-01-05.17:19:13.306>
    closer = 'terry.reedy'
    components = ['Tkinter']
    creation = <Date 2019-12-29.13:05:33.887>
    creator = 'glombardo'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39152
    keywords = ['patch']
    message_count = 11.0
    messages = ['358987', '359195', '359262', '359263', '359286', '359288', '359289', '359306', '359351', '359353', '359357']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'serhiy.storchaka', 'miss-islington', 'glombardo']
    pr_nums = ['17815', '17839', '17840']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue39152'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @GiovaLomba
    Copy link
    Mannequin Author

    GiovaLomba mannequin commented Dec 29, 2019

    The issue arises by simply calling configure on the Scale widget of the themed tk (ttk) widgets set:

    cursor = scale.configure('cursor')[-1]
    

    The above casues the following error:

    File "C:\Users\Giovanni\Tests\test_scale.py", line 604, in main
        cursor = scale.configure('cursor')[-1]
    File "C:\Users\Giovanni\AppData\Local\Programs\Python\Python37\lib\tkinter\ttk.py", line 1090, in configure
        kw.update(cnf)
    ValueError: dictionary update sequence element #0 has length 1; 2 is required
    

    The interpreter is:

    Python 3.7.4 (tags/v3.7.4:e09359112e, Jul  8 2019, 20:34:20) [MSC v.1916 64 bit (AMD64)] on win32```
    

    @GiovaLomba GiovaLomba mannequin added type-crash A hard crash of the interpreter, possibly with a core dump 3.7 (EOL) end of life topic-tkinter labels Dec 29, 2019
    @GiovaLomba
    Copy link
    Mannequin Author

    GiovaLomba mannequin commented Jan 2, 2020

    I've modified the Scale.configure implementation as follows:

       def configure(self, cnf=None, **kw):
            """Modify or query scale options.
    
            Setting a value for any of the "from", "from_" or "to" options
            generates a <<RangeChanged>> event."""
            if cnf:
                kw.update(cnf)
            retval = Widget.configure(self, **kw)
            if any(['from' in kw, 'from_' in kw, 'to' in kw]):
                self.event_generate('<<RangeChanged>>')
            return retval
    

    It works as expected for my project, but I don't have regression tests or exahustive code checking in place to verify it behaves correctly for all use cases. How can we manage to perform those tests?

    @terryjreedy
    Copy link
    Member

    Your code is buggy because 'cursor' is not a dict or anything like one.

    On the other hand, adding retval to capture and return the return from Widget.configure looks correct. (And it will not prevent kw.update('cursor') from raising. The current code is

        def configure(self, cnf=None, **kw):
            """Modify or query scale options.
        Setting a value for any of the "from", "from_" or "to" options
        generates a \<\<RangeChanged\>\> event."""
        if cnf:
            kw.update(cnf)
        Widget.configure(self, \*\*kw)
        if any(['from' in kw, 'from_' in kw, 'to' in kw]):
            self.event_generate('\<\<RangeChanged\>\>')
    

    )

    To review, test, and make the change, a github Pull Request (PR) is needed. I will make one.

    @terryjreedy
    Copy link
    Member

    Giovanni, a Python exception is a intended exit. A crash is when the program either hangs or stops unintentionally without a python exception but with an OS error message.

    I checked and Scale is the only ttk widget that overrides configure.

    @terryjreedy terryjreedy added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Jan 3, 2020
    @GiovaLomba
    Copy link
    Mannequin Author

    GiovaLomba mannequin commented Jan 4, 2020

    Hello Terry J. Reedy, first and foremost thanks for the time you've dedicated to this issue! I really appreciate you work!! Even if I work with Python since 2007, I'm new here, and not very well versed with this issue management system. This has been my first issue, hopefully not the last.. or maybe yes. Anyway, I'm going to clarify some points about your answers:

    1: scale.configure('whateveroption') can take the name of an option and should return its value, usually a tuple (unless you've already customized the option's value) as all other widgets already do.

    1. The code I've provided is not correct you're right. Mistakenly I've pasted the wrong one, but when I realized it, trying to delete and replace the uncorrect part, I've been unable to find the edit or delete feature on the issue tracker. Sorry for that.

    2. I'd like to thank you for the clarification you've given about correct definition of crash and exception. I knew them. Anyway I'd like point out that there is no option in the 'create ticket' page of the issue tracker to specify that an exception that shouldn't be raised at all is instead raised. Considering that, if a piece of code is not supposed to raise exceptions managing them involve a runtime overhead we all want to avoid. Eventually the raising of the exception may stop an application abnormally leading to the same effect experienced in an application crash.

    3. Exaclty, Scale is the only one widget that override configure. So in theory no other similar issues should be found there.

    It there is some action I need to do, or some documentation I need to read in order to be able to help here please point me there. Thank you again.

    @serhiy-storchaka
    Copy link
    Member

    There is a special case in tests for Scale.configure. It should be removed now. And maybe there is an old issue for this?

    @GiovaLomba
    Copy link
    Mannequin Author

    GiovaLomba mannequin commented Jan 4, 2020

    @serhiy.storchaka I didn't found any old issue not closed aboud tkinter Scale widget.

    @terryjreedy
    Copy link
    Member

    Giovanni, I saw that you gave a correct fix even if your example was wrong. Currently, scale.configure returns None instead of the configuration. Your patch fixes this. Once I created a git branch to verify and test the fix, it was trivial to make a PR, so I did so.

    My PR still needs to patch the ttk.scale test. If you get an idea of what to do, before I do it, post on the PR if you have a github account, or here.

    @terryjreedy
    Copy link
    Member

    New changeset 5ea7bb2 by Terry Jan Reedy in branch 'master':
    bpo-39152: add missing ttk.Scale.configure return value (GH-17815)
    5ea7bb2

    @miss-islington
    Copy link
    Contributor

    New changeset 6234301 by Miss Islington (bot) in branch '3.7':
    bpo-39152: add missing ttk.Scale.configure return value (GH-17815)
    6234301

    @miss-islington
    Copy link
    Contributor

    New changeset 636a850 by Miss Islington (bot) in branch '3.8':
    bpo-39152: add missing ttk.Scale.configure return value (GH-17815)
    636a850

    @terryjreedy terryjreedy added 3.8 only security fixes 3.9 only security fixes labels Jan 5, 2020
    @terryjreedy terryjreedy added 3.8 only security fixes 3.9 only security fixes labels Jan 5, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-tkinter type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants