Re: #3432: wxDir::GetAllFiles filespec problem

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:8>

#3432: wxDir::GetAllFiles filespec problem
------------------------+---------------------------------------------------
Reporter: mentat_kk | Type: defect
Status: new | Priority: normal
Milestone: | Component: wxMSW
Version: | Resolution:
Keywords: wxDir base | Blockedby:
Patch: 0 | Blocking:
------------------------+---------------------------------------------------

Comment(by oneeyeman):

Vadim,
This is a legitimate bug which I just confirmed by doing this:

C:\wxWidgets\samples\widgets\icons>dir *.xpm
Volume in drive C has no label.
Volume Serial Number is E48C-AC10

Directory of C:\wxWidgets\samples\widgets\icons

01/18/2013 05:50 PM 678 bmpbtn.xpm
01/18/2013 05:50 PM 1,556 bmpcombobox.xpm
01/18/2013 05:50 PM 1,551 button.xpm
01/18/2013 05:50 PM 1,553 checkbox.xpm
01/18/2013 05:50 PM 561 choice.xpm
01/18/2013 05:50 PM 1,553 choicebk.xpm
01/18/2013 05:50 PM 4,946 clrpicker.xpm
01/18/2013 05:50 PM 1,553 combobox.xpm
01/18/2013 05:50 PM 5,091 datepick.xpm
01/18/2013 05:50 PM 1,552 dirctrl.xpm
01/18/2013 05:50 PM 5,303 dirpicker.xpm
01/18/2013 05:50 PM 5,321 filepicker.xpm
01/18/2013 05:50 PM 4,824 fontpicker.xpm
01/18/2013 05:50 PM 1,550 gauge.xpm
01/18/2013 05:50 PM 1,553 hyperlnk.xpm
01/18/2013 05:50 PM 1,553 listbook.xpm
01/18/2013 05:50 PM 1,552 listbox.xpm
01/18/2013 05:50 PM 1,553 notebook.xpm
01/18/2013 05:50 PM 1,555 odcombobox.xpm
01/18/2013 05:50 PM 1,550 radiobox.xpm
01/18/2013 05:50 PM 1,547 scrolbar.xpm
01/18/2013 05:50 PM 1,551 slider.xpm
01/18/2013 05:50 PM 1,340 spinbtn.xpm
01/18/2013 05:50 PM 1,294 statbmp.xpm
01/18/2013 05:50 PM 1,552 statbox.xpm
01/18/2013 05:50 PM 1,553 stattext.xpm
01/18/2013 05:50 PM 1,549 text.xpm
01/18/2013 05:50 PM 4,997 timepick.xpm
01/18/2013 05:50 PM 1,551 toggle.xpm
29 File(s) 63,842 bytes
0 Dir(s) 11,869,315,072 bytes free

C:\wxWidgets\samples\widgets\icons>dir *.xpm1
Volume in drive C has no label.
Volume Serial Number is E48C-AC10

Directory of C:\wxWidgets\samples\widgets\icons

01/18/2013 05:50 PM 5,091 datepick.xpm1
1 File(s) 5,091 bytes
0 Dir(s) 11,869,315,072 bytes free

This from running "cmd" on Windows XP SP2.
And this means we should eliminate unneeded files with different
extension.

Will look into it.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:8>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:9>

#3432: wxDir::GetAllFiles filespec problem
------------------------+---------------------------------------------------
Reporter: mentat_kk | Type: defect
Status: new | Priority: normal
Milestone: | Component: wxMSW
Version: | Resolution:
Keywords: wxDir base | Blockedby:
Patch: 0 | Blocking:
------------------------+---------------------------------------------------

Comment(by oneeyeman):

But maybe all it needs is mentioning this in the documentation as it's
just too many possibilities exist.

Something like:

"wxDir::GetAllFiles() will return all matching file names. In case of the
matching pattern, like "*.bin" it will return "*.bin", "*.bin2", etc."

It is much easier to deal with situation like this in the user code as the
user knows which files (s)he is looking for.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:9>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:10>

#3432: wxDir::GetAllFiles filespec problem

-------------------------------+--------------------------------------------

Reporter: mentat_kk | Type: defect
Status: new | Priority: normal
Milestone: | Component: wxMSW
Version: | Resolution:

Keywords: wxDir base simple | Blockedby:
Patch: 0 | Blocking:
-------------------------------+--------------------------------------------
Changes (by vadz):

* cc: vadz (removed)
* keywords: wxDir base => wxDir base simple


Comment:

The simplest fix is certainly to check that the files returned by the
(buggy) CRT function do have the correct extension. This shouldn't be
difficult to do.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:10>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:11>

#3432: wxDir::GetAllFiles filespec problem
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: new | Priority: normal
Milestone: | Component: wxMSW

Version: 2.9-svn | Resolution:

Keywords: wxDir base simple | Blockedby:
Patch: 0 | Blocking:
-------------------------------+--------------------------------------------

Changes (by catalin):

* version: => 2.9-svn


Comment:

FindFirstFile and FindNextFile search "includes the long and short file
names", as [http://msdn.microsoft.com/en-
us/library/windows/desktop/aa364418(v=vs.85).aspx MS docs] say.
For a file named `test.bin2` the short name will be something like
`test~1.bin` so will be found when searching for `*.bin`.
Maybe this needs only a documentation update to mention that it includes
short names, or in case of a code change, that it does *not* include short
names.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:11>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:12>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too

-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect

Status: confirmed | Priority: normal

Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 0 | Blocking:
-------------------------------+--------------------------------------------

Changes (by vadz):

* status: new => confirmed


Comment:

Thanks for the explanation, this at least explains why does this happens,
so it's good to know.

But I think short names are an anachronism which nobody really uses any
more, so I'd still be for checking the returned file name with a simple
`wxString::Matches()` (this should be enough for Windows file patterns,
otherwise we also have `wxMatchWild()` in
source:wxWidgets/trunk/include/wx/filefn.h#L619).

IMO nobody expects to get "foo.binary" when searching for "*.bin", so no
code should be broken by this change.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:12>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:13>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:

Patch: 1 | Blocking:
-------------------------------+--------------------------------------------
Changes (by catalin):

* patch: 0 => 1


Comment:

Replying to [comment:12 vadz]:

> But I think short names are an anachronism which nobody really uses any
more, so I'd still be for checking the returned file name with a simple
`wxString::Matches()` (this should be enough for Windows file patterns,
otherwise we also have `wxMatchWild()` in
source:wxWidgets/trunk/include/wx/filefn.h#L619).

Patch attached using `wxMatchWild()` because its code looks better.
Otherwise they both had the same results.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:13>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:14>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by catalin):

It would be good if somebody can check the attached patch. I have some
time if it needs to be changed.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:14>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:15>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by vadz):

The code of `wxString::Matches()` appears to have been at least somewhat
optimized, so I think it should be faster (but I didn't measure it). More
importantly, looking at `wxMatchWild()` more carefully, I don't think it
does what we need here. First of all, it ignores anything starting with
dot by default. This could be disabled by passing it `false` for
`dot_special` argument but this isn't done by the patch. Second, it
handles backslashes in some way whereas I'm pretty sure that they're not
special at all for Windows here. Third, I'm not sure about whether "better
looking" should apply to the code which uses goto to jump inside an else
section of a conditional, this looks pretty gross. So I'd use
`wxString::Matches()` finally...

Other than that, why does the patch do `AfterLast('\\')`? It's not clear
why is this necessary exactly and it seems suspicious that it's called
from `FindFirst()` but not when `FindNext()` is called from `Read()`.

Also, for consistency, I'd add "spec" parameter before "finddata" in
`FindNext()` to use the same order as in `FindFirst()`.

And, finally, there should be no problem with moving
`GetNameFromFindData()` before `FindFirst()` to avoid the need to forward
declare it.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:15>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:16>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by catalin):

Replying to [comment:15 vadz]:

