From 3181f1988d199f6aee23ebdf6146517640f7549e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 27 Sep 2023 15:33:32 +0200 Subject: [PATCH 1/7] Fix using *.* in wxDir under MSW Using "*.*" as a wildcard is supposed to match everything under MSW, but ever since the changes of 4daceaacbd (Check that files returned from wxDir::FindXXX() match the filter., 2013-04-08, see #3432) it only matched the files with extension because we double-checked the match returned by the native MSW function (which does handle "*.*" correctly) using our own wxString::Matches() which doesn't handle it specially. Fix this by skipping the call to Matches() when "*.*" is used: we know that this wildcard matches everything, so rechecking the match would be useless at best, even if it were not actively harmful. And also skip this call for "*" because calling Matches("*") always succeeds anyhow. This also fixes the same bug in wxFind{First,Next}File() and any other code using them such as wx{File,Dir}Ctrl. Closes #23905. --- src/msw/dir.cpp | 8 +++++++- tests/file/dir.cpp | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/msw/dir.cpp b/src/msw/dir.cpp index 5b05d23d48..691f732937 100644 --- a/src/msw/dir.cpp +++ b/src/msw/dir.cpp @@ -86,7 +86,13 @@ CheckFoundMatch(const FIND_STRUCT* finddata, const wxString& filter) // However if the filter contains only special characters (which is a // common case), we can skip the case conversion. if ( filter.find_first_not_of(wxS("*?.")) == wxString::npos ) - return fn.Matches(filter); + { + // And maybe even skip the check entirely if we have a filter that we + // know matches everything. This is more than just an optimization, as + // "*.*" it wouldn't match the files without extension according to + // wxString::Matches(), but it should. + return filter == wxS("*.*") || filter == wxS("*") || fn.Matches(filter); + } return fn.MakeUpper().Matches(filter.Upper()); } diff --git a/tests/file/dir.cpp b/tests/file/dir.cpp index ae5b81c5d5..5d11f80a52 100644 --- a/tests/file/dir.cpp +++ b/tests/file/dir.cpp @@ -20,6 +20,16 @@ #define DIRTEST_FOLDER wxString("dirTest_folder") #define SEP wxFileName::GetPathSeparator() +// We can't use wxFileSelectorDefaultWildcardStr from wxCore here, so define +// our own. +const char WILDCARD_ALL[] = +#if defined(__WXMSW__) +"*.*" +#else // Unix/Mac +"*" +#endif +; + // ---------------------------------------------------------------------------- // test fixture // ---------------------------------------------------------------------------- @@ -145,6 +155,9 @@ TEST_CASE_METHOD(DirTestCase, "Dir::Traverse", "[dir]") wxArrayString files; CHECK( wxDir::GetAllFiles(DIRTEST_FOLDER, &files) == 4 ); + // enum all files using an explicit wildcard + CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, WILDCARD_ALL) == 4); + // enum all files according to the filter CHECK( wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "*.foo") == 1 ); From 3f70912d7451b0c2ea50474d6c23b56b17538b0a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 29 Sep 2023 16:28:29 +0200 Subject: [PATCH 2/7] Remove __WINDOWS__ check from a Windows-only file No real changes, just remove a redundant __WINDOWS__ check. --- src/msw/dir.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/msw/dir.cpp b/src/msw/dir.cpp index 691f732937..f9ae2e089e 100644 --- a/src/msw/dir.cpp +++ b/src/msw/dir.cpp @@ -27,9 +27,7 @@ #include "wx/dir.h" -#ifdef __WINDOWS__ - #include "wx/msw/private.h" -#endif +#include "wx/msw/private.h" // ---------------------------------------------------------------------------- // define the types and functions used for file searching From f27688367a40cfa60fe8eba653f7f468405c1142 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 29 Sep 2023 16:35:07 +0200 Subject: [PATCH 3/7] Remove ugly macros and useless functions from wxMSW wxDir code No real changes, just remove trivial functions used to simply return a member of a struct and also the completely useless PTR_TO_FINDDATA macro and simply use the variable directly instead. --- src/msw/dir.cpp | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/msw/dir.cpp b/src/msw/dir.cpp index f9ae2e089e..7d175d3467 100644 --- a/src/msw/dir.cpp +++ b/src/msw/dir.cpp @@ -58,11 +58,6 @@ inline void FreeFindData(FIND_DATA fd) } } -const wxChar *GetNameFromFindData(const FIND_STRUCT *finddata) -{ - return finddata->cFileName; -} - // Helper function checking that the contents of the given FIND_STRUCT really // match our filter. We need to do it ourselves as native Windows functions // apply the filter to both the long and the short names of the file, so @@ -79,7 +74,7 @@ CheckFoundMatch(const FIND_STRUCT* finddata, const wxString& filter) // Otherwise do check the match validity. Notice that we must do it // case-insensitively because the case of the file names is not supposed to // matter under Windows. - wxString fn(GetNameFromFindData(finddata)); + wxString fn(finddata.cFileName); // However if the filter contains only special characters (which is a // common case), we can skip the case conversion. @@ -134,11 +129,6 @@ FindFirst(const wxString& spec, return fd; } -inline FIND_ATTR GetAttrFromFindData(FIND_STRUCT *finddata) -{ - return finddata->dwFileAttributes; -} - inline bool IsDir(FIND_ATTR attr) { return (attr & FILE_ATTRIBUTE_DIRECTORY) != 0; @@ -235,7 +225,6 @@ bool wxDirData::Read(wxString *filename) bool first = false; WIN32_FIND_DATA finddata; - #define PTR_TO_FINDDATA (&finddata) if ( !IsFindDataOk(m_finddata) ) { @@ -250,7 +239,7 @@ bool wxDirData::Read(wxString *filename) else filespec += m_filespec; - m_finddata = FindFirst(filespec, m_filespec, PTR_TO_FINDDATA); + m_finddata = FindFirst(filespec, m_filespec, &finddata); first = true; } @@ -269,9 +258,6 @@ bool wxDirData::Read(wxString *filename) return false; } - const wxChar *name; - FIND_ATTR attr; - for ( ;; ) { if ( first ) @@ -280,7 +266,7 @@ bool wxDirData::Read(wxString *filename) } else { - if ( !FindNext(m_finddata, m_filespec, PTR_TO_FINDDATA) ) + if ( !FindNext(m_finddata, m_filespec, &finddata) ) { DWORD err = ::GetLastError(); @@ -294,8 +280,8 @@ bool wxDirData::Read(wxString *filename) } } - name = GetNameFromFindData(PTR_TO_FINDDATA); - attr = GetAttrFromFindData(PTR_TO_FINDDATA); + const wxChar* const name = finddata.cFileName; + const FIND_ATTR attr = finddata.dwFileAttributes; // don't return "." and ".." unless asked for if ( name[0] == wxT('.') && From 22c629d6672f49793c650f4d4840eb4fa0728185 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 29 Sep 2023 16:36:47 +0200 Subject: [PATCH 4/7] Fix matching "x*.*" in wxMSW wxDir too Recent commit corrected handling of "*.*", which previously didn't match the files without extensions, but still left handling of patterns such as "x*.*" broken, because they are also supposed to match all files starting with "x" under Windows, whether they have an extension or not. Fix this by using PathMatchSpecEx() shell function which should handle this correctly and update the unit test to check for this case as well. --- src/msw/dir.cpp | 23 ++++++----------------- tests/file/dir.cpp | 5 ++++- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/msw/dir.cpp b/src/msw/dir.cpp index 7d175d3467..7254b6553c 100644 --- a/src/msw/dir.cpp +++ b/src/msw/dir.cpp @@ -28,6 +28,11 @@ #include "wx/dir.h" #include "wx/msw/private.h" +#include + +#ifdef __VISUALC__ + #pragma comment(lib, "shlwapi") +#endif // ---------------------------------------------------------------------------- // define the types and functions used for file searching @@ -71,23 +76,7 @@ CheckFoundMatch(const FIND_STRUCT* finddata, const wxString& filter) if ( filter.empty() ) return true; - // Otherwise do check the match validity. Notice that we must do it - // case-insensitively because the case of the file names is not supposed to - // matter under Windows. - wxString fn(finddata.cFileName); - - // However if the filter contains only special characters (which is a - // common case), we can skip the case conversion. - if ( filter.find_first_not_of(wxS("*?.")) == wxString::npos ) - { - // And maybe even skip the check entirely if we have a filter that we - // know matches everything. This is more than just an optimization, as - // "*.*" it wouldn't match the files without extension according to - // wxString::Matches(), but it should. - return filter == wxS("*.*") || filter == wxS("*") || fn.Matches(filter); - } - - return fn.MakeUpper().Matches(filter.Upper()); + return ::PathMatchSpecEx(finddata->cFileName, filter, PMSF_NORMAL) == S_OK; } inline bool diff --git a/tests/file/dir.cpp b/tests/file/dir.cpp index 5d11f80a52..b15fed7f89 100644 --- a/tests/file/dir.cpp +++ b/tests/file/dir.cpp @@ -22,7 +22,7 @@ // We can't use wxFileSelectorDefaultWildcardStr from wxCore here, so define // our own. -const char WILDCARD_ALL[] = +const wxString WILDCARD_ALL = #if defined(__WXMSW__) "*.*" #else // Unix/Mac @@ -158,6 +158,9 @@ TEST_CASE_METHOD(DirTestCase, "Dir::Traverse", "[dir]") // enum all files using an explicit wildcard CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, WILDCARD_ALL) == 4); + // enum all files using an explicit wildcard different from WILDCARD_ALL + CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "d" + WILDCARD_ALL) == 4); + // enum all files according to the filter CHECK( wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "*.foo") == 1 ); From e58fdf882ecffba2076bdfb335b1d63f74fd2b59 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 29 Sep 2023 17:01:39 +0200 Subject: [PATCH 5/7] Work around Wine PathMatchSpecEx() bug in the just added test Skip this test under Wine until the problem is fixed. See https://bugs.winehq.org/show_bug.cgi?id=55677 --- tests/file/dir.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/file/dir.cpp b/tests/file/dir.cpp index b15fed7f89..28967b5fdd 100644 --- a/tests/file/dir.cpp +++ b/tests/file/dir.cpp @@ -159,7 +159,16 @@ TEST_CASE_METHOD(DirTestCase, "Dir::Traverse", "[dir]") CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, WILDCARD_ALL) == 4); // enum all files using an explicit wildcard different from WILDCARD_ALL - CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "d" + WILDCARD_ALL) == 4); + // + // broken under Wine, see https://bugs.winehq.org/show_bug.cgi?id=55677 + if ( !wxIsRunningUnderWine() ) + { + CHECK(wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "d" + WILDCARD_ALL) == 4); + } + else if (wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "d" + WILDCARD_ALL) == 4) + { + WARN("PathMatchSpecEx() seems to work under Wine now"); + } // enum all files according to the filter CHECK( wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "*.foo") == 1 ); From d18a16c401c0508389f236467e98ddf1bb80ec3b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Sep 2023 16:30:33 +0200 Subject: [PATCH 6/7] Add an interactive test for PathMatchSpecEx() This test allows to compare the results of wxString::Matches(), wxMatchWild() and MSW native function for the same filter. --- tests/file/dir.cpp | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/file/dir.cpp b/tests/file/dir.cpp index 28967b5fdd..17d715b108 100644 --- a/tests/file/dir.cpp +++ b/tests/file/dir.cpp @@ -257,3 +257,45 @@ TEST_CASE_METHOD(DirTestCase, "Dir::GetName", "[dir]") CHECK( d.GetNameWithSep() == "/" ); #endif } + +// Disabled by default test allowing to check the result of matching against +// the given filter. +#ifdef __WXMSW__ + +#include "wx/msw/wrapwin.h" +#include +#ifdef __VISUALC__ + #pragma comment(lib, "shlwapi") +#endif + +#include "wx/crt.h" +#include "wx/filefn.h" + +TEST_CASE("Dir::Match", "[.]") +{ + wxString filter; + REQUIRE( wxGetEnv("WX_TEST_DIR_FILTER", &filter) ); + + static const wxString filenames[] = + { + "foo", + "foo.bar", + "foo.bar.baz", + ".hidden", + ".hidden.ext", + }; + + // Show the results of matching the pattern using various functions. + wxPrintf("%-15s %20s %20s %20s\n", + "File", "wxString::Matches", "wxMatchWild", "PathMatchSpecEx"); + for ( const auto& fn : filenames ) + { + wxPrintf("%-15s %20d %20d %20d\n", + fn, + fn.Matches(filter), + wxMatchWild(filter, fn), + PathMatchSpecEx(fn.wc_str(), filter.wc_str(), PMSF_NORMAL) == S_OK); + } +} + +#endif // __WXMSW__ From 5406a8f0c2500779a368e4af0805874e5fd927ac Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Sep 2023 23:20:16 +0200 Subject: [PATCH 7/7] Don't use newer PathMatchSpecEx() unnecessarily Use older and so hopefully compatible with all MinGW versions PathMatchSpec() instead as we don't use any of the newer function flags and it seems to behave identically with the default flag. --- src/msw/dir.cpp | 2 +- tests/file/dir.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/msw/dir.cpp b/src/msw/dir.cpp index 7254b6553c..f406a91281 100644 --- a/src/msw/dir.cpp +++ b/src/msw/dir.cpp @@ -76,7 +76,7 @@ CheckFoundMatch(const FIND_STRUCT* finddata, const wxString& filter) if ( filter.empty() ) return true; - return ::PathMatchSpecEx(finddata->cFileName, filter, PMSF_NORMAL) == S_OK; + return ::PathMatchSpec(finddata->cFileName, filter) == TRUE; } inline bool diff --git a/tests/file/dir.cpp b/tests/file/dir.cpp index 17d715b108..fa2b836900 100644 --- a/tests/file/dir.cpp +++ b/tests/file/dir.cpp @@ -167,7 +167,7 @@ TEST_CASE_METHOD(DirTestCase, "Dir::Traverse", "[dir]") } else if (wxDir::GetAllFiles(DIRTEST_FOLDER, &files, "d" + WILDCARD_ALL) == 4) { - WARN("PathMatchSpecEx() seems to work under Wine now"); + WARN("PathMatchSpec() seems to work under Wine now"); } // enum all files according to the filter @@ -287,14 +287,14 @@ TEST_CASE("Dir::Match", "[.]") // Show the results of matching the pattern using various functions. wxPrintf("%-15s %20s %20s %20s\n", - "File", "wxString::Matches", "wxMatchWild", "PathMatchSpecEx"); + "File", "wxString::Matches", "wxMatchWild", "PathMatchSpec"); for ( const auto& fn : filenames ) { wxPrintf("%-15s %20d %20d %20d\n", fn, fn.Matches(filter), wxMatchWild(filter, fn), - PathMatchSpecEx(fn.wc_str(), filter.wc_str(), PMSF_NORMAL) == S_OK); + PathMatchSpec(fn.wc_str(), filter.wc_str()) == TRUE); } }