imgui.get_io().set_ini_filename(None) crashes

[dcnieho]

Title says it all. I changed to imgui.get_io().set_ini_filename('') which works fine, but guess it should error instead of crash?

[dcnieho]

And i still think it would be nice to change

.def_readonly("ini_filename", &ImGuiIO::IniFilename, "= \"imgui.ini\"    // Path to .ini file (important: default \"imgui.ini\" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")
.def("set_ini_filename",
       &ImGuiIO::SetIniFilename, py::arg("filename"))

into

.def_property("ini_filename", &ImGuiIO::IniFilename, &ImGuiIO::SetIniFilename, "= \"imgui.ini\"    // Path to .ini file (important: default \"imgui.ini\" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")

if thats legal, else

.def_property("ini_filename", [](const ImGuiIO &in) {return in.IniFilename;}, &ImGuiIO::SetIniFilename, "= \"imgui.ini\"    // Path to .ini file (important: default \"imgui.ini\" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")

so that users can simply get and set through imgui.get_io().ini_filename

[pthom]

Title says it all. I changed to imgui.get_io().set_ini_filename('') which works fine, but guess it should error instead of crash?

This is not a bug. The signature is:

def set_ini_filename(self, filename: str) -> None:
    pass

It does not expect None as a parameter. It crashes because beneath this it's a bare char* pointer.
Python says "we're all consenting adults" for private members, and C says this for pointer casting.
I will not fix that. How could I? On the C++ side I expect a char* and you send a py::none.
The strange thing is that pybind11 did not refuse the None param: the bug is here.

---

About publishing ini_filename as a readwrite member:

If I publish it as read_write, like this:

        //.def_readonly("ini_filename", &ImGuiIO::IniFilename, "= \"imgui.ini\"    // Path to .ini file (important: default \"imgui.ini\" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")
        .def_readwrite("ini_filename", &ImGuiIO::IniFilename)

Then, bad things happen, for example this python extract:

        ini = imgui.get_io().ini_filename
        imgui.get_io().ini_filename = "ALLO"
        ini2 = imgui.get_io().ini_filename

Will crash like this:

 File sandbox_app.py", line 14, in gui
    ini2 = imgui.get_io().ini_filename
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x88 in position 0: invalid start byte

About publishing ini_filename as a readwrite property with getter and setters:

.def_property("ini_filename", &ImGuiIO::IniFilename, &ImGuiIO::SetIniFilename, "= "imgui.ini" // Path to .ini file (important: default "imgui.ini" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")

It does work at the moment. Changing the generator to add properties is a lot of work, for a small benefit IMHO.

If you really desire this change, please post a suggestion in the discussion section. I'd like to keep the number of unresolved issues to a minimum, before I communicate more about this library.

[dcnieho]

Title says it all. I changed to imgui.get_io().set_ini_filename('') which works fine, but guess it should error instead of crash?

This is not a bug. The signature is:

def set_ini_filename(self, filename: str) -> None:
    pass

It does not expect None as a parameter. It crashes because beneath this it's a bare char* pointer. Python says "we're all consenting adults" for private members, and C says this for pointer casting. I will not fix that. How could I? On the C++ side I expect a char* and you send a py::none. The strange thing is that pybind11 did not refuse the None param: the bug is here.

Hmm, should this be filed with pybind then? My attempt to call should indeed be rejected, i agree with that, but it shouldn't crash.

[dcnieho]

I understand that publishing it as a readwrite member isn't possible with the bare pointer.

About publishing ini_filename as a readwrite property with getter and setters:

.def_property("ini_filename", &ImGuiIO::IniFilename, &ImGuiIO::SetIniFilename, "= "imgui.ini" // Path to .ini file (important: default "imgui.ini" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")

It does work at the moment. Changing the generator to add properties is a lot of work, for a small benefit IMHO.

If you really desire this change, please post a suggestion in the discussion section. I'd like to keep the number of unresolved issues to a minimum, before I communicate more about this library.

This is not high priority, and my suggestion doesn't even solve the need for you to provide a manual implementation of the setter, so its not helpful from that perspective. It only provides the user with a slightly nicer (closer to imgui) and more consistent user interface. I'll post it as a suggestion.

[dcnieho]

I understand that publishing it as a readwrite member isn't possible with the bare pointer.

About publishing ini_filename as a readwrite property with getter and setters:

.def_property("ini_filename", &ImGuiIO::IniFilename, &ImGuiIO::SetIniFilename, "= "imgui.ini" // Path to .ini file (important: default "imgui.ini" is relative to current working dir!). Set None to disable automatic .ini loading/saving or if you want to manually call LoadIniSettingsXXX() / SaveIniSettingsXXX() functions.")

It does work at the moment. Changing the generator to add properties is a lot of work, for a small benefit IMHO.
If you really desire this change, please post a suggestion in the discussion section. I'd like to keep the number of unresolved issues to a minimum, before I communicate more about this library.

This is not high priority, and my suggestion doesn't even solve the need for you to provide a manual implementation of the setter, so its not helpful from that perspective. It only provides the user with a slightly nicer (closer to imgui) and more consistent user interface. I'll post it as a suggestion so its not forgotten, should it be something you want to work on at some point.

[pthom]

I'll close this issue for now. We'll see if we forward this to pybind11 team (but I guess they are well aware of it already)

[dcnieho]

The solution to this one is here.

.def("set_ini_filename",
    &ImGuiIO::SetIniFilename, py::arg("filename").none(false))

makes pybind reject None as an argument. Any raw pointer argument to a function should probably be marked with that.

In this specific case however, there is a problem with this code:

void ImGuiIO::SetIniFilename(const char* filename)
{
    // ImGuiIO::IniFilename is a bare pointer with no storage
    // Let's add a permanent storage when customized by the user
    static char sIniFilename[1024];
    strncpy(sIniFilename, filename, 1024);
    IniFilename = sIniFilename;
}
void ImGuiIO::SetLogFilename(const char* filename)
{
    // ImGuiIO::LogFilename is a bare pointer with no storage
    // Let's add a permanent storage when customized by the user
    static char sLogFilename[1024];
    strncpy(sLogFilename, filename, 1024);
    LogFilename = sLogFilename;
}

This need a check for nullptr filename input, passing that to snprintf is where the crash comes from. Setting the raw pointer field this is wrapping to null is actually explicitly supported. So something like this:

void ImGuiIO::SetIniFilename(const char* filename)
{
    if (!filename)
        IniFilename = nullptr;
    else {
        // ImGuiIO::IniFilename is a bare pointer with no storage
        // Let's add a permanent storage when customized by the user
        static char sIniFilename[1024];
        strncpy(sIniFilename, filename, 1024);
        IniFilename = sIniFilename;
    }
}

would be a fixed implementation. And the same for SetLogFilename, even if the docs don't mention nullptr is supported, the implementation indeed checks for it.