bpo-25684: ttk.OptionMenu radiobuttons aren't unique between instance… by csabella · Pull Request #2276 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@@ -276,7 +276,25 @@ def test_menu(self):
self.assertRaises(tkinter.TclError, optmenu['menu'].invoke, -1)
self.assertEqual(optmenu._variable.get(), items[0])

# check that radiobuttons are unique across instances (bpo25684)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this test into separate method?

Please add an entry in Misc/NEWS.d/next/Library/ and move the test into separate method.

@serhiy-storchaka Thank you for the review. I've made the requested changes. It was the first time I've added a NEWS entry though, so hopefully I used blurb correctly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this is merged and backported, I will try to remember to add a note to the S.O. question.

Cheryl, this would be a good issue for learning to use cherry_picker. I believe you can install into the same venv as you used for coverage and blurb.

items[2])

optmenu.destroy()
del textvar2, optmenu_radiobutton_name, optmenu2_radiobutton_name

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. del just unbinds the local names. This happens anyway when the function exits.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. For some reason, even though they were local, I thought that the Tk variables had to be explicitly deleted for refleaks. But, it's probably just when it's in setUp.

optmenu['menu'].invoke(1)
optmenu2['menu'].invoke(2)
optmenu_radiobutton_name = optmenu['menu'].entrycget(0, 'variable')
optmenu2_radiobutton_name = optmenu2['menu'].entrycget(0, 'variable')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume that this is actually the tk name of the StringVar that has the selected value.
If so, optmenu_stringvar_name would seem clearer. With the bug fixed, the name should be 'PY_VAR####' instead of the current ttk default '::selectedButton'.

self.assertEqual(self.root.tk.globalgetvar(optmenu_radiobutton_name),
items[1])
self.assertEqual(self.root.tk.globalgetvar(optmenu2_radiobutton_name),
items[2])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find any doc for globalgetvar other than its use in the Variable subclass get methods. It appear to be a _tkinter function that gets the value of a Variable from its name.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a lot of trouble with this when I was trying to create the test. I had even asked for help on core_workflow. But, after looking through the other tests in this suite, I saw that globalgetvar gave me what I wanted. I ran the test over the old code and it failed (which was good - failed on the old and passed on the new) and I also looked at the values in debug and this was exactly what I needed. I don't know if there was any other way to get that information.

Although I approved, I also suggested a change. The last commit satisfies Serhiy request.

The release candidate for the final bugfix release of 3.5, 3.5.4, is out. Since this is not a security issue, I believe it should not be backported to 3.5