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

Finish IDLE Query dialog appearance and behavior. #71808

Open
serhiy-storchaka opened this issue Jul 26, 2016 · 25 comments
Open

Finish IDLE Query dialog appearance and behavior. #71808

serhiy-storchaka opened this issue Jul 26, 2016 · 25 comments
Assignees
Labels
3.10 only security fixes topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 27621
Nosy @terryjreedy, @stevendaprano, @roseman, @serhiy-storchaka
Files
  • idle_query_press_return.patch
  • dlgonmac.png: snapshot of open module dialog running on mac showing background glitches
  • query.patch
  • query2.diff: put error messages in query widget
  • 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-07-26.06:02:46.721>
    labels = ['expert-IDLE', 'type-bug', '3.10']
    title = 'Finish IDLE Query dialog appearance and behavior.'
    updated_at = <Date 2020-06-08.00:56:20.579>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2020-06-08.00:56:20.579>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = False
    closed_date = None
    closer = None
    components = ['IDLE']
    creation = <Date 2016-07-26.06:02:46.721>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['43891', '43940', '43966', '43998']
    hgrepos = []
    issue_num = 27621
    keywords = ['patch']
    message_count = 25.0
    messages = ['271326', '271341', '271342', '271344', '271358', '271360', '271464', '271481', '271482', '271652', '271663', '271753', '271756', '271757', '271761', '271789', '271870', '271930', '271937', '272073', '272220', '272221', '272341', '272342', '297337']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'steven.daprano', 'markroseman', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'test needed'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27621'
    versions = ['Python 3.10']

    @serhiy-storchaka
    Copy link
    Member Author

    When open new Query dialog in IDLE (e.g. "Open Module" dialog), move a focus on the Cancel button and press <Return> on keyboard, the OK button is invoked instead of Cancel.

    @serhiy-storchaka serhiy-storchaka added topic-IDLE type-bug An unexpected behavior, bug, or error labels Jul 26, 2016
    @terryjreedy
    Copy link
    Member

    While writing bpo-27620 and adding a binding to Query for <Escape>, a few hours ago, I realized that this would be the case, that it could be considered a bug, and that the fix will be a 'return' function that looks at the focus before invoking [Ok] (the default) or [Cancel] (when it has the focus).

        def return_key(self, event):
            if self.focus_get() == self.button_cancel:
                self.cancel()
            else:
                self.ok()

    appears to work. I will push tomorrow after writing a test. Something similar is needed for config dialog. I just tested the search dialog and it behaves the same as query does now. If one tabs enough to put the focus on close, <Return> does find anyway.

    Do you agree that Esc should always cancel regardless of where the focus is? I would appreciate your input on bpo-27620 as to the 'right' behavior we should aim at for the different dialogs.

    While I think of it, the indication of ttk.Entry focus with the default Windows theme is a shift of the 1 pixel black border to a 1 pixel blue border. It is not obvious. I want to look as other Window's theme or possibly a custom style. Now a priority yet.

    @terryjreedy
    Copy link
    Member

    I made this a dependency of bpo-27620.

    @serhiy-storchaka
    Copy link
    Member Author

    I have wrote similar patch. There are many ways to fix this issue. Tk code for dialog widgets is more complex and general.

    @stevendaprano
    Copy link
    Member

    I expect that ESC should always cancel, and RETURN/ENTER should always accepts (OK or Save or whatever the "main" button is) regardless of where the focus is. If you want a keyboard shortcut to push the button with focus, use SPACE, not ENTER.

    @serhiy-storchaka
    Copy link
    Member Author

    This is against the convention.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Jul 27, 2016

    Side note that on Mac OS X, buttons normally don't get the focus, so that this isn't an issue. Well except that buttons are getting the focus here. :-)

    Also since we're reinventing the wheel, please note that alternative keyboard shortcuts (e.g. command-period) don't work, and there isn't a default button set for the dialog which there should be.

    Interestingly, you can change things in System Preferences -> Keyboard -> Shortcuts so that buttons can get the focus via tabbing through the interface. If this is enabled and you tab to the Cancel button and his Return, it should still be treated as if you hit the Okay button on Mac. Hitting space while focus is on the Cancel button does treat it as if you clicked on Cancel.

    @terryjreedy
    Copy link
    Member

    I am not trying to re-invent the wheel. I am trying to bring IDLE up to uniform standards. In particular, make all dialogs usable from the keyboard. The config dialog is the worst. See bpo-27620, the master issue.

    For a two button messagebox, I went back to 3.5.2, which still uses the commondialog box. It has the default button marked as such. <Return> always does the default action. <Escape> always cancels. <Tab> moves the focus, but does not affect the above. <Space> enters ' ' in text entry and 'clicks' buttons. So this is at least 'a' Windows standard, as well as 'a' Mac standard.

    The search dialogs act the same. The space behavior built-in to the TK widgets, at least on Windows. I presume this is a class binding. The dialogs have no platform specific code, so they are also the defacto IDLE standard.

    Serhiy, it is definitely a tk convention, and I presume more universal than that, that the 'default ring' always indicate what button/action is invoked by <Return>. If the default moved with the focus, the default ring should move also. I am not inclined to do that without a strong reason.

    The button default ring is controlled by the 'default' option" 'active' = visible, 'normal' = possible, 'disabled' = not possible. (From the tk docs. The NMT reference is useless here.) I will add "default='active'" for the Ok button.

    (Mark: "default=1", suggested on http://www.tkdocs.com/widgets/button.html, is a nasty bug. "_tkinter.TclError: bad default "1": must be normal, active, or disabled".)

    @terryjreedy terryjreedy changed the title <Return> incorrectly works in IDLE Query dialogs Finish IDLE Query dialog appearance and behavior. Jul 28, 2016
    @terryjreedy
    Copy link
    Member

    I meant to direct the focus ring fix here instead bpo-27620. To me, this issue is done, at least for now.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Jul 29, 2016

    Terry, thanks for the TkDocs correction.

    As you'll note from the attached dlgonmac.png, there's a bit of tweaking needed with regard to geometry management etc. to get the background right.

    Now that ttk is ok (so to speak), would you be open to some patches that fix this up, a bit more akin to what you see in the 'query dialog' subsection if you scroll down a bit in http://www.tkdocs.com/tutorial/idle.html#idledialogs

    Separately, would you be open to a patch changing things to use the "inline" error handling illustrated on the goto line dialog on that page (i.e. showing error message in query dialog in red vs. popping up an alert)?

    Older code for that can be found here btw: https://github.com/roseman/idle/blob/master/querydialog.py

    @serhiy-storchaka
    Copy link
    Member Author

    In Tk widgets tk_dialog and tk_messageBox (corresponding Tkinter widgets dialog.Dialog and messagebox.Message) pressing Return invokes focused button. The comment in tk_dialog implementation:

    # 4. Create a binding for <Return> on the dialog if there is a
    # default button.
    # Convention also dictates that if the keyboard focus moves among the
    # the buttons that the <Return> binding affects the button with the focus.
    

    I expected the same behavior in IDLE Query dialog. If this behavior considered outdated, I'm nice to close this issue.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Jul 31, 2016

    Serhiy, the tk_dialog has been superseded by tk_messageBox, and does not reflect current platform standards. I just tried tk_messageBox on the Mac, which always activates the default button if you press 'return', even if another button has the focus. I expect Windows and X11 are different, and will check that out when I get a chance.

    @terryjreedy
    Copy link
    Member

    Background: For Open Module, the old commondialog did not have the asymmetrical drop shadow. That is an unintentional addition (from a frame?) and I don't like it. Perhaps we should use grid instead. Rows 0 and 1 for the initial label and entry, with columnspan 2, rows 8 and 9 for error message and buttons. This leaves room for insertion of path message, path entry, and browse button. Patch welcome.

    A definite yes for error label in widget. I dislike the popup. Begone! Should a blank label reserve space or should it be inserted as needed, making box expand.

    @terryjreedy
    Copy link
    Member

    Serhiy: I believe Notepad, Wordpad, Libre Office Write behave as you describe. But there are differences in details. So I don't consider issue closed, but it is low priority to me.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 1, 2016

    Just to follow up, both Windows and Linux the 'correct' behaviour seems to be that space or return activates the button with the current focus. Mac behaves differently in that return key always activates default button even if focus is on another button (and normally on Mac, buttons don't get focus).

    I'll put together a patch that cleans up the layout and does the error label in dialog thing.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 1, 2016

    I've attached query.patch, which does the cosmetic and layout changes, and adds a couple Mac-specific things. I've added the inline error message widget but don't use it yet (as this will involve changes to the subclasses and the tests, given errors will show up when the dialog is running, not after).

    Given I'm a bit rusty at this, would appreciate if someone could check this out and make sure I did things correctly. :-)

    @terryjreedy
    Copy link
    Member

    The file has the correct format for review, applies cleanly, and tests pass. The relief is gone (easy). The buttons are no longer symmetrical; this looks 'wrong' to me. (My reaction might be different if the widget was much wider, so the buttons were clearly right-justified rather than looking like their placement had a bug.) This results from using 3 columns instead of 2. I want to try the latter. I will switch error messages to the widget, see what happens, and deal with needed test changes. I will post before committing so you can test on Mac first.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 3, 2016

    Thanks Terry! I'd be good if you want to put a width back on the buttons, but I'd suggest "width=6" rather than using 8 as it was before.

    @terryjreedy
    Copy link
    Member

    I changed showerror from widget to query function, added a second error label to Help Source, adjusted validation functions, and rewrote tests. I also made keypad enter work on all systems (it already works in Search and I use it often). I want to look at spacing in a separate pass.

    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Aug 5, 2016

    Looks great Terry - thanks. Only nit is that test_click_help_source fails on Mac, courtesy a leading 'file://' added in the last few lines of path_ok

    @terryjreedy
    Copy link
    Member

    See bpo-27714 for more on the test failure.

    @terryjreedy
    Copy link
    Member

    Actually, the query test failure is posted to bpo-27380

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 10, 2016

    New changeset f0e86b60de5f by Terry Jan Reedy in branch 'default':
    Issue bpo-27621: Put query response validation error messages in query box
    https://hg.python.org/cpython/rev/f0e86b60de5f

    @terryjreedy
    Copy link
    Member

    There is still Serhiy's original issue about moving <Return> default with focus, plus possible appearance changes.

    @terryjreedy
    Copy link
    Member

    bpo-24812 specifically talks about Mac standards.

    @terryjreedy terryjreedy added the 3.10 only security fixes label Jun 8, 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.10 only security fixes topic-IDLE type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    3 participants