wxMSW SaveAs with filename containing "." does not correctly append extension. (Issue #23193)
jpbreen85
Description
Under Windows, the SaveAs functionality behaves in an unexpected way when the filename contains a "." character, and has no extension. The code in AppendExtension is satisfied by the existence of a generic ".", even if it is not associated with an extension, and so does not append an extension even if one is not present.
Code snippet from AppendExtension:
wxString wxFileDialogBase::AppendExtension(const wxString &filePath,
const wxString &extensionList)
{
// strip off path, to avoid problems with "path.bar/foo"
wxString fileName = filePath.AfterLast(wxFILE_SEP_PATH);
// if fileName is of form "foo.bar" it's ok, return it
int idx_dot = fileName.Find(wxT('.'), true);
if ((idx_dot != wxNOT_FOUND) && (idx_dot < (int)fileName.length() - 1))
return filePath;
This code snippet shows that the "." can occur anywhere in the filename, not just as part of the extension. This results in a valid filename containing ".", but no extension, not having an extension correctly appended. The ideal behaviour should be for an extension to be properly identified, then checked against the extentionList argument. This would ensure only correct extensions are accepted.
Platform and version information
- wxWidgets version you use: 3.2.1
- wxWidgets port you use: wxMSW
- OS and its version: Windows 10
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Sorry, I don't understand what the problem is exactly. Which file name do you enter into the dialog, as the user, and which file gets saved and which file would you expect it to be?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jpbreen85
If you invoke wxDocument::SaveAs() via wxID_SAVEAS, with extension filter of the form
"Test (.test)|.test" and enter a filename in the dialog such as "this.is.a.file" with no extension specified the above code wxFileDialogBase::AppendExtension treats this as having a valid extension and returns before appending an extension. The code in wxFileDialogBase::AppendExtension is faulty as it always assumes any "." character present in a filename is associated with an extension, when it can be a valid part of a filename. So the desired outcome would be a file with filename "this.is.a.file.test" instead wxDocument::SaveAs() results in a filename "this.is.a.file".
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
wxDocument::SaveAs() just uses wxFileDialog, so it's really about the behaviour of wxFileDialog itself. And I actually don't think we want to append the extension to a file already having one, even if it's a different one -- e.g. even if the filter uses *.txt as the only allowed extension, if I enter foo.md into the file name zone, I want the file to be saved with this full name and not as foo.md.txt.
So I'm tempted to say that this works as expected.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jpbreen85
But the "." character does not have to be associated with an extension. You can have a perfectly valid filename "this.is.a.file" with no extension specified. "." is a valid character within a filename. The code should be able to differentiate between a "." in a filename and a "." associated with an extension, using the wxString &extensionList passed into wxFileDialogBase::AppendExtension.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
We can't distinguish between using period before an extension and as something else. However you can always append the desired extension manually (e.g. enter a.file.test) if this is what you really want, but you wouldn't be able to easily prevent it from happening if you did not want it to be appended. So the current behaviour is IMO the least bad choice.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
As vadz wrote, there is no definition of an extension, so any string after the last period is usually treated as one.
However, in the case of file dialog, this can get trickier, as Windows differentiates between known and unknown extensions, see the informative discussion and code in #22088.
So, IMO, here it is important that wxWidgets returns the same file name that the file dialog tests for existence when wxFD_OVERWRITE_PROMPT is set. Does it now, both for "known" and "unknown" extensions?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jpbreen85
Thank you for your reply. I believe the discussion in #22088 is not relevant in this case. The example where test.txt returns test.txt.dxf is incorrect because of the code in wxFileDialogBase::AppendExtension. This method returns due to the existence of the "." character, and the extension, in this case, "dxf" is not appended. Vadz in his previous comment suggested that "if I enter foo.md into the file name zone, I want the file to be saved with this full name and not as foo.md.txt.". However it could be argued that this is the expected behaviour. We have access to the extensionList, why not use it to ensure the filename has the desired extension?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #23193 as completed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
It could be argued that this is the expected behaviour, but as I also wrote, if you disagree with this you can't override it easily. If we do not add the extension automatically, you can easily add it yourself, so unless there is a 100% consensus that we always want to append it (and there clearly isn't one), I think it's better to make the choice which is easy to override.
So I don't think we're going to change anything there, sorry.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Reopened #23193.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Interestingly enough this just came out in another context and made me think more about this and realize that even if we don't want to always append extension, we still can improve the behaviour here:
- We can safely assume that if there is a space after the last period, then this period doesn't mark the start of an extension. This would make appending extension work with the file names like "My 0.02 cents" and doesn't seem to have any drawbacks.
- We probably can also assume that an extension doesn't have more than a few (4? 5? 8?) letters, and so still append the extension if the part after the last period is too long. This is less robust, because it could result in false positives (
README.markdownseems plausible), but could also be helpful.
Anyhow, at least (1) seems worth doing, so reopening this to do it.
What do the others think about (2) and what should the cut off length be if we do add this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Another idea that arose while discussing this is to add some wxFD_ALWAYS_APPEND_EXTENSION flag that could be specified to force appending an extension to the file being saved. This file could even be turned on for this wxFileSelector() call:
as docview-based applications really want to use some extension, as otherwise they wouldn't be able to open files they have just saved themselves.
I.e. I think it's actually reasonable to implement what this issue originally proposed for these applications, even if I still think we don't want to impose this on all code using wxFileDialog.
And we could still implement (1) from the previous comment when wxFD_ALWAYS_APPEND_EXTENSION is not given, although this would be much less important then, I guess.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
OK, this is embarrassing, but after discussing this for so long, I've finally actually tested this and found that I can't even reproduce the problem: whatever I enter in the "Save" file dialog of the dialogs sample, the file dialog itself already appends the extension to the entered name, so it doesn't matter what AppendExtension() does because the extension is already there.
How exactly can I reproduce the problem you're seeing? The other bug report I got was under Windows 11 and I still don't have it, so I can't test there, but under Windows 10 the extension seems to be always appended by the dialog itself.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
I have this simple code built with wxWidgets master as 64-bit static
#include <wx/wx.h> #include <wx/filedlg.h> class MyApp : public wxApp { bool OnInit() override { for ( ; ; ) { wxFileDialog fd(nullptr, "Save file", "", "", "Text Files (*.txt)|*.txt|XYZ Files (*.xyz)|*.xyz|All Files (*.*)|*.*", wxFD_SAVE | wxFD_OVERWRITE_PROMPT); if ( fd.ShowModal() != wxID_OK ) break; wxLogDebug("Selected file '%s'.", fd.GetFilename()); } return false; } }; wxIMPLEMENT_APP(MyApp);
Running it on on Windows 10 22H2 (build 19045.2486) produced (filter is set to default Text Files (*.txt)|*.txt):
| Entered | Getfilename() returns |
Note |
|---|---|---|
| test | test.txt | expected, the extension is already in m_path returned by fileDialog.Show() |
| test. | test..txt | odd?, the extension is already in m_path returned by fileDialog.Show() |
| test.cpp | test.cpp | expected, known extension not modified |
| test.blah | test.blah.txt | expected, unknown extension, the extension is already in m_path returned by fileDialog.Show() |
I was a bit surprised to see that when I set the filter to All Files (*.*)|*.*, IFileDialog appended the extension from the filter I had selected before All Files (be it .txt or .xyz).
Repeating the test on Windows 11 22H2 (build 22621.1194) as VirtualBox VM produced different results:
| Entered | Getfilename() returns |
Note |
|---|---|---|
| test | test.txt | expected, the extension was not in m_path returned by fileDialog.Show(), wxFileDialogBase::AppendExtension() added it |
| test. | test.txt | the lone period was not in m_path returned by fileDialog.Show(), wxFileDialogBase::AppendExtension() added ".txt" |
| test.cpp | test.cpp | expected, known extension not modified |
| test.blah | test.blah | not modified |
Am I seeing things, how can there be a difference between Windows 10 and 11 with such basic code?! As I have "Hide extension for known file types" in Explorer on Windows 10 unchecked , I unchecked it also on Windows 11, but it did not change anything. TBH, I ran out of time and could not check where m_path returned by fileDialog.Show() comes from.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, this confirms that the behaviour of the file dialog has changed in Windows 11.
I really need to set it up here to be able to debug it, but I think the OP might have just entered the OS version wrong and also ran into this there, as under Windows 10 I just don't see it (but thanks for pointing out the problem with the empty extension, we need to fix this too).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
The plot thickens... I've installed only a slightly different (10.0.22621.1104) Windows 11 22H2 version in a VM and I see exactly the same behaviour as under Windows 10 there, i.e. I still get the path with the extension appended from IFileDialog itself. I tested with the dialogs sample initially, but I've also retested with the exact example from above and I still exactly the same thing as under Windows 10.
I guess I'm going to need to update to 22621.1194, but could they have really changed this inside such a minor update? I feel like I'm missing something here...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I can't reproduce the problem even after enabling OneDrive and I don't know what else to try.
For those who can reproduce it, could you please apply this:
diff --git a/src/msw/filedlg.cpp b/src/msw/filedlg.cpp index fd85776b6c..10bb5a2939 100644 --- a/src/msw/filedlg.cpp +++ b/src/msw/filedlg.cpp @@ -1597,7 +1597,11 @@ int wxFileDialog::ShowIFileDialog(WXHWND hWndParent) wxString defExt = wildFilters[m_filterIndex].BeforeFirst(';').AfterFirst('.'); if ( !defExt.empty() && defExt != wxS("*") ) - fileDialog->SetDefaultExtension(defExt.wc_str()); + { + hr = fileDialog->SetDefaultExtension(defExt.wc_str()); + if ( FAILED(hr) ) + wxLogApiError(wxS("IFileDialog::SetDefaultExtension"), hr); + } } if ( !m_dir.empty() )
and check if you see any error messages?
And, whether you do or not, does moving this call before the call to SetFileTypes() like this change anything:
diff --git a/src/msw/filedlg.cpp b/src/msw/filedlg.cpp index fd85776b6c..72ddacc3fe 100644 --- a/src/msw/filedlg.cpp +++ b/src/msw/filedlg.cpp @@ -1574,6 +1574,20 @@ int wxFileDialog::ShowIFileDialog(WXHWND hWndParent) wildFilters); if ( nWildcards ) { + // We need to call SetDefaultExtension() to make the file dialog append + // the selected extension by default. It will append the correct + // extension depending on the current file type choice if we call this + // function, but won't do anything at all without it, so find the first + // extension associated with the selected filter and use it here. + wxString defExt = + wildFilters[m_filterIndex].BeforeFirst(';').AfterFirst('.'); + if ( !defExt.empty() && defExt != wxS("*") ) + { + hr = fileDialog->SetDefaultExtension(defExt.wc_str()); + if ( FAILED(hr) ) + wxLogApiError(wxS("IFileDialog::SetDefaultExtension"), hr); + } + wxVector<COMDLG_FILTERSPEC> filterSpecs(nWildcards); for ( UINT n = 0; n < nWildcards; ++n ) { @@ -1588,16 +1602,6 @@ int wxFileDialog::ShowIFileDialog(WXHWND hWndParent) hr = fileDialog->SetFileTypeIndex(m_filterIndex + 1); if ( FAILED(hr) ) wxLogApiError(wxS("IFileDialog::SetFileTypeIndex"), hr); - - // We need to call SetDefaultExtension() to make the file dialog append - // the selected extension by default. It will append the correct - // extension depending on the current file type choice if we call this - // function, but won't do anything at all without it, so find the first - // extension associated with the selected filter and use it here. - wxString defExt = - wildFilters[m_filterIndex].BeforeFirst(';').AfterFirst('.'); - if ( !defExt.empty() && defExt != wxS("*") ) - fileDialog->SetDefaultExtension(defExt.wc_str()); } if ( !m_dir.empty() )
?
For me it changes nothing and the extension is still always appended unless a known (i.e. existing under HKCU) extension is already present.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
Do you have OneDrive enabled?
I have OneDrive installed on both Win 10 and 11 but I am not signed in on either.
I have tried again on Windows 11 and recorded the screen. I ran the code from my previous post, where I
- Put a breakpoint in
GetPathFromIFileDialog()(wxMSWImpl::GetFSPathFromShellItem()just obtains the path usingIShellItem::GetDisplayName(SIGDN_FILESYSPATH)). - Typed "test.blah" in File name and pressed Save.
- Inspected variable
pathwhen the breakpoint hit.
If I do exactly the same on Windows 10, the file name part of path is not "test.blah" but "test.blah.txt"
I am not normally using Win11, I did a cursory Settings tour but I did not find anything looking like it could affect this. So it may be just a VM-specific, though I have no idea how could that happen.
After typing all above, being desperate and remembering the recent dark mode issue while not believing this could affect this issue in any way; I still tried also the 32-bit build on Windows 11. Believe it or not, 32-bit build worked the same on Windows 11 as 64-bit build on Windows 10, i.e., differently than the 64-bit build on the same Windows 11 VM. O.O I tried switching (and rebuilding) between the builds, but 32- and 64-bit build still behaved differently. Both 32- and 64-bit wxWidgets were built with the latest MSVS 2022 solution as Debug Static.
I also tested both builds with CMake (with -DwxBUILD_SHARED=OFF), my code as patch to the minimal sample to make sure it is not something in my MSVS project. I could not reproduce it in this way. I then went back to the MSVC project where the issue was seen but before pointing it to CMake-built library, I ran it once again: The "issue" was gone, the code now behaves exactly same everywhere (i.e., "test.blah" -> "test.blah.txt")? Sigh.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
FWIW I did think there could be a difference between 32- and 64-bit builds and so I've tested with both -- but don't see the problem in 64-bit under Windows 11 neither. I used a DLL build, however, as I really didn't think it could make any difference. I can/will retry with a statically linked executable later, but it's really incredible that you can't reproduce it any longer -- if I understand you correctly? -- when you could do it previously.
If you do manage to catch it again, could you please try applying the patches above? If SetDefaultExtension() fails for some reason, this would explain the problem, and seeing the HRESULT could give a hint as to what this reason is.
Thanks again for your help!
@jpbreen85 Could you please confirm that you saw the problem under Windows 10 and not 11 originally?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
...but it's really incredible that you can't reproduce it any longer -- if I understand you correctly? -- when you could do it previously.
I have no explanation for this. The source code, project files, and libraries were the same, unmodified. It as if running the code from the cmake-built library and sample fixed it at the Windows level.
Coincidentally, the sample and my test app both had the same executable name (minimal.exe), but I tried to use a different name (and also removing the manifest), still works now.
It cannot be explained by a VM glitch either, between the yesterday and today, I have rebooted the VM several times and I also turned-off the VM host PC and could still reproduce. Until running the cmake-built executable...
At least I have the screen recording to prove I am not crazy or trolling.
If you do manage to catch it again, could you please try applying the patches above?
Sorry, my previous post took so long to write that you posted yours in the meantime, which I missed. I cannot reproduce anymore no matter what I try. I will try again tomorrow, but I am not hopeful.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
@PBfordev ,
Was there an update installed? with Windows 10+ it is automatic, so had to ask.
Thank you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev , Was there an update installed? with Windows 10+ it is automatic, so had to ask.
No, there was no update installed in February, according to Update History.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
I think I have solved the "mystery". It was not a mystery, it was my mistake, sorry.
I usually have several wxWidgets version installed and I switch between them by changing the folder in system environment variable WXWIN.
I was 100% sure I was testing on Windows 11 with the GIT master. But I must have tested with wxWidgets 3.2.1 first, i.e., initially built the test app with 3.2.1 and after switching WXWIN to the master, even if I think I used Rebuild command, I must have not and just used Build command, where the old version linking with 3.2.1 was not updated. I must have used Rebuild only after testing with CMake. This would also explain why the 32-bit build worked, since I built wxWidgets as 32-bit only after switching from 3.2.1 to the master, after having finished testing the 64-bit build.
When testing with multiple versions, I usually check the version at runtime by ctrl+alt+mdlclick, but the test app did not have a place to click, so I did not here (and I was sure I was using the master anyway).
The reason I think the above must have happened is because I can reproduce with 3.2.1 but not 3.2.2 or master, probably due to this post 3.2.1 change: 0a33da8
Once again, I am very sorry for the noise.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks for clearing this up and sorry for completely failing to realize that the original report was using 3.2.1 (which didn't have this fix yet) and that the other report I got was from a program using 3.2.0.
So this bug report is, finally, exactly the same as #22898 actually and so was already fixed in both master and 3.2.2.
I'm going to close it again and maybe open another one to add wxFD_ALWAYS_APPEND_EXTENSION later if we decide it's still necessary -- but now that the file dialog appends it itself again, it's much less critical.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #23193 as completed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.