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

Modernizing EasyGUI's documentation and verifying it works with Python 3.5 to 3.9. #179

Merged
merged 19 commits into from
Jan 27, 2021

Conversation

asweigart
Copy link
Contributor

I've made several documentation updates, a few bug fixes, and verified EasyGUI still works with modern Python versions. I haven't made any backwards-incompatible changes. I've also added a couple of basic unit tests as well.

…default buttons from Yes/No to True/False, since ynbox() already has Yes/No default buttons.
…aises a ValueError instead of displaying the cryptic "Add more items" text to the displayed list. Getting rid of the mutable blank lists as default arguments (even though later code prevents bugs) and adding ['Choice 1', 'Choice 2'] defaults. This commit also removes trailing whitespace.
…should they be), so we can remove this code.
… docstrings for fileopenbox() and filesavebox() should be escaped since \* is not a valid escape sequence. Also, the regex string in button_box.py was not a raw string.
…ialog box based on them) in focus when they appear. This is only a half fix; there seem to be some cases where the dialog box isn't put in focus. I'll file an issue about the "topmost" attribute and how it fails mysteriously.
…he dialog boxes. This initial test file tests msgbox() and buttonbox(). More tests should follow.
…uns on Python 3.5, 3.6, 3.7, 3.8, and 3.9. Hooray!
@jjdenis
Copy link
Collaborator

jjdenis commented Jan 20, 2021 via email

Copy link
Contributor

@spyoungtech spyoungtech left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Looks good. Have a few comments/suggestions if you care to take a look.

easygui/boxes/derived_boxes.py Show resolved Hide resolved
@@ -0,0 +1,2 @@
from .boxes.demo import easygui_demo
easygui_demo()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strongly opinionated, but maybe consider shooting for an interface like python -m easygui.demo (for example by creating a demo.py) instead of using the toplevel namespace.

Thought: Maybe in the feature easygui benefits from a bonafide CLI that works something like python -m eaygui msgbox --title="Future" --msg="Hello Future"

Copy link
Contributor Author

@asweigart asweigart Jan 20, 2021

Choose a reason for hiding this comment

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

There's already the easygui.py script that launches the demo. I'm just adding this so that we have the more modern python -m format as well. We can file a new issue for the command line form (I like that idea but don't have time to work on it).

@@ -396,6 +396,7 @@ def configure_root(self, title):
self.boxRoot.protocol('WM_DELETE_WINDOW', self.x_pressed)
self.boxRoot.bind("<Escape>", self.cancel_pressed)
self.boxRoot.iconname('Dialog')
self.boxRoot.attributes("-topmost", True) # Put the dialog box in focus.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some thoughts:

  • Are we sure this is always a desired behavior?
  • Could it break someone's software by introducing this? (I think it could)
  • Should we increment the minor version based on this change?
  • Consider making this configurable and/or opt-in

(Same thoughts apply to other places this change is added, but I'll leave just this comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it is always the desired behavior. It's the behavior for some of the dialogs already with no apparent reason as to which does and which doesn't (I think it's whenever you have a preselected button, the behavior puts it on top and in focus). EasyGUI is about dialog boxes rather than a general GUI window toolkit, so the "display and put it in focus" should be the intended behavior. I think the dialog boxes that didn't do that were an oversight.

choices = [str(c) for c in choices]

while len(choices) < 2:
choices.append("Add more choices")
raise ValueError('at least two choices need to be specified')
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strongly opinionated here, but wouldn't it be perfectly viable to have choices be a list of length 1?

For example, if you derive your choices dynamically and, in some cases, you might only have one choice.

I would consider raising an error if there are no choices... but even then, perhaps it could be perfectly OK to have no choices as well 🤔

See also: #149

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree with the idea that if there are zero or one choices to "choose" from, then that probably indicates a bug and raising an error here would highlight it. I just thought the "Add more choices" choice additions did more to hide the problem than anything, so I went with the existing "at least 2 choices" behavior.

easygui/boxes/derived_boxes.py Outdated Show resolved Hide resolved
Comment on lines -45 to -53
#. Can I specify a custom sort order for the items in a choicebox?

No, there is no way to specify a custom order. The reason is that
the data must be sorted in order for the "jump to" feature
(namely, to jump down in the list by pressing keyboard keys) to work.

For some tips on how to control the order of items in a choicebox,
see this recipe from the Easygui Cookbook.

Copy link
Contributor

@spyoungtech spyoungtech Jan 20, 2021

Choose a reason for hiding this comment

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

Was this just wrong to begin with? If there is a behavior change, it would be good to just amend this answer to say something like

changed in easygui 0.98.2: choices are now displayed in the order they are provided

Keep in mind folks running older versions may still have this question (if it's something changed, not sure if it is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The sorting thing was never necessary, and the workaround isn't needed. I think the confusion could have come from the fact that clicking a choice highlights it, but the previously selected choice is still underlined, and using the arrow keys selects a new choice above or below the underlined choice instead of the highlighted one.

Or maybe it was some other issue? Either way, I couldn't seem to recreate the problem this workaround was supposed to be for. And the sorting behavior was all dead code at this point anyway, so I took the code and the documentation for it out.

sphinx/tutorial.rst Outdated Show resolved Hide resolved
Comment on lines +28 to +29
This is a bit unorthodox, and I'm welcome to other suggestions about how to
deal with this possible scenario.
Copy link
Contributor

@spyoungtech spyoungtech Jan 20, 2021

Choose a reason for hiding this comment

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

You could use tkinter's .after callback to ensure the dialog is closed.

def test_pressing_escape_closes():
    t = KeyPresses([Key.esc])
    t.start()
    box = easygui.buttonbox(run=False)
    def killbox():
        # if pynput failed, makes sure the dialog is closed to try to prevent deadlock
        try:
            box.ui.boxRoot.destroy()
        except:
            pass
        pytest.fail("Timed out waiting on pynput")
    box.ui.boxRoot.after(int(GUI_WAIT * 1000 + 2000), killbox)
    assert box.run() is None

One problem is that msgbox doesn't accept run=False. But it could be patched in using unittest.mock (or the python2 backport mock)

    def unrun_wrapper(f):
        """
        A patched version of buttonbox that always provides run=False

        Since ``msgbox`` doesn't have a ``run`` parameter, we patch ``buttonbox`` instead

        :param f:
        :return:
        """

        @wraps(f)
        def unrun(*args, **kwargs):
            kwargs['run'] = False
            return f(*args, **kwargs)

        return unrun
    patchedbuttonbox = mock.Mock(wraps=unrun_wrapper(easygui.buttonbox))
    with mock.patch('easygui.boxes.derived_boxes.buttonbox', patchedbuttonbox):
        box = easygui.msgbox(*args, **kwargs)
    def killbox():
        # if pynput failed, makes sure the dialog is closed to try to prevent deadlock
        try:
            box.ui.boxRoot.destroy()
        except:
            pass
        pytest.fail("Timed out waiting on pynput")
    box.ui.boxRoot.after(int(GUI_WAIT * 1000 + 2000), killbox)
    assert box.run() == expected

This works, tested on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to any changes to the unit tests.

test_cases/test_easygui.py Outdated Show resolved Hide resolved
Comment on lines 95 to 124
def test_buttonbox():
# Test hitting space to click OK with different default buttons:
t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[1]') == 'Button[1]'

t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[2]') == 'Button[2]'

t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[3]') == 'Button[3]'

# Test hitting Esc to close.
# TODO: If button boxes aren't given a default choice, then their window won't be in focus and this test hangs.
#t = KeyPresses([Key.esc])
#t.start()
#print('Line', inspect.currentframe().f_lineno)
#assert easygui.buttonbox() is None

# Test hitting Esc to close.
t = KeyPresses([Key.esc])
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox(default_choice='Button[1]') is None

Copy link
Contributor

@spyoungtech spyoungtech Jan 20, 2021

Choose a reason for hiding this comment

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

Same idea here... also marked the TODO item test as an xfail instead of commenting it out.... although the test actually passes normally for me on MacOS.

Suggested change
def test_buttonbox():
# Test hitting space to click OK with different default buttons:
t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[1]') == 'Button[1]'
t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[2]') == 'Button[2]'
t = KeyPresses(' ')
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=('Button[1]', 'Button[2]', 'Button[3]'), default_choice='Button[3]') == 'Button[3]'
# Test hitting Esc to close.
# TODO: If button boxes aren't given a default choice, then their window won't be in focus and this test hangs.
#t = KeyPresses([Key.esc])
#t.start()
#print('Line', inspect.currentframe().f_lineno)
#assert easygui.buttonbox() is None
# Test hitting Esc to close.
t = KeyPresses([Key.esc])
t.start()
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox(default_choice='Button[1]') is None
@pytest.mark.parametrize('position', (0, 1, 2))
def test_default_choice_is_selected(position):
# Test hitting space to click OK with different default buttons:
t = KeyPresses(' ')
t.start()
choices = ('Button[1]', 'Button[2]', 'Button[3]')
expected = choices[position]
print('Line', inspect.currentframe().f_lineno)
assert easygui.buttonbox('Message', 'Title', choices=choices, default_choice=expected) == expected
# TODO: If button boxes aren't given a default choice, then their window won't be in focus and this test hangs.
@pytest.mark.xfail
def test_hitting_escape_presses_close():
t = KeyPresses([Key.esc])
t.start()
box = easygui.buttonbox(run=False)
def killbox():
# if pynput failed, makes sure the dialog is closed to try to prevent deadlock
try:
box.ui.boxRoot.destroy()
except:
pass
box.ui.boxRoot.after(int(GUI_WAIT * 1000 + 2000), killbox)
assert box.run() is None
def test_hitting_esc_closes_with_default_choice():
# Test hitting Esc to close.
t = KeyPresses([Key.esc])
t.start()
assert easygui.buttonbox(default_choice='Button[1]') is None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.

asweigart and others added 5 commits January 20, 2021 14:23
@robertlugg robertlugg merged commit cd3cf7b into robertlugg:master Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants