SDL icon indicating copy to clipboard operation
SDL copied to clipboard

Implement file dialogs

Open Semphriss opened this issue 2 years ago • 56 comments

I have several uncertainties about my PR as of now, and since I know I'll forget a few of them, I'd like to specify that I'm open to adapt anything in this PR as necessary, no matter how big or small.

Description

This PR adds support for open file, save file, and open folder dialogs across platforms.

Example of use:

#include <SDL3/SDL.h>

#include <stdio.h>

void callback(const char* const* files, void* ptr)
{
    if (files)
    {
        while (*files)
        {
            printf("'%s'\n", *files);
            files++;
        }
    }
    else
    {
        printf("Error: %s\n", SDL_GetError());
    }
}

int main()
{
    SDL_DialogFileFilter filters[3] = { {"JPG files", "*.jpg;*.jpeg"},
                                        {"PNG files", "*.png"}, {} };
    SDL_Window *w;
    SDL_Renderer *r;
    SDL_Event e;
    int quit = 0;

    SDL_Init(SDL_INIT_VIDEO);
    w = SDL_CreateWindow("Hello", 640, 480, 0);
    r = SDL_CreateRenderer(w, NULL, 0);

    /*
     * Arguments, in order:
     * - A function to call when files are chosen (or dialog is canceled, or error happens)
     * - A user-managed void pointer to pass to the function when it will be invoked
     * - The window to bind the dialog to, or NULL if none
     * - A list of filters for the files, see SDL_DialogFileFilter above (not for SDL_ShowOpenFolderDialog)
     * - The path where the dialog should start. May be a folder or a file
     * - Nonzero if the user is allowed to choose multiple entries (not for SDL_ShowSaveFileDialog)
     */
    /* SDL_ShowOpenFileDialog(callback, NULL, w, filters, "/home/", 1); */
    /* SDL_ShowSaveFileDialog(callback, NULL, w, filters, "/home/"); */
    /* SDL_ShowOpenFolderDialog(callback, NULL, w, "/home/", 1); */
    SDL_ShowOpenFileDialog(callback, NULL, w, filters, "/home/", 1);

    while (!quit)
    {
        SDL_Delay(100);

        SDL_SetRenderDrawColor(r, 0, 0, 0, SDL_ALPHA_OPAQUE);
        SDL_RenderClear(r);
        SDL_RenderPresent(r);

        while (SDL_PollEvent(&e))
        {
            if (e.type == SDL_EVENT_QUIT)
            {
                quit = 1;
                break;
            }
        }
    }

    SDL_DestroyRenderer(r);
    SDL_DestroyWindow(w);
    SDL_Quit();
    return 0;
}

I left the file above in test/dialog.c; is this the proper place to put files like that?

I also left a few TODO's in the code, at places where I would need help understanding the ways of SDL.

Here are my comments, questions and worries:

General

I did not know where to put the code related to dialogs. I initially though about putting it in the video folder, but I realized that on Unix, the concept of file dialogs was not tightly linked to the graphical interfaces. For example, whereas the video system is (or seems, based on a quick look) largely based window managers like Wayland or X11, the file dialogs are based on toolkits like GTK or KDE. As such, I decided to keep dialogs in a separate folder, to keep my personal clutter away from the existing codebase. I'm willing to move my code to a better place if necessary.

In addition, I thought it could be relevant to have a separate folder for dialogs, since more dialog types can be added later, like a color picker dialog.

If dialog-related code is agreed to stay in its own folder, I'd have a few more questions:

  • How should I integrate it in CMakeLists.txt? I decided to keep all my code stuck together so that it would be easy to move around as necessary.
  • Is there a template for the structure of the files? I did try to imitate what I saw in other files, but I most likely overlooked many important things. Most notably, I noticed most source files have an SDL_defined platform-specific include guard, but I have found that I did not need any for my integration. That probably means I did something wrong along the way; how should I structure the macros, guards, includes, and the rest?
  • How should I handle the dummy implementation? (I presume this will be answered with the questions above)

API

I thought it would be better to make the function asynchronous to avoid hanging applications while a dialog is open.

I noticed various platforms had varying degrees of flexibility when customizing the dialogs (some allowed to change the title, some also the buttons, etc.); since there is already a lot of arguments in the functions, I preferred to keep it simple and only include the functionally relevant stuff. I can add more argument for cosmetic changes if wanted.

I originally did the filters be a single string in the style of "Office Document|*.doc;*.docx", but I thought that would make it harder to localize the label name, so I settled for an hybrid between char-separated and struct with { "Office document", "*.doc;*.docx" }. I though it would be needlessly complicated to also split each extension pattern to be its own string, so I kept it char-separated.

