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

New API to set each property of the dialog separately #92

Closed
btzy opened this issue Apr 9, 2023 · 7 comments · Fixed by #128
Closed

New API to set each property of the dialog separately #92

btzy opened this issue Apr 9, 2023 · 7 comments · Fixed by #128
Milestone

Comments

@btzy
Copy link
Owner

btzy commented Apr 9, 2023

EDIT: A versioned struct will be used instead of this, implemented in #128.

The current API looks like:

nfdresult_t res = NFD_OpenDialogN(outPath, filterList, filterCount, "/path/to/default/folder");

This interface is inextensible. It is impossible to add new parameters without breaking API and ABI, so it is not possible to provide some improvements in a non-breaking manner. They include:

Adding a new API that sets each property separately will allow support for additional parameters, and also make it more future-proof, like the following:

NFD_OpenFileDialog* dialog = NFD_OpenFileDialog_Create();
NFD_OpenFileDialog_SetFilterList(dialog, filterList, filterCount);
NFD_OpenFileDialog_SetDefaultFolder(dialog, "/path/to/default/folder");
nfdresult_t res = NFD_OpenFileDialog_Show(dialog, outPath);

I'm not decided on this and am not sure that this is the best way yet - feel free to leave comments.

The current API will be retained, and it will call the new API under the hood.

@krupkat
Copy link

krupkat commented Apr 10, 2023

You probably thought about this, but there is also the option of renaming the new extended function, e.g. to NFD_OpenDialogN_v2 and then call the new function from the old one.

I'm curious - in your example, if you change the NFD_OpenFileDialog struct, won't the ABI still break?

@btzy
Copy link
Owner Author

btzy commented Apr 11, 2023

You probably thought about this, but there is also the option of renaming the new extended function, e.g. to NFD_OpenDialogN_v2 and then call the new function from the old one.

That will work for now, but do we want to keep increasing the version number every time we add a new parameter? In addition, having more parameters makes the function more unwieldy to call, and it is not as obvious what each argument means. NFD_OpenDialogN_v4(outPath, filterList, filterCount, nullptr, nullptr, nullptr, "/usr/bin/local") anyone?

I'm curious - in your example, if you change the NFD_OpenFileDialog struct, won't the ABI still break?

No, because nfd.h only contains a forward declaration of NFD_OpenFileDialog, but not its definition. Something like

struct NFD_OpenFileDialog;

The definition is in each NFD source file, and they may differ between backends. For example, on Windows, NFD_OpenFileDialog may be a typedef for IFileDialog, or a struct that contains an IFileDialog* and some other fields (in case they need to be stored separately).

This means that you can't create an instance of NFD_OpenFileDialog in your code, so it does not break ABI to add more fields to the struct.

@btzy
Copy link
Owner Author

btzy commented Apr 11, 2023

The main reason for naming it NFD_OpenFileDialog instead of NFD_OpenDialog is the name clash with the existing function, but I'm open to using other names that might be less verbose.

@krupkat
Copy link

krupkat commented Apr 11, 2023

Thanks for taking the time to explain, it makes sense! The renaming method isn't sustainable long term, I agree.

@btzy
Copy link
Owner Author

btzy commented Apr 20, 2023

Another interesting possibility is to put the following in nfd.h:

typedef struct {
    Filter* filter_list = NULL;
    size_t filter_count = 0;
    const char* title = NULL;
    const char* current_folder = NULL;
    bool modal = true;
    const char* parent_window = NULL;
    /* more fields might be added here in the future so this struct should not be passed across an ABI boundary */
} NFD_OpenDialogProperties;

/* This is the new function to call to show an open file dialog */
nfd_result NFD_OpenDialogWithProperties(const char** outPath, NFD_OpenDialogProperties props) {
    // This calls some library functions in some order supported by the library (the order and the way it is called depends on the platform)
    // E.g. on Linux we might need to pass the parent window when constructing the dialog
    void* handle = NFD_OpenDialog_Impl_New(props.parent_window);
    if (filter_count) NFD_OpenDialog_Impl_SetFilterList(handle, props.filter_list, props.filter_count);
    if (title) NFD_OpenDialog_Impl_SetTitle(handle, props.title);
    if (current_folder)NFD_OpenDialog_Impl_SetCurrentFolder(handle, props.current_folder);
    // E.g. `result` and `modal` are passed together here due to the shape of the platform native API
    return NFD_OpenDialog_Impl_Show(handle, outPath, props.modal);
}

/* APIs to set individual properties - while these are stable, users should not use them directly because the functions may need to be called in a specific order!  These functions may vary between backends.  Use NFD_OpenDialogWithProperties() instead in user code.  */
void* NFD_OpenDialog_Impl_New(const char* parent_window);
void NFD_OpenDialog_Impl_SetFilterList(void* handle, Filter* filter_list, size_t filter_count);
void NFD_OpenDialog_Impl_SetTitle(void* handle, const char* title);
void NFD_OpenDialog_Impl_SetCurrentFolder(void* handle, const char* current_folder);
bool NFD_OpenDialog_Impl_Show(void* handle, const char** result, bool modal);

The user will then write the following code to open the file dialog (all the properties are optional, just set what you need):

char* result;
if (NFD_OpenDialogWithProperties(&result, {
        .filter_list = my_list,
        .filter_count = 4,
        .parent_window = get_native_window()}) == NFD_OKAY) {
    printf("Processing file %s", result);
    free(result);
}

As long as the caller does not rely on the size of the NFD_OpenDialogProperties struct, they won't need to modify their code to compile with a newer version of NFDe (i.e. no API break). If NFDe is compiled as a shared library, it is also possible to update the NFDe shared library without recompiling the calling code (i.e. no ABI break).

Programs that depend on NFDe should not pass the NFD_OpenDialogProperties across an ABI boundary, but in any case this seems very unlikely for most projects. Probably the only projects that might consider putting NFD_OpenDialogProperties in a public interface are libraries that bundle NFDe with other platform abstraction libraries, but putting a warning about that in the NFDe documentation/README should probably suffice.

However, I have never seen this style of interface in other open source libraries.

@stevengj
Copy link

stevengj commented Apr 2, 2024

As long as the caller does not rely on the size of the NFD_OpenDialogProperties struct

If you want to do this, and ensure ABI compatibility if the struct is changed, the right thing to do is to not expose the struct at all, only a pointer to the struct — declare it as an opaque struct in the public header, and provide accessor functions for the properties.

In the public header file, you define e.g.:

typedef struct NFD_OpenDialogProperties_s *NFD_OpenDialogProperties; /* pointer to opaque struct */

with the actual struct NFD_OpenDialogProperties_s defined in an internal (not public) header or .c file. Then define constructor/destructor and accessor functions:

NFD_OpenDialogProperties *NFD_OpenDialogProperties_create(); /* return pointer to new struct, default vals */
void NFD_OpenDialogProperties_free(NFD_OpenDialogProperties *p); /* deallocate struct + data */

/* accessors */
const char *NFD_OpenDialogProperties_title_get(const NFD_OpenDialogProperties *p);
void NFD_OpenDialogProperties_title_set(NFD_OpenDialogProperties *p, const char *title);
/* etc. */

This way you can change the struct in any way that you want, and the ABI need not change.

@btzy
Copy link
Owner Author

btzy commented Apr 2, 2024

The current plan is to pass a struct together with a version number, and when new fields are added to the end of the struct, the version number will be bumped. It is implemented in #128. It has the benefit of not needing to introduce a new exported function for each property.

@btzy btzy closed this as completed in #128 Jun 23, 2024
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

Successfully merging a pull request may close this issue.

3 participants