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

Set default file extension in Windows save dialog #86

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

samangh
Copy link

@samangh samangh commented Mar 12, 2023

First of all, thank you for your work on portable-file-dialogs. It's quite useful :-).

This pull request will set the default file extension when using the file save dialog in Windows. The default file extension is set to the first file extension provided in the filter pattern.

It turns out that the GetSaveFileNameW function in Windows does not automatically get the file extension from the filters, instead the the default extension must be manually set. So this is what this pull request does.

This should close issue #43.

@gamagan
Copy link

gamagan commented Mar 12, 2023

Questions:

  1. What happens if the user selects a different file extension from the dropdown? Does the file
    a) Still get saved with the default extension
    b) Uses the new extension selected
    c) Uses no extension
    d) Other
  1. Seem to me like regular expressions are overkill for getting the extension value. Can't a few calls to std::string::find() accomplish that?

With this commit, in a Windows file extension, if the user has not
entered a file extension, then the file extension from the filter
pattern will be used.
@samangh
Copy link
Author

samangh commented Mar 13, 2023

@gamagan, thanks for the questions. They caused me to look at this more closely and do some experimentation.

I was actually incorrect in my previous message. As long as the lpstrDefExt pointer is allocated, Windows does the right thing and uses the extension defined in the pattern filter. The actual content of lpstrDefExt is only used if the filter pattern is blank or is *.*, and the user has not entered an extension.

I have updated my pull request to match this. In the new pull request, things should work as expected. So, for example let's say you do:

pfd::save_file("Select a file", ".",                            
              { "Image Files", "*.png *.jpg *.jpeg *.bmp",
                "AudioFiles", "*.wav *.mp3",
                "All Files", "*" });

The default extension will be .png if Image Files is selected, .wav if Audio Files is selected, and no extension is added if All files is selected.

@gamagan
Copy link

gamagan commented Mar 13, 2023

Wow. That's nice that the change turned out to be so simple for the benefits it provides. Good work.

So, Windows only supports 3 letter extensions, in that case? There's no code to set the size of the buffer, so I would think that the extension size would be fixed. It wouldn't be ideal if it only supported 3 letters.

@samangh
Copy link
Author

samangh commented Mar 13, 2023

Yes, the automatically added extensions are limited to 3 characters. This is a limitation of the deprecated GetSaveFileNameW function used here.

The lpstrDefExt value defines the default extension when the function can't figure out what the extension should be (i.e. the pattern filter blank or is *.*). We leave it empty, so that Windows does not add an extention in those cases. As we are leaving lpstrDefExt empty, it's size is irrelevant.

@samangh
Copy link
Author

samangh commented Mar 30, 2023

@samhocevar, I was wondering if you've had a chance to look at the PR? I think it's a nice uncontroversial 2-line change.

@samangh
Copy link
Author

samangh commented Jan 13, 2024

@samhocevar? Any chance of merging this pull request?

@tgalaj
Copy link

tgalaj commented Jan 31, 2024

@samhocevar? Any chance of merging this pull request?

Yup, accepting this PR would make our lives much, much easier @samhocevar

This small library is really great, it would be a waste to abandon it.

@christophe-f8
Copy link

christophe-f8 commented Feb 29, 2024

I have the same problem, the extension is not automatically added and I'd like this to be merged too. For now, I'm using @samangh branch.

I did further improve this actually so I thought I would share. I didn't wanted to open yet another PR. @samangh maybe you can update yours ?
Anyway, here are the two small modifications:

  • increase the extension length to 64 characters (with only 3 characters it wouldn't work with ".json" for example).
  • Solved a similar bug in file open dialogs: if the user types a name of a file that does not exists, there was no popup showing (works well including with multiple file selections).

In the end, the few lines at that place in the file become:

        if (in_type == type::save)
        {
            auto wextension = std::wstring(64, L'\0');              // Changed 3 to 64 here
            ofn.lpstrDefExt = (LPWSTR)wextension.data();
            
            if (!(options & opt::force_overwrite))
                ofn.Flags |= OFN_OVERWRITEPROMPT;

            dll::proc<BOOL WINAPI (LPOPENFILENAMEW)> get_save_file_name(comdlg32, "GetSaveFileNameW");
            if (get_save_file_name(&ofn) == 0)
                return "";
            return internal::wstr2str(woutput.c_str());
        }
        else
        {
            if (options & opt::multiselect)
                ofn.Flags |= OFN_ALLOWMULTISELECT;
            ofn.Flags |= OFN_PATHMUSTEXIST;
            if (in_type == type::open)                                 // Added these two lines
                ofn.Flags |= OFN_FILEMUSTEXIST;

            dll::proc<BOOL WINAPI (LPOPENFILENAMEW)> get_open_file_name(comdlg32, "GetOpenFileNameW");
            if (get_open_file_name(&ofn) == 0)
                return "";
        }

The required version number (3.5) us so old that CMake is now
complaining about this.

Removing a required CMake version fixes this. We don't use anything
complicated so any CMake version will be compatible.
@samangh
Copy link
Author

samangh commented Mar 7, 2024

@christophe-f8 Sure, I can look at merging this in my branch.

As this is meant to be a portable library I'll have to check that the same behaviour is also implemented in other environments.

@samangh
Copy link
Author

samangh commented Mar 10, 2024

@christophe-f8, your suggested changes are now in my fork.

samangh added 2 commits March 11, 2024 00:31
`GetActiveWindow()` will return NULL if called asynchronously, as it's
calling thread will not have a message loop. To work around this, use
`GetForegroundWindow()` if `GetActiveWindow()` is null.

This will cause any dialogs to be modal.

Fixes samhocevar#82 and
samhocevar#93
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 this pull request may close these issues.

5 participants