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

display.message_box() #2427

Merged
merged 25 commits into from
Oct 15, 2023
Merged

display.message_box() #2427

merged 25 commits into from
Oct 15, 2023

Conversation

yunline
Copy link
Contributor

@yunline yunline commented Aug 29, 2023

As a replacement of video.messagebox().

Examples:

pygame.display.message_box("Hello")

图片

pygame.display.message_box(
    "Hello",
    "Hello pygame-ce",
    type = "warn"
)

图片

result = pygame.display.message_box(
    "Error",
    "V我50",
    type="error",
    buttons=("Wednesday", "Thursday", "Friday"),
    return_button = 1,
    escape_button = None
)

print(result)

图片

@yunline yunline added New API This pull request may need extra debate as it adds a new class or function to pygame display pygame.display labels Aug 29, 2023
@yunline yunline marked this pull request as ready for review August 29, 2023 13:31
@yunline yunline requested a review from a team as a code owner August 29, 2023 13:31
docs/reST/ref/display.rst Outdated Show resolved Hide resolved
@MyreMylar
Copy link
Member

Should we take the opportunity of this function moving modules (from a dead module) to rename it from messagebox() to message_box() which is more in the PEP8 style?

@yunline yunline changed the title display.messagebox() display.message_box() Sep 4, 2023
@Matiiss
Copy link
Member

Matiiss commented Sep 5, 2023

Should we have some constants for the modes too?
For example
pygame.MESSAGE_BOX_INFO or pygame.display.MESSAGE_BOX_INFO (or MESSAGEBOX_...) or something of the like that would simply represent those strings.

src_c/display.c Outdated Show resolved Hide resolved
src_c/display.c Outdated Show resolved Hide resolved
@zoldalma999
Copy link
Member

Should this be added in system instead of display? I think it would make sense there, especially since SDL3 will (probably) have support for (save, open file, open folder) file dialogs as well.

@Starbuck5
Copy link
Member

Should this be added in system instead of display? I think it would make sense there, especially since SDL3 will (probably) have support for (save, open file, open folder) file dialogs as well.

I've supported having this in display in the past. Good point about SDL file dialogs though (libsdl-org/SDL#7445). They have a full implementation waiting in a PR.

In the SDL implementation, the messagebox and file dialog systems are very separate: different API styles, different modules. Hence it will be okay for us to have messagebox in pygame.display and file dialogs in pygame.system.

src_c/display.c Outdated Show resolved Hide resolved
docs/reST/ref/display.rst Outdated Show resolved Hide resolved
test/message_box_test.py Outdated Show resolved Hide resolved
src_c/display.c Outdated Show resolved Hide resolved
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Alright, lets pull this in. I couldn't find anything wrong in testing.

Thanks for moving this forward!

Eventually it would be nice to have _sdl2.video.messagebox be a compat shim over this API, but that's for another pull request.

docs/reST/ref/display.rst Outdated Show resolved Hide resolved
src_c/display.c Show resolved Hide resolved
src_c/display.c Show resolved Hide resolved
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

Left two reviews that are nice-to-haves

docs/reST/ref/display.rst Show resolved Hide resolved
test/display_test.py Show resolved Hide resolved
@Starbuck5
Copy link
Member

@yunline I'm not sure why circleci is failing for you, maybe a rebase against main would fix it?

This looks ready to merge, I left a tiny wording note and this needs to be squashed down to fewer commits before merging.

@MyreMylar MyreMylar merged commit a917d3f into pygame-community:main Oct 15, 2023
30 checks passed
@yunline yunline deleted the c-msgbox branch October 17, 2023 05:20
@pmp-p
Copy link
Member

pmp-p commented Nov 22, 2023

That seems like a blocking function, it is really necessary to add things that cannot be ported on some platforms ?

i mean was really video.messagebox used anyway ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking display pygame.display New API This pull request may need extra debate as it adds a new class or function to pygame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants