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

Inconsistency between p_open Functionality and Documentation in BeginPopupModal #6900

Closed
sakiodre opened this issue Oct 5, 2023 · 3 comments

Comments

@sakiodre
Copy link
Contributor

sakiodre commented Oct 5, 2023

Version/Branch of Dear ImGui:

Version: 1.89.9 WIP
Branch: master

My Issue/Question:

The comment for BeginPopupModal, added in commit #f1c75964, mentions that:

popup visibility status is owned by Dear ImGui (and manipulated with e.g. OpenPopup) so the actual value of *p_open is meaningless here.

However, when p_open is set to false when passed to the function, this condition evaluates to false, which returns false and hides the popup. Tracing back through the git history, this if block dates back to #8c790a32 when p_open was first added.

The code clearly indicates that if p_open is false, the popup will not be displayed, contradicting the comment. Could you clarify the intended behavior?

Standalone, minimal, complete and verifiable example:

ImGui::Begin("window");

if (ImGui::Button("open"))
    ImGui::OpenPopup("popup window");

bool p_open = false;

if (ImGui::BeginPopupModal("popup window", &p_open))
{
    ImGui::EndPopup();
}

ImGui::End();
@ocornut ocornut added the popups label Oct 5, 2023
@ocornut
Copy link
Owner

ocornut commented Oct 5, 2023

I think we should preserve the current behavior and amend the comment.

Note however it is inconsistent with Begin() with currently DOES ignore the value of *p_open (I don't recall the reason presently, in believe in theory Begin() could assert to prevent misuse). Either way the current popup behavior is the desirable behavior.

@ocornut ocornut added the doc label Oct 5, 2023
ocornut added a commit that referenced this issue Oct 5, 2023
…set back user value to false when popup is closed in ways other than clicking the close button. (#6900)
ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Oct 5, 2023
@ocornut
Copy link
Owner

ocornut commented Oct 5, 2023

Pushed 1107bff with better docs + an actual fix
Pushed regression test ocornut/imgui_test_engine@a0d32c3

@sakiodre
Copy link
Contributor Author

sakiodre commented Oct 5, 2023

That was fast, thank you!

@sakiodre sakiodre closed this as completed Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants