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

IDLE: replace DynOptionMenu with a ttk Combobox or OptionMenu #71942

Open
jfoo mannequin opened this issue Aug 13, 2016 · 18 comments
Open

IDLE: replace DynOptionMenu with a ttk Combobox or OptionMenu #71942

jfoo mannequin opened this issue Aug 13, 2016 · 18 comments
Assignees
Labels
3.7 (EOL) end of life topic-IDLE type-feature A feature request or enhancement

Comments

@jfoo
Copy link
Mannequin

jfoo mannequin commented Aug 13, 2016

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
    Copy link
    Mannequin Author

    jfoo mannequin commented Aug 13, 2016

    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 jfoo mannequin assigned terryjreedy Aug 13, 2016
    @jfoo jfoo mannequin added topic-IDLE type-feature A feature request or enhancement labels Aug 13, 2016
    @jfoo
    Copy link
    Mannequin Author

    jfoo mannequin commented Aug 14, 2016

    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.

    @jfoo jfoo mannequin closed this as completed Aug 14, 2016
    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    jfoo mannequin commented Aug 15, 2016

    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.

    @jfoo jfoo mannequin reopened this Aug 15, 2016
    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 17, 2016

    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
    Copy link
    Mannequin Author

    jfoo mannequin commented Oct 20, 2016

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

    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Member

    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 terryjreedy added the 3.7 (EOL) end of life label Oct 22, 2016
    @terryjreedy
    Copy link
    Member

    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).

    @terryjreedy
    Copy link
    Member

    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>>. bpo-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.

    @jfoo
    Copy link
    Mannequin Author

    jfoo mannequin commented Jul 15, 2017

    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.

    @csabella
    Copy link
    Contributor

    csabella commented Aug 6, 2017

    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?

    @terryjreedy
    Copy link
    Member

    Yes, if nothing else, we can use ttk.OptionMenu. bpo-24776 is about the font tab specifically, including possibly using a spinbox for font size. bpo-24781 is about Highlights tab. Whatever we do with theme selection we will probably copy for keyset selection.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 7, 2017

    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

    @csabella
    Copy link
    Contributor

    csabella commented Aug 7, 2017

    Maybe there should be a bug tracker issue to add it to ttk.py?

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Jun 26, 2018

    Given the difference between the old and new (ttk) spinbox, especially on macOS, I'd like to incorporate it into IDLE when available. See screenshot spinbox.png, noting white border around old one.

    Terry, can we add a spinbox wrapper to IDLE for the time being? If so, would you prefer it done as its own (very little) module? Or is there any value to adding a module to hold various small UI wrappers and convenience procs?

    @terryjreedy
    Copy link
    Member

    Yes, we should use ttk.Spinbox: bpo-33962.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @terryjreedy terryjreedy changed the title Retire DynOptionMenu with a ttk Combobox IDLE: replace DynOptionMenu with a ttk Combobox or OptionMenu Jan 27, 2023
    @terryjreedy
    Copy link
    Member

    All current uses of DynOptionMenu are in configdialog. The only use of 'highlightthickness' is for the color-select target list (normal text, etc). I cannot see that it makes much of any difference, as the instances are left out of tab navigation. (And Enter does not select highlighted items anyway.) The only other kwargs passed for other instances (default and custom lists for highlights and keysets) are command=None, which I presume is default.

    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 topic-IDLE type-feature A feature request or enhancement
    Projects
    Status: In Progress
    Development

    No branches or pull requests

    2 participants