#241 Research use of the new Common Item dialogs

[Ognyan Chernokozhev]

According to Microsoft's documentation, starting with Windows Vista, the Common Item Dialog supersedes the older Common File Dialog when used to open or save a file.

We should try to make use of these new dialogs for opening and saving files, and for browsing for folders (using the FOS_PICKFOLDERS flag).

[Ognyan Chernokozhev]

Attached are screenshots of the new file open dialog and the old file open dialog.

The most obvious difference is that the new dialog follows the system dark mode setting, while the old one does not. Also the new dialog displays the full path to the current directory.

[Ognyan Chernokozhev]

As a proof of concept, I copied the code from the Win32 samples in the TOWLMakerApp::CmOpen implementation to see how it works, but the code is quite complex and ugly. We need to design a better encapsulation in OWLNext.

[Vidar Hasfjord]

Good call — these dialogs are good candidates for modernisation.

Tip: To create new dialog classes, ask Bing Chat or other AI for help, including a relevant example of our code and coding standard.

[Ognyan Chernokozhev]

I am looking at the usage patterns of the file open and save dialogs, and in almost all cases it seems to be like this:

TFileOpenDialog dlg(parent, data);
int ret = dlg.Execute();

or even

int ret = TFileOpenDialog(parent, data).Execute();

In other words, the file open and save dialogs are treated like a function call, in which you pass the initialization data (file filters, flags, etc.) and get the selected file(s) as results.

There is no other interaction with the dialog object. The only exceptions to this straightforward usage that I can find are:

• The DocView open/save classes which were supposed to have special processing when the user changes the file type - but that does not seem to work, see https://sourceforge.net/p/owlnext/discussion/97175/thread/4b8ca81553/
• The Open Dialog example that derives from TFileOpenDialog and overrides the ShareViolation virtual method to demonstrate different behavior.

That is why I think there is no need to try to create a class that encapsulates the new Common Item dialogs, but instead to make public static methods like OpenFile, SaveFile and BrowseFolder. These methods may have a parameter that specifies whether to use the new style dialog, or the previous one.

[Vidar Hasfjord]

@jogybl wrote:

That is why I think there is no need to try to create a class that encapsulates the new Common Item dialogs, but instead to make public static methods like OpenFile, SaveFile and BrowseFolder.

As I see it, the main purpose of classes is to provide simple customisation. Although no internal code or code examples make use of this, it may be very useful for client code.

Indeed, my application Lima VVA customises the open and save dialogs for data import and export functionality. In particular, the custom dialogs provide an Option button, letting the user specify options for the data import/export procedure.

Happy new year!

[Ognyan Chernokozhev]

One advantage of using the new common open file dialog is that when multi-select is enabled, the number of files that can be selected is no longer limited by the size of a provided buffer.

[Vidar Hasfjord]

Hi Ognyan, great work on the support for the new dialogs!

I haven't closely reviewed your code yet, but it looks well written for the most part! However, I notice that you use TCommonFileDialogs just as a namespace for your new functions and data types. Do you plan on making it into a full-blown class from which object(s) can be instantiated? If not, inheriting from TDialog makes no sense.

Consider using a simple nested namespace (e.g. owl::CommonFileDialogs). Or better yet in my view, since the new functions already have long descriptive names, just put them directly in the owl namespace. You will then have to give the associated data types more descriptive names, though.

Please post your revisions here for simple review, reference and merging.

[Ognyan Chernokozhev]

If not, inheriting from TDialog makes no sense.

Good catch, it was a copy/paste error.

I have added a static bool flag that controls the default behavior of the methods - since it may be the only easy way to control the open/save dialogs used internally in OWLNext, for example in DocView.

I like the idea of nested namespace, it will still be grouping the functions and related data types and flags.

[Ognyan Chernokozhev]

Please review the latest changes and fixes.

[Vidar Hasfjord]

@jogybl wrote:

I have added a static bool flag that controls the default behavior of the methods - since it may be the only easy way to control the open/save dialogs used internally in OWLNext, for example in DocView.

If you want to avoid nasty globals, don't be afraid to look at adding DocView template flags, members to TDocManager or TApplication.

In any case, on naming convention, start the names of flags and boolean variables with a modal verb that has boolean association such as "Should", "Has", "Can", "Will", "Must", etc.

[Ognyan Chernokozhev]

It is not only the DocView. There are several places where OWLNext creates open or save dialogs - TEditFile, TCoolEditFile.

I am envisioning such setting that would enable applications to have a setting - whether exposed in the UI, or controlled via ini file or registry, that will allow the users to choose whether to use the new or old dialogs.

[Vidar Hasfjord]

@jogybl wrote:

I am envisioning [a global setting] that will allow the users to choose whether to use the new or old dialogs.

I see, a global flag makes sense for a truly global setting. However, TApplication is effectively a singleton, so it could house such a setting, I suppose. But, then again, it might not make much or any difference.

(Note that in the 16-bit era putting the global setting in the DLL would be wrong, since the DLL instance in memory could serve more than one application. Since 32-bit Windows, each application has a separate instance of the DLL in memory, so now the design choice is merely aesthetic.)

I will review your changes in more detail later. So far it looks good!

[Vidar Hasfjord]

@jogybl wrote:

Please review the latest changes and fixes.

I have now briefly reviewed your code:

• BUG: COM objects are not reliably released ("TODO: Use CComPtr...").
• BUG: ShowFileOpenDialog/ShowFileSaveDialog: IDOK/IDCANCEL returned as bool.
• ShowOpenSaveDialog: Unsafe _tcscpy calls (potential buffer overrun).
• GetFilterStrings: May read beyond the filter buffer, if corrupted (consider safe alternative).
• BrowseForFolder: For consistency, use "FAILED" rather than "!SUCCEEDED".
• ShowOpenSaveDialog: Nested and repeated code (consider simplifying).
• Prefer 'int' for indexes and sizes.
• Generally, don't use "\n" in exception messages (the handler should do the formatting, if any).
  
  

Other than that, it looks pretty good!

[Ognyan Chernokozhev]

GetFilterStrings: May read beyond the filter buffer, if corrupted (consider safe alternative).

It uses similar code as in TOpenSaveDialog::TData::SetFilter
What is a safer alternative?

[Ognyan Chernokozhev]

I have added usage of CComPtr for exception safety. Unfortunately, it turned out that C++ Builder no longer supports ATL (since XE apparently), so I had to copy the template definitions from VC++.

[Vidar Hasfjord]

@jogybl wrote:

What is a safer alternative [implementation of GetFilterStrings]?

The issue is that the function doesn't know the size of the buffer it is accessing. Passing the size to the function in some way would allow you to write a safe implementation that never reads beyond the buffer, even if the content is corrupted. See std::string_view.

[Ognyan Chernokozhev]

That would require some refactoring of the handling of TOpenSaveDialog::TData::Filter, at the very least additionally store the length of the buffer. And considering it is a public field, there is easy way to ensure that the user does not modify it directly, thus making the length field unreliable.

Future code should use the new CommonFileDialogs::TData object and the way more safer AddFilter() method.

[Vidar Hasfjord]

@jogbybl wrote:

That would require some refactoring

Agree. It is a wider issue with the code base, not particular to your new code. But worthwhile to think about for improving safety and robustness going forward.

[Vidar Hasfjord]

By the way, in modern C++ it is now recommended to not use new and delete.

COMDLG_FILTERSPEC* filterSpecs = new COMDLG_FILTERSPEC[data.Filter.size()];
for (unsigned int i = 0; i < data.Filter.size(); ++i)
{
  filterSpecs[i].pszName = data.Filter[i].Name.c_str();
  filterSpecs[i].pszSpec = data.Filter[i].Spec.c_str();
}

// Set the file types to display only. Notice that, this is a 1-based array.
hr = pfd->SetFileTypes(static_cast<UINT>(data.Filter.size()), filterSpecs);

delete[] filterSpecs;

Instead, you could do something like this:

// Set the file types to display.
//
auto fs = vector<COMDLG_FILTERSPEC>{};
transform(begin(data.Filter), end(data.Filter), back_inserter(fs),
  [&](const auto& f) { return COMDLG_FILTERSPEC{f.Name.c_str(), f.Spec.c_str()}; });
hr = pfd->SetFileTypes(static_cast<UINT>(size(fs)), data(fs));

[Ognyan Chernokozhev]

Great suggestion, thanks.
Implemented and tested in [r6808].

[Vidar Hasfjord]

@jogybl wrote:

