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

Windows file dialog causes Begin/End Mismatch error #6695

Closed
Ena-Shepherd opened this issue Aug 7, 2023 · 12 comments
Closed

Windows file dialog causes Begin/End Mismatch error #6695

Ena-Shepherd opened this issue Aug 7, 2023 · 12 comments

Comments

@Ena-Shepherd
Copy link

Ena-Shepherd commented Aug 7, 2023

Version/Branch of Dear ImGui:

Version: 1.89.5
Branch: shadow

Back-end/Renderer/Compiler/OS

Back-ends: imgui-sfml
Compiler: MinGW32-make
Operating System: Windows

My Issue/Question:

Hello, i'm currently trying to set up a file dialog window using windows api windows.h and commdlg.h to setup an import menu for the user to use json configuration files and load their content in my app.
Opening the dialog window works really well outside of my project, but when i'm adding the same lines to my project, i get an error type Assertion failed: (g.CurrentWindowStack.Size == 1) && "Mismatched Begin/BeginChild vs End/EndChild calls: did you forget to call End/EndChild?". Usually, you get this error when you didn't pay attention to closing/opening properly your windows. This is clearly not the case here.

This is really curious as commenting GetOpenFileName(&ofn) makes my app work well again.
Calling dialog window also causes other access to ressources not to succeed, such as std::ifstream.
I'm also creating child windows dynamically in another part of the program, and opening a file from the dialog seems to freeze the rest of the app.
Assuming it really freezes, the error would make sense.

Screenshots/Video

void openFileDialog(std::string &fileName) {
    // common dialog box structure, setting all fields to 0 is important
    OPENFILENAME ofn = {0};
    TCHAR szFile[9600] = {0};

    ofn.lStructSize = sizeof(ofn);
    ofn.hwndOwner = NULL; // <-- maybe needing HANDLE here ?
    ofn.lpstrFile = szFile;
    ofn.nMaxFile = sizeof(szFile);
    ofn.lpstrFilter = _T("json files\0*.json\0");
    ofn.nFilterIndex = 1;
    ofn.lpstrFileTitle = NULL;
    ofn.nMaxFileTitle = 0;
    ofn.lpstrInitialDir = NULL;
    ofn.Flags = OFN_PATHMUSTEXIST | OFN_FILEMUSTEXIST;

    if (GetOpenFileName(&ofn) == TRUE) {
        std::cout << "file selected : " << ofn.lpstrFile  <<std::endl;
        fileName = ofn.lpstrFile;
    }
}

Another notable point is that i need to compile with ComDlg32.Lib (from WinSDK) as source.