> I'm not sure about whether "better looking" should apply to the code
which uses goto to jump inside an else section of a conditional, this
looks pretty gross.

Oh, yes, I didn't notice it. Probably because I had already noticed the
goto in `wxString::Matches()`, though it doesn't look as bad.. That's a
surprising frequency of goto use..

> So I'd use `wxString::Matches()` finally...

Ok.

> Other than that, why does the patch do `AfterLast('\\')`?

That must be a leftover. I probably thought that `::FindFirstFile()` finds
the file including its path but it's not like that, it only gets the file
name.

> Also, for consistency, I'd add "spec" parameter before "finddata" in
`FindNext()` to use the same order as in `FindFirst()`.

Ok.

> And, finally, there should be no problem with moving
`GetNameFromFindData()` before `FindFirst()` to avoid the need to forward
declare it.

There is. In the current code only `GetNameFromFindData()` calls
`FindNext()`. After the changes both `FindFirst()` and `FindNext()` call
`GetNameFromFindData()`.

I'll update the patch with the above changes.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:16>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:17>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by catalin):

Patch updated. I've moved the functions around and it might be more
difficult to read.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:17>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:18>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: confirmed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution:
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by vadz):

Thanks, I'm applying this with some minor changes, please recheck them and
let me know if I broke anything:

1. Added `FreeFindData()` call to `FindFirst()` as I think we were
leaking the find handle otherwise in case of a search which was successful
for Windows but unsuccessful from our point of view.
1. Corrected check for `fd` validity in `FindFirst()`, it must be
compared with `INVALID_HANDLE_VALUE`, not 0 (and we already have a helper
function to do it).
1. Extracted the check into a separate function and check
`filter.empty()` there as I couldn't convince myself that it was always
non-empty even inside `FindNext()`.
1. Got rid of recursion in `FindNext()`, it just seems unnecessary.
1. Also got rid of `spec` argument there (idem).

Considering the complexity of this code I think it would be really great
to add a test for this case to `DirTestCase::Enum()` in
`tests/file/dir.cpp`. If you could please do it, it would be great. TIA!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:18>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:19>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect

Status: closed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution: fixed

Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Changes (by VZ):

* status: confirmed => closed
* resolution: => fixed


Comment:

(In [73790]) Check that files returned from wxDir::FindXXX() match the
filter.

Native Windows functions used by wxDir check the filter against both the
short
and the long name resulting in unexpected results, e.g. searching for
"foo.baz" would find "foo.bazaar".

Fix this by explicitly rechecking that we have a valid match ourselves.

Closes #3432.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:19>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:20>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: closed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution: fixed
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by catalin):

Replying to [comment:18 vadz]:

> I'm applying this with some minor changes

They all look fine to me.

> Considering the complexity of this code I think it would be really great
to add a test for this case to `DirTestCase::Enum()` in
`tests/file/dir.cpp`.

I've added a patch with a test case but to `DirTestCase::Traverse()`
because from what I could see `DirTestCase::Enum()` deals only with dirs..


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:20>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:21>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: closed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution: fixed
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by VZ):

(In [73836]) Add test for correct short/long file names in wxDir.

Verify that enumerating the files using the pattern *.foo doesn't match
*.foo.bar files, as it used to do under MSW.

See #3432.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:21>

[wxTrac]

Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:22>

#3432: wxDir::FindFirst/Next() unexpectedly match pattern against short file names
too
-------------------------------+--------------------------------------------
Reporter: mentat_kk | Type: defect
Status: closed | Priority: normal
Milestone: | Component: wxMSW
Version: 2.9-svn | Resolution: fixed
Keywords: wxDir base simple | Blockedby:
Patch: 1 | Blocking:
-------------------------------+--------------------------------------------

Comment(by VZ):

(In [74243]) Restore case-insensitivity for file name matching under
Windows.

This was broken by the changes of r73790, see #3432. Fix this by
converting
both the file name and the wildcard mask to the upper case before checking
whether the former matches the latter.

Closes #15243.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/3432#comment:22>