Windows

I used the older GetOpenFileName instead of the newer IFileDIalog. I can implement the newer one if wanted.

Unix/Linux

There are plenty of ways to display a dialog on Linux, but apparently no standard one. I settled for the easiest option: using Zenity. It does, however, require opening up a whole new process. There are libraries like GTK that would allow doing this without passing through a new process, but I'm not familiar enough with GTK to implement it.

macOS

I'm just not sure if I should use runModal(), begin(), or something else.

Emscripten

I initially planned implementing file dialogs on Emscripten, but I realized that there are no dialogs that work with MEMFS; they only appear on interaction with the user's host filesystem. This poses a number of problems:

  1. Due to browsers being restricted for security, this means that the function can't deal with filenames. It would have to deal with file handles directly. It could probably be achieved by changing the API, but it would make the code much more complex, and mostly, it would penalize the other platforms.
  2. No "Open folder" dialog. Even if it existed, it wouldn't be of any use, since the website is not allowed to open and scan the contents of a folder in the user's host filesystem.
  3. Although the "Open File" dialog could be made workable by transferring the file to open in MEMFS and then returning a file path, the "Save file" dialog would be useful only to actually perform the action of saving the data, and the application may never get the actual path.

Because those differences pretty much break the whole principle of the file dialogs, I believe it would be better to let the end user implement their own dialogs with Emscripten.

Haiku

~~I did start trying to port the code to Haiku, but I've been stopped by a few problems that I'm not familiar enough yet to resolve, so it may come later.~~

Edit: Haiku dialogs are now supported.

Mobile

I don't think Android or iOS support file dialogs in the traditional sense, so I did not implement anything for those platofrms.

Existing Issue(s)

Fixes #7445

Semphriss avatar Jun 01 '23 22:06 Semphriss

This is a reasonable first-pass at this functionality. @libsdl-org/a-team, thoughts?

slouken avatar Jun 04 '23 07:06 slouken

My patch makes dialogs a proper subsystem that can be disabled by configuring with -DSDL_DIALOG=OFF, builds the test with cmake, and fixes a bug with zenity args.

I didn't look into the implementation but noticed the following issues:

  • User cancellation does not work with zenity. It always signals an error.
  • When I run test/dialog with all SDL_ShowXXXDialog commands enabled, no dialogs are shown, one error message is printed + SDL quits with exit code 1.
  • I think you need to do SDL_DetachThread to avoid thread handle leaks.

madebr avatar Jun 04 '23 17:06 madebr

There are plenty of ways to display a dialog on Linux, but apparently no standard one. I settled for the easiest option: using Zenity.

There is a standard way to display a file dialog on Linux: portals. There's also a fallback portal in the event the DE/WM doesn't provide its own file chooser, xdg-desktop-portal-gtk. I strongly encourage you to use portals on Linux instead, we need more people behind portals to help move the desktop forward.

https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.FileChooser

orowith2os avatar Jun 04 '23 18:06 orowith2os

@madebr Thanks for the patch!

I'm trying to reproduce the two issues you've mentioned, about Zenity misreporting a cancellation as an error, and about multiple simultaneous dialogs causing problems, but both seem to work fine on my system. I'm testing on Ubuntu 22.04, what system do you use?

I'll also add the SDL_DetachThread's where appropriate :+1:


@orowith2os I've been looking into this when I was looking for which mechanism to implement, and I noticed that my system (Ubuntu 22.04) did not seem to come with portals by default. To what extent is it standard across Linux distros + *BSD, and other Unixes? When I looked into it, it seemed to be Flatpak-specific.

To clarify, I'm not against implementing different mechanisms, I actually planned to implement Zenity + Kdialog + GTK + Qt + any other that might be relevant (I only stopped after appreciating the load of work Zenity was). But I understand, from your message, that it would be standard as to be more reliable and widely supported than Zenity or Kdialog, but my personal research indicated me otherwise; to what extent can we rely on the availability of portals across OSes?

Semphriss avatar Jun 04 '23 20:06 Semphriss

To what extent is it standard across Linux distros + *BSD, and other Unixes?

Just a Linux desktop thing, but other Unixes can implement them too. It's not flatpak-specific either, nor should it be treated as such. It came out of the need for a secure Flatpak filechooser API, but it has grown to work well outside of it and integrates very nicely with the environment, regardless of application toolkit.

Ubuntu should also come with portals by default, assuming you're using GNOME; try systemctl --user status xdg-desktop-portal-gnome, systemctl --user status xdg-desktop-portal-gtk, and systemctl --user status xdg-desktop-portal. Otherwise, it would be very weird, considering Ubuntu relies on portals in Snap too.

If an environment doesn't have at least xdp-gtk installed, I like to treat that as a desktop bug.

orowith2os avatar Jun 04 '23 21:06 orowith2os

Ah, good to know! I must have picked a faulty example snippet to test it.

May I ask for your help to implement it? Given that I struggle to even get a basic example to work, I think it would be more efficient to let you do it - if you have the time, of course.

Semphriss avatar Jun 04 '23 21:06 Semphriss

I'm more suited to Rust code, so unless SDL3 is modified to include Rust in the codebase too, I can't help much, sorry.... You might have luck with the libportal library, and maybe some other third party libraries.

Worst case you just stick with zenity, which I believe uses portals if needed and otherwise falls back to GTK3? In which case this was more just me wanting SDL2 to directly use portals and make them more common.

orowith2os avatar Jun 04 '23 21:06 orowith2os

I just added filter support on macOS, which I believe is the last thing I had planned to do for this PR. I'll mark it as ready for review, and I remain available to help with further changes.

Semphriss avatar Jun 06 '23 21:06 Semphriss

Something I'd need help with: is there an SDL equivalent to mbstowcs?

Semphriss avatar Jun 07 '23 14:06 Semphriss

Something I'd need help with: is there an SDL equivalent to mbstowcs?

You'll need to use MultiByteToWideChar() in SDL.

slouken avatar Jun 07 '23 14:06 slouken

Some (possibly unreasonable) notes:

  • If these are async, do we want a callback, or do we just want it to post to the event queue when it's complete?
  • Should these be async at all?
  • If we're worried about having too many arguments to a function, put them in a struct (and allow strings to be NULL to signify "I don't care about the title bar" or whatever, so the app can supply as many arguments as it cares about).

icculus avatar Jun 07 '23 17:06 icculus

Should these be async at all?

Yes, probably. It's not a good idea to block on a file dialog, as the app gets marked as unresponsive after 10-30 seconds, at least on GNOME/Linux. This is an issue with Inochi Creator, although with different libraries.

orowith2os avatar Jun 07 '23 18:06 orowith2os

About async:

  • I've considered whether to make them sync or async, and I decided to settle on async because many use cases for those dialogs should allow the main app to continue running in the background. Notably, dialogs which aren't bound to a window shouldn't prevent windows from rendering properly. The user could decide to launch the dialogs from a different thread if they wanted async, but the point below makes this impossible:
  • Certain platforms, including macOS, provide functions which are async by default. I could probably write some code to freeze the main thread until the dialog closes, but I've noticed that the Cocoa dialogs require the main thread to keep running for the dialog window to refresh properly. After some investigation, it seems that macOS has strict requirements between dialogs and threads, so even if I were to make it sync somehow, the user could not safely launch a dialog from another thread if they wanted it to be async. Since macOS would have to be async in any case, I decided to keep it similar across all platforms. (Now that I think about it, I should probably write that in the documentation.)

As for callbacks vs. events, the Haiku port does seem like it would be much easier to implement with events, but the downside I've thought about is that this would separate the code that fires up the dialog and the code that handles the reply very far apart from each other. It would also require the app to be able to identify which dialog event corresponds to which dialog invocation, and I didn't think that would be easy, especially with multithreaded apps and/or concurrent calls to the functions.

For the function arguments, one of the reasons I decided to not include any cosmetic (non-functional) argument was because the cross-platform support for many options was very poor (is it worth having an option if only one platform supports it?). Since I wasn't sure which subset of the available options I should pick, if any, I decided not to include any, but I'm fine with changing that if wanted.

Those are the thoughts I had during the development, I thought it would be relevant to share them. Nevertheless, if it's still preferable to change those things, I'm fine with changing them.

Semphriss avatar Jun 07 '23 18:06 Semphriss

A little something while I have it on my mind: some platforms require the filters to be formatted like png;jpg;jpeg while others need *.png;*.jpg;*.jpeg. Which one should I require for SDL?

Semphriss avatar Jun 07 '23 18:06 Semphriss

Okay, these are fair points.

icculus avatar Jun 07 '23 18:06 icculus

Should the api support modal dialogs? The current api simply shows a dialog, but that can easily become obscured or hidden. I think a user can become confused that way.

madebr avatar Jun 07 '23 18:06 madebr

Should the api support modal dialogs?

I think they should be modal to the specified window, and not modal (or maybe app modal) if no window is specified. Although...maybe we should forbid NULL windows...? I don't really know what support for that on various platforms looks like...I'm also not sure we can dictate modality details across all platforms, but this is not an area I'm super-familiar with.

icculus avatar Jun 07 '23 18:06 icculus

A little something while I have it on my mind: some platforms require the filters to be formatted like png;jpg;jpeg while others need .png;.jpg;*.jpeg. Which one should I require for SDL?

I'd vote for png;jpg;jpeg ...we can easily add the *. on the backend when necessary and it prevents shenanigans like "abc*xxx.jpg" being entered when we only promise file extensions.

icculus avatar Jun 07 '23 18:06 icculus

Should the api support modal dialogs?

I just checked the definition of "modal", my apologies if I get the definition wrong, but currently, both Windows and macOS use modal dialogs if a window is provided; only the Unix implementation does not support modal dialog because it uses Zenity under the hood. I believe a library implementation (or possibly portals) would support binding a dialog to a window.

Although...maybe we should forbid NULL windows...?

Implementing Zenity made me think that there might be applications that need graphical file dialogs, but not any window. (Zenity was made to allow using common dialogs from shell scripts.) All platforms I've looked into support binding no window, and one (Unix, with Zenity) currently does not support binding a window at all.


I'll change the extension format :+1:

I've noticed that this commit I pushed doesn't seem to appear here, I hope that will fix itself with the next commit.

Semphriss avatar Jun 07 '23 19:06 Semphriss

To clarify:

I think they should be modal to the specified window, and not modal (or maybe app modal) if no window is specified.

This is how I made it work, with a small detail: If a certain platform or implementation does not support setting the modality of a dialog, the option is ignored. For example, both Windows and macOS make a dialog modal if a window is provided and not modal if no window is provided; on Unix, because Zenity does not support binding a dialog to a window, the option is currently ignored, but a future implementation should add this functionality if it is supported. Similar with a potential Haiku implementation.

Semphriss avatar Jun 07 '23 19:06 Semphriss

This all sounds great. :)

slouken avatar Jun 07 '23 19:06 slouken

A few remaining problems I'd need feedback on:

  • The "All files" filter does not work on macOS (not sure if it is even supported)
  • The PSP builds fail because of some missing sceModuleInfo, I'm not familiar with PSP development enough to fix it
  • The MSVC builds fail because of unresolved external symbol __imp__beginthreadex and __imp__endthreadex, I'm not sure how to fix it

Semphriss avatar Jun 07 '23 22:06 Semphriss

I'm not sure about the all files macOS filter, but I've seen file dialogs that show all files, so I'm sure it's possible. Don't worry about the PSP build failure. The Win32 thread issue can be solved by including "../../thread/SDL_systhread.h" and then using SDL_CreateThreadInternal()

slouken avatar Jun 08 '23 00:06 slouken

For macOS, I've noticed that by default it's not possible to select filters from a drop-down menus like with Windows or Zenity, and all filters are active by default (as in, the user can select both JPEG and PNG files simultaneously), unlike Windows and Zenity.

It is possible (on all platforms including macOS) to allow selecting any file by setting the filters to NULL, so I could work around this issue by detecting the wildcard All files (*.*) filter and disabling the filters if it is found in the list. The resulting behavior would be indistinguishable from the expected behavior. Would it be a good idea to implement that?

Semphriss avatar Jun 08 '23 01:06 Semphriss

About SDL_CreateThreadInternal(), I must confess I'm not very familiar with threads, so I should ask: how do I determine which value I should pass to the stack argument when creating a thread?

Semphriss avatar Jun 08 '23 01:06 Semphriss

Pass zero for the stack argument, which uses the system default, especially since this is going to be calling back into app code.

icculus avatar Jun 08 '23 04:06 icculus

It is possible (on all platforms including macOS) to allow selecting any file by setting the filters to NULL, so I could work around this issue by detecting the wildcard All files (*.*) filter and disabling the filters if it is found in the list. The resulting behavior would be indistinguishable from the expected behavior. Would it be a good idea to implement that?

Yep, that sounds great.

slouken avatar Jun 08 '23 05:06 slouken

There are only two problems left with MSVC builds:

  • One linkage (?) problem on Windows. I'm not sure if there's a standard practice with SDL that I failed to follow somewhere, it's probably something simple but I'm not familiar enough with SDL, how should I fix this?
  • UWP is apparently just not supported, I'm not sure if I can #ifdef out UWP from the Windows implementation, or wire something in CMakeLists.txt?

The PSP builds also fail, but I'm leaving that alone as suggested.

Semphriss avatar Jun 08 '23 19:06 Semphriss

Can you squash for a single commit for review?

Thanks!

slouken avatar Jun 11 '23 17:06 slouken

@madebr I've added you as a co-author on the squashed commit since you wrote some code to improve the PR, is that okay?

Semphriss avatar Jun 11 '23 19:06 Semphriss