classification
Title: Retire DynOptionMenu with a ttk Combobox
Type: enhancement Stage: test needed
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: csabella, jfoo, markroseman, terry.reedy
Priority: normal Keywords: patch

Created on 2016-08-13 19:52 by jfoo, last changed 2017-08-07 20:34 by csabella.

Files
File name Uploaded Description Edit
issue27755.patch jfoo, 2016-08-13 19:53 review
Messages (15)
msg272609 - (view) Author: Justin Foo (jfoo) * Date: 2016-08-13 19:52
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.
msg272673 - (view) Author: Justin Foo (jfoo) * Date: 2016-08-14 13:56
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.
msg272699 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-08-14 22:28
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?
msg272757 - (view) Author: Justin Foo (jfoo) * Date: 2016-08-15 13:14
I wasn't sure if the ongoing work in #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.
msg272974 - (view) Author: Mark Roseman (markroseman) * Date: 2016-08-17 17:35
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 #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)
msg279041 - (view) Author: Justin Foo (jfoo) * Date: 2016-10-20 15:40
Is #24781 likely to make it into Python 3.6? Otherwise, would this patch be of any benefit in the meantime?
msg279043 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-10-20 16:21
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.)
msg279175 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-10-22 01:19
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.
msg279176 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-10-22 02:00
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).
msg298091 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-10 21:50
I did more experiments with DOM (DynamicOptionMenu) and Combobox (CB).  This was partly to evaluate CB as replacement for DOM for font size; partly to consider CB for the font list.   At least by default, both start closed.  Font list should start open. CB has an Entry area, DOM does not.  They get focus by clicking.  Both open with Down or click on Down button.  DOM also opens on Up, not essential. CB adds scrollbar if needed.  DOM?  Needed for font list.  Up and Down move the selection highlight but do not select.  Selection by click and <Return> close the box and generate <<ComboboxSelect>>  This is unlike Listbox where moving highlight generates <<ListboxSelect>>.  #30870 fixed code so that Up and Down, in addition to click, caused sample change.  I want to keep this for font choice and add it for size choice.  For CB, click on Down button closes without changing selection.  For DOM, it does not, a deficiency.
msg298381 - (view) Author: Justin Foo (jfoo) * Date: 2017-07-15 03:50
Thanks for analysing further, Terry. My original goal was a drop-in replacement for an aspect of IDLE I found quite annoying (on Windows).

Unfortunately, my limited playing with tkinter didn't inspire me to go beyond this.
msg299816 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-08-06 22:45
FYI, I believe that the ttk.OptionMenu is essentially the same as the DynOptionMenu.  ttk.OptionMenu added a `set_menu` which is the same code as DynOptionMenu.SetMenu, except the options are radiobuttons (which marks the current item.)

The only difference is that DynOptionMenu has a highlightthickness, but this isn't directly available on any widgets in ttk. 

One other thing to consider is a Spinbox for the fontsize and maybe for the indentsize.  There is a ttk Spinbox in the ttk documentation, but it doesn't appear to be implemented in Python.

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

Maybe that should be added to ttk?  Or maybe it didn't change from tkinter so that's why it wasn't added?
msg299820 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-08-07 01:16
Yes, if nothing else, we can use ttk.OptionMenu. #24776 is about the font tab specifically, including possibly using a spinbox for font size. #24781 is about Highlights tab.  Whatever we do with theme selection we will probably copy for keyset selection.
msg299861 - (view) Author: Mark Roseman (markroseman) * Date: 2017-08-07 19:05
Cheryl, regarding the spinbox, as per http://www.tkdocs.com/tutorial/morewidgets.html#spinbox, the ttk version appeared in Tk 8.5.9, which might explain it's absence in tkinter.

A wrapper isn't too hard to do of course... e.g. https://stackoverflow.com/questions/30783603/create-new-ttk-widget-from-tkinter
msg299867 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-08-07 20:34
Maybe there should be a bug tracker issue to add it to ttk.py?
History
Date User Action Args
2017-08-07 20:34:55csabellasetmessages: + msg299867
2017-08-07 19:05:35markrosemansetmessages: + msg299861
2017-08-07 01:16:34terry.reedysetmessages: + msg299820
2017-08-06 22:45:20csabellasetnosy: + csabella
messages: + msg299816
2017-07-15 03:50:26jfoosetmessages: + msg298381
2017-07-10 22:05:15terry.reedylinkissue24776 dependencies
2017-07-10 21:50:21terry.reedysetmessages: + msg298091
2016-10-22 02:00:24terry.reedysetmessages: + msg279176
2016-10-22 01:19:52terry.reedysetresolution: duplicate ->
stage: resolved -> test needed
messages: + msg279175
versions: + Python 3.7
2016-10-20 16:21:53terry.reedysetmessages: + msg279043
2016-10-20 15:40:20jfoosetmessages: + msg279041
2016-08-17 17:35:50markrosemansetmessages: + msg272974
2016-08-15 13:14:21jfoosetstatus: closed -> open

messages: + msg272757
2016-08-14 22:28:08terry.reedysetnosy: + markroseman
messages: + msg272699
2016-08-14 14:43:23SilentGhostsetstage: resolved
2016-08-14 13:56:36jfoosetstatus: open -> closed
resolution: duplicate
messages: + msg272673
2016-08-13 19:53:46jfoosetfiles: + issue27755.patch
keywords: + patch
2016-08-13 19:52:27jfoocreate