Unfortunately, it turned out that C++ Builder no longer supports ATL (since XE apparently), so I had to copy the [CComPtr] template definitions from VC++.

I was actually discovering the same thing and looking at solutions! It turns at that WRL has a similar ComPtr (used in the WebView example), but Embarcadero doesn't have "wrl/client.h" either, so it is not an option, sadly.

However, we don't need the full functionality of ComPtr, so we can make our own, using std::unique_ptr with a custom deleter. Here is a simple definition and usage:

template <typename T>
struct TComDeleter
{
  void operator()(T* ptr) const
  {
    if (ptr)
      ptr->Release();
  }
};

template <class T>
using TComPtr = unique_ptr<T, TComDeleter<T>>;

// Usage:

auto psiResult = [&]      
{
  IShellItem* p = nullptr;
  const auto hr = psiaResults->GetItemAt(dwIndex, &p);
  if (FAILED(hr)) throw runtime_error{"IShellItemArray::GetItemAt failed: " + to_string(hr)};
  return TComPtr<IShellItem>{p};
}();

You can also use a unique_ptr to manage the display name string:

auto pszFilePath = [&]
{
  PWSTR p = nullptr;
  const auto hr = psiResult->GetDisplayName(SIGDN_FILESYSPATH, &p);
  if (FAILED(hr)) throw runtime_error{"IShellItemArray::GetDisplayName failed: " + to_string(hr)};
  using TDisplayNamePtr = unique_ptr<WCHAR, decltype(&CoTaskMemFree)>;
  return TDisplayNamePtr{p, &CoTaskMemFree};
}();

data.FileNames.push_back(to_tstring(pszFilePath.get()));

And you can circumvent the problem with Embarcadero refusing to compile your lambda callback by using a static_cast (thereby eliminating your workaround TLocal::BrowseCallbackProc):

bi.lpfn = static_cast<int (CALLBACK*)(HWND, UINT, LPARAM, LPARAM)>(
  [](HWND w, UINT msg, LPARAM, LPARAM lpData) -> int
{
  if (msg == BFFM_INITIALIZED)
    ::SendMessage(w, BFFM_SETSELECTION, TRUE, lpData);
  return 0;
});

(Thanks to Bing Chat for this solution.)

Couple of other suggestions:

• Split up ShowOpenSaveDialog and factor out the common functionality as helper functions.
• Use TXOwl rather than std::runtime_error.
• Use nullptr rather than NULL.
• Don't use Hungerian notation. Follow the OWLNext coding convention.

[Ognyan Chernokozhev]

And you can circumvent the problem with Embarcadero refusing to compile your lambda callback by using a static_cast (thereby eliminating your workaround TLocal::BrowseCallbackProc):
...
(Thanks to Bing Chat for this solution.)

Still gives a compilation error. Maybe BingChat has not used Embarcadero C++ Builder :-)

..\source\owlcore\commonfiledialogs.cpp(447,17): error E4352: cannot cast from type '(lambda at ..\..\source\owlcore\commonfiledialogs.cpp:448:9)' to pointer type 'int (*)(HWND, UINT, LPARAM, LPARAM) __attribute__((stdcall))' (aka 'int (*)(HWND__ *, unsigned int, long, long) __attribute__((stdcall))') [C:\Work\OWLNext\Subversion\trunk\build\embarcadero\owl.cbproj]

[Vidar Hasfjord]

@jogybl wrote:

Still gives a compilation error.

It looks specific to version 12 Athens then. I don't have the latest version. I still use 11 Alexandria Community Edition. And with this version, the lambda actually compiles fine with or without the cast:

bi.lpfn = [](HWND w, UINT msg, LPARAM, LPARAM lpData) -> int
{
  if (msg == BFFM_INITIALIZED)
    ::SendMessage(w, BFFM_SETSELECTION, TRUE, lpData);
  return 0;
};

[Ognyan Chernokozhev]

I am using Alexandria 11 as well on that machine, and the code did not compile :(

[Vidar Hasfjord]

@jogybl wrote:

Maybe BingChat has not used Embarcadero C++ Builder :-)

Actually, to be fair to Bing Chat, what I asked was: "Where do I put CALLBACK here?". It clarified that the lambda syntax does not support syntax for this vendor specific attribute, but that a workaround would be to use a cast. So, full understanding, top marks!