IDLE: replace DynOptionMenu with a ttk Combobox or OptionMenu

[jfoo]

BPO	27755
Nosy	@terryjreedy, @roseman, @csabella
PRs	bpo-27755: IDLE: Convert configdialog DynOptionMenu to ttk OptionMenu #3215 bpo-27755: replace DynOptionMenu/OptionMenu with ttk.Combobox #7979
Files	issue27755.patch spinbox.png: screenshot of classic and ttk spinbox versions

^{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/terryjreedy'
closed_at = None
created_at = <Date 2016-08-13.19:52:27.674>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'Retire DynOptionMenu with a ttk Combobox'
updated_at = <Date 2018-06-27.22:03:03.630>
user = 'https://bugs.python.org/jfoo'

bugs.python.org fields:

activity = <Date 2018-06-27.22:03:03.630>
actor = 'markroseman'
assignee = 'terry.reedy'
closed = False
closed_date = None
closer = None
components = ['IDLE']
creation = <Date 2016-08-13.19:52:27.674>
creator = 'jfoo'
dependencies = []
files = ['44097', '47652']
hgrepos = []
issue_num = 27755
keywords = ['patch']
message_count = 17.0
messages = ['272609', '272673', '272699', '272757', '272974', '279041', '279043', '279175', '279176', '298091', '298381', '299816', '299820', '299861', '299867', '320459', '320466']
nosy_count = 4.0
nosy_names = ['terry.reedy', 'markroseman', 'jfoo', 'cheryl.sabella']
pr_nums = ['3215', '7979']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27755'
versions = ['Python 3.6', 'Python 3.7']

[jfoo]

One aspect of the IDLE interface that looks extremely old is the the dropdown menu. In the patch, I think I've preserved whatever essential functionality DynOptionMenu used to have, but I'm relatively unfamiliar with Tk so I'm not sure.

[jfoo]

Ah, I've noticed the folly of my ways. This sort of stuff is already being managed with patches filed under various other issues. It just wasn't obvious to me as I wasn't seeing many new improvements in the default branch or much communication on IDLE-dev.

[terryjreedy]

Justin, we are using ttk in 3.6, so the idea is feasible if it works. I do not remember a specific proposal to use ttk Combobox, as it could not be done before now.

Mark, any comments?

[jfoo]

I wasn't sure if the ongoing work in bpo-24781 essentially rendered my patch obsolete, so I keenly await Mark's response.

Upon reflection, I think my patch is a cheap win even if it's later overhauled by other improvements.

[roseman]

Justin, as you say, I think your patch is entirely reasonable as an interim step, as eventually doing a broader improvement on the preferences dialog as suggested in bpo-24781 makes sense.

My reworked version used Combobox in similar ways; I think we can safely do away with the wrapper class and just use the ttk widget directly in the dialog (as the widget already handles the dynamic changes to the list, which the old tk_optionMenu didn't)

[jfoo]

Is bpo-24781 likely to make it into Python 3.6? Otherwise, would this patch be of any benefit in the meantime?

[terryjreedy]

I have not been working on IDLE for the last month, but may get back to it. Patched like this can go in any release, not just .0 version releases. (See PEP-434 for why.)

[terryjreedy]

Your ping got me to look at the doc for tk_optionMenu, which is a function rather than a widget (http://www.tcl.tk/man/tcl8.6/TkCmd/optionMenu.htm), the code for tkinter.OptionMenu (the only accurate doc I know of), which is a widget with the function API, the code for DynOptionMenu(OptionMenu), which is both too general-purpose and too special-cased, the doc (and a bit of code) for ttk.Combobox, and finally your patch. Conclusion: I really want to replace DynOptionMenu with Combobox, but not with the current patch.

Micro-issue: it is too late to add anything to tkinter. In any case, it should be a separate issue, with possible a check for anything else that is missing. IDLE can easily define the constant if needed, though I prefer literal strings myself.

Real issue: no tests. Tests for a new class, if any, can await finalization of the API. However, I want to have automated tests for configdialog before making substantive changes. (Justin, what OS are you working on?)

A plus for the patch is that aside from inessential name changes, it is nearly a drop-in replacement. I could *almost* be persuaded that it would be safe to just do it. However, this is a result of perpetuating what I consider to be an awful API. I will come back to this, but on to the methods.

Enable/disable: I am not a fan of trivial half-line functions. "Cb.enable()" would be the same as "Cb['state'] = 'readonly', Beside this, the cb for font size should perhaps eventually be 'normal' so users can type in a font size. The current patch will not apply cleanly because I since augmented the list with sizes needed for code projection on classroom screens. That itself was a substitute for entry or other adjustmen method.

On_select: I believe "self.variable.set(self.get())" should be redundant, so that '<<ComboboxSelected>>' can be bound directly to command when there is one (currently only one case, but I might want to change this.

Reset: passing a display string should not be options, and IDLE always passes one. 'self.variable.set(selection)' should be redundant. This leaves two lines to set the entry and the option list. For one or two boxes, a wrapper function would not be worthwhile. But for multiple instances, it might be, whether as helper function or wrapper class method.

.__init__: IDLE never passes anything for '*values' and always passes
None for 'value'. This ends up as the default '' for the current value. Both parameters and the corresponding lines can go. So can 'self.enable': the initial state should be added to kwargs. If 'variable' is needed, it should be passed in the normal widget way: 'textvariable=mystringvar'. 'self.set(variable.get())' is redundant because 'variable is initially '' and so it the entry.

But are textvariables needed? Not for setting and getting Combobox entry values. IDLE uses them to trace changes to the current selection. But with Combobox, the change functions could be bound to '<<ComboboxSelected>>' instead of the Var tracer. I think I would prefer this, as the tracer bindings have to be explicitly removed separately from .destroy().

Do I want this binding combined with initialization? Currently the change handler bindings are done in one function. There is a lot of duplication, and I am working to factor some of it out. (But I need tests before applying.) So I may want to leave them together, which means no wrapper class.

Testing a dialog requires two things: simulating user actions and capturing the result. The first depends on the widget, so it makes sense to focus on testing one widget class (like Combobox) rather than, say, one pane with mixed widget types. The second requires a mock that captures the changes sent to the config database when [Ok] or [Apply] are invoked.

[terryjreedy]

I did some experiments that cleared up some previous puzzlements about seemingly inconsistent behavior between interactive commands and those in scripts. The following prints '.mycombo' twice, on two lines, after selecting 'beta'.

import tkinter as tk
from tkinter import ttk
root = tk.Tk()
#root.withdraw()
cb = ttk.Combobox(root, name='mycombo', values=('alpha', 'beta'))
cb.pack()
def here(event): print(event.widget)
cb.bind('<<ComboboxSelected>>', here)
cb.set('none')
root.update()
cb.event_generate('<Button-1>')
root.update()
cb.event_generate('<Key-Down>')
root.update()
cb.event_generate('<Key-Return>')
root.update()
cb.event_generate('<<ComboboxSelected>>')
root.update()

cb.set does *not* trigger ComboboxSelected. Key_Return after Button-1 does. So does directly generating the event.

Uncomment .withdraw() and Key-Return does not trigger. But the directly generating the pseudoevent still works. About a month ago, I added .withdraw to tests to stop (obnoxious) screen flashes. I don't call it for shell interaction. Removing at least some of the .update()s will also stop triggering. In IDLE's interactive mode, tcl update is called 20 times a second, which is at least once per statement types and entered. Hence scripted and interactive statements had different effects.

Directly simulating user actions is more satisfying than generating the pseudoevent, but IDLE tests do not need to verify that Combobox works as advertised -- there is a ttk gui test for that. So I believe set('value') + generate('ComboboxSelected') should be sufficient on the input end (after conversion to Combobox and event binding).