Standalone, minimal, complete and verifiable example: (see #2261)

// Here's some code anyone can copy and paste to reproduce your issue
#include <windows.h>
#include <commdlg.h>
#include <tchar.h>
#include <string>

// openFileDialog definition here...

std::string fileName;
Begin("PresetsMenu");
  if (Button("Import", ImVec2(50, 50))) {
      openFileDialog(fileName);
  }
End();

Thank you for your time and support,
don't hesitate to ask for more info if you need.

@GamingMinds-DanielC
Copy link
Contributor

Modal dialogs like this open up their own message loop for the duration of them being open. That message loop will send messages to your main window as well, so depending on when exactly ImGui code is being handled you can potentially enter into another processing loop while the outer loop is waiting for the dialog to close. Take a look at the call stack during debugging.

The way I handle this case in my code is to request a file dialog and defer its handling to between frames. I give this request a lambda function to be called when it is closed. Maybe this way is feasible for you as well.

@ocornut
Copy link
Owner

ocornut commented Aug 7, 2023

In our typical examples+backend setup, calling e.g. ::GetOpenFileName() in the middle of dear imgui code works fine (I just tried your "verified" example ;), it'll just appear to "freeze" the underlying contents but that's expected.

As mentioned by Daniel you might have some re-entrant logic involved with alter the ImGui flows while the modal native window is open.

@Ena-Shepherd
Copy link
Author

Thank you a lot for your quick answers !
I'll try this solution.

@Ena-Shepherd
Copy link
Author

Update about a working solution

I execute my filedialog as a subprocess with a pipe listening to the path value being cout'ed into stdout after file selection.

Code

subprocess.exe

#include <windows.h>
#include <commdlg.h>
#include <tchar.h>
#include <iostream>

int main(int ac, char** av) {
        // common dialog box structure, setting all fields to 0 is important
        OPENFILENAME ofn = {0};
        TCHAR szFile[260] = {0};

        // Initialize remaining fields of OPENFILENAME structure
        ofn.lStructSize = sizeof(ofn);
        ofn.hwndOwner = NULL;
        ofn.lpstrFile = szFile;
        ofn.nMaxFile = sizeof(szFile);
        ofn.lpstrFilter = _T("Json files\0*.json\0");
        ofn.nFilterIndex = 1;
        ofn.lpstrFileTitle = NULL;
        ofn.nMaxFileTitle = 0;
        ofn.lpstrInitialDir = NULL;
        ofn.Flags = OFN_PATHMUSTEXIST | OFN_FILEMUSTEXIST;

        if (GetOpenFileName(&ofn) == TRUE) {

            std::cout << ofn.lpstrFile << std::endl;
        }
    return 0;
}

import-config.cpp

#include <windows.h>
#include <tchar.h>
#include <iostream>

void openFileDialog() {
    STARTUPINFO si = {0};
    PROCESS_INFORMATION pi = {0};
    SECURITY_ATTRIBUTES sa = {0};
    HANDLE hReadPipe, hWritePipe;

    sa.nLength = sizeof(SECURITY_ATTRIBUTES);
    sa.bInheritHandle = TRUE;
    sa.lpSecurityDescriptor = NULL;

    // Create a pipe for standard output redirection
    if (!CreatePipe(&hReadPipe, &hWritePipe, &sa, 0)) {
        std::cerr << "Error creating pipe." << std::endl;
    }

    si.cb = sizeof(STARTUPINFO);
    si.hStdOutput = hWritePipe;
    si.dwFlags |= STARTF_USESTDHANDLES;

    // Specify the executable path of the subprocess (in this example, "subprocess.exe")
    TCHAR szExePath[] = _T("./subprocess.exe");

    // Create the subprocess
    if (CreateProcess(NULL, szExePath, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi)) {
        std::cout << "Subprocess created successfully!" << std::endl;

        // Close the unused read pipe
        CloseHandle(hWritePipe);

        // Read and display output from the read pipe
        char buffer[4096];
        DWORD bytesRead;
        while (ReadFile(hReadPipe, buffer, sizeof(buffer), &bytesRead, NULL) && bytesRead > 0) {
            std::cout.write(buffer, bytesRead);
        }

        // Close the read pipe
        CloseHandle(hReadPipe);

        // Wait for the subprocess to finish
        WaitForSingleObject(pi.hProcess, INFINITE);

        // Close process and thread handles
        CloseHandle(pi.hProcess);
        CloseHandle(pi.hThread);
    } else {
        std::cerr << "Error creating subprocess." << std::endl;
    }
}

And then, you just need to have something working like this call :

if (Button("Import", ImVec2(50, 50))) {
    openFileDialog();
}

Have a good day 😄

@ocornut
Copy link
Owner

ocornut commented Aug 14, 2023

This seems like an overcomplicated solution but it if works for you...

As stated by our answer, this "just works" in a simple application. You did not provide a "Standalone, minimal, complete and verifiable example". The issue is that your application must have some code for allowing reentrant refresh while modal dialogs are open, and IMHO that is the code you should rework and fix.

@GamingMinds-DanielC
Copy link
Contributor

I don't think blocking the process is a good idea, also you technically don't have an owner for the modal dialog. Sure, this approach is easier to use since you have a seemingly immediate handling, but it might lead to other problems. Well, good luck with it.

@alektron
Copy link
Contributor

alektron commented Apr 22, 2024

I have a somewhat related issue with blocking windows like Window's file open dialog or message box.
Defering to the end of the frame does not really resolve this either.

Imagine the following simple scenario:

if (ImGui::IsKeyPressed(ImGuiKey_O)) {
    MessageBox((HWND)ImGui::GetMainViewport()->PlatformHandle /*Or NULL, same issue*/,
    L"Test", L"My test", MB_ICONERROR);
}

The frame in which the 'O' key is pressed, will open the message box. The release event however gets eaten by the message box so ImGui will keep assuming the O key is pressed 'forever'. In this example this will keep opening the message box. This can be alleviated by passing the repeat = false flag to IsKeyPressed but that does not change the fact that 'O' stays pressed.

The obvious workaround would be to use IsKeyReleased instead, however that is not always possible.
In my case the problem occurs with the classic ´Ctrl + O´ shortcut to open a file open dialog.
Using IsKeyReleased often cause a timing issue due to some users (including me) releasing the 'Ctrl' key before the 'O' key and then being confused why nothing is happening. This case is even worse, since now both the O Release and the Ctrl Release event have been missed.

This may sound a little bit like the old "Events are missed at low framerates" fixed by #4858 problem but AFAIK it is not really related to that. The events can't be trickled down over two frames since as seen from ImGui, the released event never even came.

@ocornut
Copy link
Owner

ocornut commented Apr 22, 2024

The frame in which the 'O' key is pressed, will open the message box. The release event however gets eaten by the message box so ImGui will keep assuming the O key is pressed 'forever'.

Have you tried:

io.ClearInputKeys();
io.SetAppAcceptingEvents(false);
::MessageBox(...);
io.SetAppAcceptingEvents(true);

See #7264

The obvious workaround would be to use IsKeyReleased instead,

That's a XY workaround, you shouldn't consider it.

@alektron
Copy link
Contributor

alektron commented Apr 22, 2024

That seems like a good enough solution for me. A bit unfortunate that it is still something one must always remember to do. I can imagine this issue popping up repeatedly as e.g. new people join our team.
But it works well enough for now. Thank you!

That's a XY workaround, you shouldn't consider it.

I know of the XY problem, but I have not heard of a 'XY workaround' before. You are however absolutely correct.

@ocornut
Copy link
Owner

ocornut commented Apr 22, 2024

I would hope you don't have many calls to ::MessageBox() or ``::GetOpenFileName()` as they are OS-dependent and blocking calls I imagine you'll tend to wrap them.

@alektron
Copy link
Contributor

They are wrapped, yes. But the OS abstraction layer does not have access to ImGui so something like

void OS::MessageBox(...) {
  ImGui::GetIO().ClearInputKeys();
  MessageBoxW(...);
}

is not possible.
We'd have to wrap it again like:

void UI::MessageBox(...) {
  ImGui::GetIO().ClearInputKeys();
  OS::MessageBox(...);
}

and then somehow enforce the usage of UI::MessageBox instead of OS::MessageBox (maybe with more explicit naming, so it is clear that it's not an ImGui message box).
Anyways, that's a minor issue. Knowing about ClearInputKeys/SetAppAcceptingEvents was already helpful enough.

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

No branches or pull requests

5 participants