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

Settings callbacks #993

Closed
wants to merge 1 commit into from
Closed

Conversation

thedmd
Copy link
Contributor

@thedmd thedmd commented Jan 22, 2017

This PR add callbacks for saving and loading INI settings file content.

struct ImGuiIO
{
    ...

    // Optional: save or load configuration.
    void        (*SaveIniCb)(const char* buffer, size_t size);
    size_t      (*LoadIniCb)(char* buffer, size_t size);

Example usage:

    static std::string mySettings;

    ImGuiIO& io = ImGui::GetIO();
    io.SaveIniCb = [](const char* buffer, size_t size)
    {
        mySettings.assign(buffer, size);
    };

    io.LoadIniCb = [](char* buffer, size_t size) -> size_t
    {
        if (buffer)
            memcpy(buffer, mySettings.data(), size);
        return mySettings.size();
    };

Save callback is straightforward, it receive a text buffer and size to save.
Load callback is called twice, first time with buffer set to NULL to determine size, second time with a buffer to read content.

@MrSapps
Copy link

MrSapps commented Jan 23, 2017

"Load callback is called twice, first time with buffer set to NULL to determine size, second time with a buffer to read content."

Then isn't size_t size redundant? Since it must be the same as what was returned the 1st time, else it will cause a bug?

@thedmd
Copy link
Contributor Author

thedmd commented Jan 23, 2017

Value returned describe preferred buffer size and parameter describe actual buffer size. If present it avoid creating implicit assumptions about buffer size and avoid buffer overflow errors. By omitting it code end up less secure prone to errors.

Example implementation of Load() cuts corners, actually should look more like like this:

    io.LoadIniCb = [](char* buffer, size_t size) -> size_t
    {
        if (buffer)
            memcpy(buffer, mySettings.data(), std::min(size, mySettings.size()));
        return mySettings.size();
    };

Rule of thumb is to never make an assumption about someone else allocating buffer big enough when you're then one putting stuff in it.

Edit:
As I think about it. Load() should return number of bytes copied to buffer instead of size of data.

    io.LoadIniCb = [](char* buffer, size_t size) -> size_t
    {
        if (buffer)
        {
            const auto bytes_copied = std::min(size, mySettings.size());
            memcpy(buffer, mySettings.data(), bytes_copied);
            return bytes_copied;
        }
        return mySettings.size();
    };

@thedmd
Copy link
Contributor Author

thedmd commented May 2, 2018

I just want to pin a comment from another issue so I don''t have to look for it next time.
#437

@ocornut ocornut added this to the v1.61 milestone May 2, 2018
@ocornut
Copy link
Owner

ocornut commented May 2, 2018

FYI I think they are slightly different issue, this issue (#993) is 80% done as if you dig in imgui_internal.h you'll find there are functions to load and save from memory. It's been one of my target for 1.61 or maybe 1.62 to get some sort of public API working to allow wiring that nicely. I won't implement actually callbacks but rather 1) let the user load settings themselves using the memory api (which is already possible, the only thing being that it's not a public API), 2) provide some sort of flag to the user to request saving, which the user is free to process whenever they want. So the change from what we have currently are very small, it's mostly settling on the stable API design.

Issue #437 relates to using the .ini file as a general storage facility for the end-user.
Some of the internals have been reworked a few months ago to make it easier for subsystems to register data in there (I'm using it for docking), but a end-user "persistent storage" API could be useful in the future.

@thedmd
Copy link
Contributor Author

thedmd commented May 2, 2018

I pinned your comment because I vaguely remembered reading about changes in imgui_internal and couldn't find it quickly. And it touched topic of storing data by ImGui. : )

I'm looked at ImGuiSettingsHandler, I will use it if I find a need to save custom data. Anyway, internal API looks promising.

As for this PR I think I will close it as soon as public API appears.

Thank you for clarification.

ocornut added a commit that referenced this pull request May 7, 2018
…), SaveIniSettingsToDisk(), SaveIniSettingsToMemory(), io.WantSaveIniSettings. (#923, #993)
@ocornut
Copy link
Owner

ocornut commented May 7, 2018

Closing as solved, see #923 for details.

@ocornut ocornut closed this May 7, 2018
ocornut added a commit that referenced this pull request May 7, 2018
@thedmd thedmd deleted the settings-callbacks branch June 24, 2019 07:10
@ocornut ocornut added the settings .ini persistance label Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
settings .ini persistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants