#36 CFileDialog not working with OFN_ALLOWMULTISELECT flag

[Claudio Nicora]

Hi David, I have an issue with a CFileDialog with OFN_ALLOWMULTISELECT flag in Win32++ 8.4 (I've marked this bug report 8.2 because there's no 8.4 tag).

This is an excerpt of my code:

// OpenFile dialog
CFileDialog dlg(
    TRUE,               // open file dialog
    NULL,               // extension
    NULL,               // current open file path
    OFN_ALLOWMULTISELECT | OFN_EXPLORER | OFN_FILEMUSTEXIST | OFN_PATHMUSTEXIST | OFN_ENABLESIZING,  // flags
    L"Text files (*.txt)\0*.txt\0All files\0*.*\0\0"
);

// select input file
if (dlg.DoModal(*this) != IDOK) {
    return 1;
}

// retrieve all filenames
CString file;
int pos = 0;
while (pos >= 0) {
    file = dlg.GetNextPathName(pos);
    // do things with this filename...
}

When I select more than one file, calls to GetNextPathName() return partial filenames with trailing garbage.

Looking at CFileDialog source I found something strange in CFileDialog::DoModal():625

m_OFN.lpstrFile = m_sFileName.GetBuffer(m_OFN.nMaxFile);
int ok = (m_IsOpenFileDialog ? ::GetOpenFileName(&m_OFN) : ::GetSaveFileName(&m_OFN));
m_sFileName.ReleaseBuffer();
m_OFN.lpstrFile = const_cast<LPTSTR>(m_sFileName.c_str()); // <--- this removes everything after first \0

If flag OFN_ALLOWMULTISELECT is active, the returned file list is a \0 separated string terminated with a double \0\0.
Don't know what's the meaning of last line, anyway .c_str() truncates the buffer and retargets m_OFN.lpstrFile pointer on a wrong one.

I've actually patched it like this and it works:

if (!(m_OFN.Flags & OFN_ALLOWMULTISELECT)) {
    m_OFN.lpstrFile = const_cast<LPTSTR>(m_sFileName.c_str());
}

To stay on the safest side I execute the cast only if OFN_ALLOWMULTISELECT flag is not active, because I don't know if it's really needed by calls other than GetNextPathName().
Hope you can suggest a better fix...;)

[David]

Hi Claudio,

Thanks for bringing this to my attention. I've updated the code to handle the OFN_ALLOWMULTISELECT flag properly.

        m_OFN.hwndOwner = hWndOwner;
        m_OFN.lpstrFile = m_sFileName.GetBuffer(m_OFN.nMaxFile);
        int ok = (m_IsOpenFileDialog ? ::GetOpenFileName(&m_OFN) : ::GetSaveFileName(&m_OFN));
        m_sFileName.ReleaseBuffer(m_OFN.nMaxFile);   // <-- modified
        m_OFN.lpstrFile = const_cast<LPTSTR>(m_sFileName.c_str());
        m_hWnd = 0;

The m_OFN.nMaxFile parameter is now also larger if the OFN_ALLOWMULTISELECT flag is used.

You can download the updated code as a snapshot in the code section.

Best regards,
David

[Claudio Nicora]

Thanks for fixing it (I haven't thought about .ReleaseBuffer(...) call).

Now there should be a way for the caller to "hint" for the expected maximum number of selected files, based on specific usage.
Actually it's hard coded to 256 files, but sometimes this could not be enough; think about a folder full of pictures or mp3s where the user could easily select them all with a CTRL+A.

• (easy) expose m_OFN.nMaxFile through a method or adding another optional parameter to DoModal().
• (hard) add some kind of "dynamic buffer" with a hook as explained on the following CodeProject article:
  https://www.codeproject.com/Articles/3235/Multiple-Selection-in-a-File-Dialog
  beware: read carefully the comments of the article because there could be some issues starting from Win7
• (hardest) one of the commenters of the article above, added class CMultiFileDialog to WTL; to me it seems the more complete one:
  https://sourceforge.net/p/wtl/code/HEAD/tree/trunk/wtl/Include/atldlgs.h
  .
  Thanks for your work
  Claudio

[Claudio Nicora]

Another suggestion: I've changed your hardcoded nMaxFile value up to 10000*MAX_PATH and, after that, calls to GetNextPathName() became awfully slow because of the last line below:

inline CString CFileDialog::GetNextPathName(int& pos) const
//  Return the next file path name from a group of files selected. The
//  OFN_ALLOWMULTISELECT flag allows multiple files to be selected. Use pos = 0
//  to retrieve the first file. The pos parameter is updated to point to the
//  next file name. The pos parameter is set to -1 when the last file is retrieved.
{
    assert(pos >= 0);

    BOOL IsExplorer = m_OFN.Flags & OFN_EXPLORER;
    TCHAR chDelimiter = (IsExplorer ? _T('\0') : _T(' '));

    CString strFile(m_OFN.lpstrFile + pos, m_OFN.nMaxFile); // strFile can contain NULLs

The second parameter should be m_OFN.nMaxFile - pos to avoid copying always m_OFN.nMaxFile bytes.
But also with that change it's really slow, not related with the selected number of files but only with buffer size.

Since CString strFile is only used to search and extract chars from buffer, maybe it's better to replace it with plain string function calls and act directly on m_OFN.lpstrFile buffer without copy.

EDIT: fixed issue description

[David]

Hi Claudio,

We can use the SetParameters function to specify a larger buffer size before calling DoModal. You can set the nMaxFile parameter to 10000*MAX_PATH if you wish.

I've updated the code to improve the performance as follows:

inline CString CFileDialog::GetNextPathName(int& pos) const
{
    assert(pos >= 0);

    BOOL IsExplorer = m_OFN.Flags & OFN_EXPLORER;
    TCHAR chDelimiter = (IsExplorer ? _T('\0') : _T(' '));

    int bufferSize = MIN(_MAX_PATH, m_OFN.nMaxFile - pos);
    CString strFile(m_OFN.lpstrFile + pos, bufferSize); // strFile can contain NULLs

Best regards,
David

[Claudio Nicora]

Sorry, I've missed Get/SetParameters().
Since other options, like Filter, have their own SetFilter() setter, I thought a buffer setter method could fit to avoid get-customize-set steps; anyway this is perfect for my needs.

Keep up the good work.
Claudio

[Claudio Nicora]

I'm sorry but now GetNextPathName() doesn't work properly and returns only the first filename.
pos parameter is immediately set to -1 after the first call...

I've double-checked if my extended buffer size is set correctly (10000 * MAX_PATH).
It seems that the introduced string copy buffer optimization prevents detecting other files...

[David]

Hi Claudio,

Perhaps the OFN_ALLOWMULTISELECT flag was removed when you called SetParameters. This code displays muiltiple files.

CFileDialog dlg(
    TRUE,               // open file dialog
    NULL,               // extension
    NULL,               // current open file path
    OFN_ALLOWMULTISELECT | OFN_EXPLORER | OFN_FILEMUSTEXIST | OFN_PATHMUSTEXIST | OFN_ENABLESIZING,  // flags
    L"Text files (*.txt)\0*.txt\0All files\0*.*\0\0"
);

OPENFILENAME ofn = dlg.GetParameters();
ofn.nMaxFile = 100000 * MAX_PATH;
dlg.SetParameters(ofn);

// select input file
if (dlg.DoModal(*this) != IDOK) {
    return 1;
}

// retrieve all filenames
CString file;
int pos = 0;
while (pos >= 0) {
    file = dlg.GetNextPathName(pos);
    Trace(file); Trace("\n");
}

Best regards,
David

[Claudio Nicora]

Perhaps the OFN_ALLOWMULTISELECT flag was removed when you called SetParameters.

No it wasn't, otherwise I won't be able to select more than one file.

It's an unlucky combination of path and filename length that lead a subtle bug to happen.
Let me try to explain:
My test folder is C:\Test and it contains 8 files: 01.txt, 02.txt, ..., 08.txt.
I tried your sample code and... it had the same issues as mine, returning only the first selected filename.

After banging my head against the wall ;) and trying everything possible, I renamed C:\Test to C:\TestFolder and BANG, it worked.

Digging into code I (probably) found the culprit in GetNextPathName() (wxx_commondlg.h:742):

if (strFile.GetAt(nFileLen + 1) == _T('\0'))

which should be like this:

if (strFile.GetAt(Index + nFileLen + 1) == _T('\0'))

With that particular combination of short path (C:\Test) and filename (01.txt) the test was successful, so GetNextPathName() returned pos = -1.

[David]

Thanks Claudio.

Your suggested change is correct. I've commited the change to SourceForge.

Best regards,
David

[Claudio Nicora]

Great, thanks for your help.
Will use the updated file waiting for 8.5 release.

Cheers
Claudio