Opened 3 months ago

Closed 3 weeks ago

#18736 closed enhancement (fixed)

Allow multiple selection of folders in wxDirDialog

Reported by: david_mtl Owned by: Vadim Zeitlin <vadim@…>
Priority: normal Milestone:
Component: GUI-all Version: dev-latest
Keywords: wxDirDialog multiple selection Cc: ian.s.mcinerney@…
Blocked By: Blocking:
Patch: yes

Description

This is a feature request.

Allow the selection of multiple folders within wxDirDialog. Could be done trough the flag wxFD_MULTIPLE like wxFileDialog.

This feature is available on Mac/Windows/Linux:

Win32:
https://docs.microsoft.com/en-us/windows/win32/api/shobjidl_core/nf-shobjidl_core-ifiledialog-setoptions
flag used: FOS_ALLOWMULTISELECT and FOS_PICKFOLDERS

Mac AppKit:https://developer.apple.com/documentation/appkit/nsopenpanel/1530786-allowsmultipleselection

Gtk:
FileChooserDialog with flag SELECT_FOLDER
https://valadoc.org/gtk+-3.0/Gtk.FileChooserDialog.html
FileChooser (parent class of FileChooserDialog)
https://valadoc.org/gtk+-3.0/Gtk.FileChooser.set_select_multiple.html

Change History (18)

comment:1 Changed 3 months ago by imcinerney

  • Cc ian.s.mcinerney@… added

I actually have started implementing this a while ago (https://github.com/imciner2/wxWidgets/tree/im/dirdialog) along with the hidden flag (and a small rewrite of the OSX dialog to use non-deprecated functions). It is just missing the Windows dialog changes though.

I have been hoping to get back to it sometime, but I haven't had a chance to yet (hopefully in the next few weeks).

Last edited 3 months ago by imcinerney (previous) (diff)

comment:2 Changed 3 months ago by pb101

  • Cc pbfordev@… added
  • Keywords features folders removed
  • Version changed from 3.1.3 to dev-latest

As for wxDirDialog on MSW: Windows 7+ use IFileDialog while the older versions use SHBrowseForFolder().

I wrote the IFileDialog-based code and seeing this ticket, I used that code to write a proof of concept for multiple selection implementation (not extensively tested yet), see here.

It should be trivial to adapt it so it could be merged into wxWidgets codebase (i.e., I would do it), if the maintainers give green light to adding this feature.

However, at first glance it appears that SHBrowseForFolder() does not allow retrieving multiple folders, but I may be wrong: TBH, I did not look hard. Similarly, I do not use Linux but the link in the OP has GTK3 in it, is the API supported on GTK2 as well?

comment:3 follow-up: Changed 3 months ago by imcinerney

  • Cc ian.s.mcinerney@… removed

According to the docs, the API for multiple selection was added in GTK 2.4 (I was also adding the hidden selection option which is GTK 2.6).

As for Windows, I was planning on doing the IFileDialog interface and marking it as a Windows Vista+ feature (I am not well versed in the Win32 API, so even doing this is somewhat of a stretch for me).

comment:4 in reply to: ↑ 3 Changed 3 months ago by pb101

Replying to imcinerney:

As for Windows, I was planning on doing the IFileDialog interface and marking it as a Windows Vista+ feature

wxWidgets does not use IFileDialog on Vista due to this bug.

However, multiple selection requires using IFileOpenDialog instead of IFileDialog; so who knows, perhaps it works there (or the bug was fixed in the meantime even for IFileDialog)...

comment:5 Changed 3 months ago by imcinerney

  • Cc ian.s.mcinerney@… added

Yea, I meant the IFileOpenDialog (it has been a while since I looked at this). From my reading, I don't think this is possible with the old style file dialogs, so this would have to be only on the new style (which I guess would be Windows 7+ then). I don't see a way around having this restriction, and there are some other items that require Windows 7+ already (such as the wxGA_PROGRESS style inside wxGauge and the wxAppProgressIndicator), so there is precedent for adding features with that minimum version.

It should be fairly easy to update it so that the change is transparent to the library users - and all that happens is that the new select multiple and select hidden flags are ignored.

comment:6 Changed 3 months ago by pb101

Replying to imcinerney:

I don't see a way around having this restriction

I wonder if there should be something like

bool wxDirDialog::IsMultipleSelectionAvailable();

so the programmer knows whether multiple selection is available at runtime.

Anyway, I think now we should wait what the maintainers have to say about adding this feature, and if they agree then decide on the public API (probably similar to wxFileDialog and what you already have).

comment:7 Changed 3 months ago by vadz

  • Component changed from GUI-generic to GUI-all
  • Status changed from new to confirmed

Please don't wait for the maintainers :-)

The feature would obviously be useful, so any contributions are welcome. If it can't be supported on all platforms, it just won't be supported there, i.e. the dialog will always be limited to a single selection -- which won't be worse than now, so it's not a blocker.

Finally, if we really need a function for checking for support of this feature, let's make it IsMultipleSelectionSupported(wxString* reason = NULL) for consistency with the existing wxWindow::IsTransparentBackgroundSupported(). It could also be static, but it probably doesn't matter much, as you'd typically have a wxDirDialog object at hand anyhow when you'd want to use it.

TIA!

comment:8 Changed 3 months ago by pb101

I have created a preliminary MSW implementation, see here
https://github.com/wxWidgets/wxWidgets/commit/ad102de8766a4fa571158d94c88ed82e9b828c08

I wanted to do a Draft PR against Ian's branch but before I pulled Ian's branch, my master was based on the current master, so a PR would pull in all the commits not in Ian's branch. I am not friends with GIT, so I did not know what to do and just left it in my branch.

Adding support for multiple selection made the original function way too long so I decided to split it into several ones. This unfortunately makes the diff less readable. If you wish, I can merge it into one again (but IMO it was really long and had too many nested scope levels).

Additionally, I have few questions:

  1. Does wxWidgets 3.1.4 support compilers not having correct Vista headers and libraries? If so, IFileOpenDialog, IShellItemArray and related need to be added, it seems there's quite a few of those.
  1. I believe that calling GetPaths() on a wxDirDialog not having wxDD_MULTIPLE flag set is a programmer error and as such should be caught with wxCHECK. If you disagree I will remove the check, but if you agree the analogical check should be then added in GetPath().
  1. I wonder if the error message should be changed from "Couldn't obtain folder name" to "Couldn't obtain folder name(s).". This would be more technically correct but would require translating this string in all the languages.
  1. I noticed that IFileDialog in wxDirDialog was created without FOS_NOCHANGEDIR (wxWidgets changes the CWD when wxDD_CHANGE_DIR is set by themselves, does nothing otherwise); however, it seems that the CWD is not changed anyway. Not sure if this option should be added now....
  1. Speaking of wxDD_CHANGE_DIR: When multiple selection is allowed what should be the CWD changed to? Ian's GTK code uses the last selected folder, is this the most logical solution? Maybe just using wxDD_CHANGE_DIR together with wxDD_MULTIPLE does not make much sense? Whichever solution is used, it should be documented.
Last edited 3 months ago by pb101 (previous) (diff)

comment:9 Changed 3 months ago by vadz

  • Patch set

Thanks! The code looks fine to me (it might be worth adding a smart pointer calling CoTaskMemFree() in its dtor, but this is not really related to this patch) and I probably could combine it with Ian's branch myself, but I don't know if I should do it or let Ian do it when he finishes with the other changes. Ian, please let me know!

As for the questions:

  1. Splitting the function is perfectly fine, ideal would have been to separate the changes in 2 commits, the first one splitting it without making any other changes and the second one doing the real changes, but it's more of a note for the future than anything else.
  1. IFileOpenDialog is defined in MinGW-w64 since basically always (I see it in 4.7), but plain MinGW-w32 still doesn't have it even in 8.2. We still are supposed to support it, but I don't know if it's worth continuing to do it... If we can just disable this code for it, I'm 100% fine with it. If we have to drop support for it, I'm probably fine with ii too, we can't keep defining all COM interfaces later than 2000 by hand forever.
  1. I agree with this, thanks.
  1. The best would be to use wxPLURAL, the new phrase would need to be translated, of course, but at the very least the old one would still be translated too, so it won't be a regression. And, of course, using wxPLURAL results in more grammatically correct sentence.
  1. I think we should use FOS_NOCHANGEDIR, this should be consistent with the other platforms and changing CWD after showing a dialog is really unexpected. In fact, it really looks just like a bug.
  1. I'm perfectly fine with disallowing the use of wxDD_CHANGE_DIR with wxDD_MULTIPLE, it doesn't seem to really make sense. The only problem is that it does work for files, but then all the chosen files are in the same directory (AFAIK?), so it's a bit different.

Thanks again!

comment:10 Changed 3 months ago by imcinerney

Thanks for looking at the Windows part of this! I am not versed in the win32 APIs, so that was actually where I had got stuck/left off in my implementation (my initial attempts were full of null-pointer referencing when parsing the returned paths, and at the time I didn't know what was causing them, so I took a break from fighting with it).

I will rebase my branch and then put my GTK and OSX changes into a PR this week.

@vadz, if you want to make changes to the MSW portions, go ahead. As I said, I don't really know what I am doing in that code currently. I can make the pull request then you can add those changes into it so that we can look at everything at once.

Some comments about the points in comment 8:

2) I had been trying to make my API match the API for the wxFileDialog. In that one, it is perfectly fine to call GetPaths or GetPath for any combination of the wxFD_MULTIPLE flag - it will just either return one or multiple (if the select multiple flag isn't set, then the two functions return the same thing just in a different container, if it is set then the GetPath function only returns one path while GetPaths returns all the selected paths). I think consistency between these two APIs would be desirable, so what we do in one should be mirrored in the other.

5) This would probably make sense. It is fairly easy to add some checks for this in the constructor to ensure they aren't enabled at the same time. (I would assume this can just be done with wxCHECKs, or do we want some sort of graceful handling of the error?)

comment:11 Changed 3 months ago by pb101

Vadz, thanks for the quick reply.

Ian or vadz, please do not do anything with my commit yet, there will be changes based on the things below.

1. MinGW32 support
Is there a precedent for wxWidgets features not being available specifically for MinGW32? I'd rather not do that, but it seems that IShellItemArray may require some other declarations so I am not thrilled at the possibility of having to add all of those (but perhaps it is not as bad as it looks). If you decide that supporting the compilers with severaly outdated headers is not mandatory, what is the best way to check?
For example, something like

#if defined(__CYGWIN__) || defined(__MINGW32__)

or just checking for

if defined(__IFileOpenDialog_INTERFACE_DEFINED__)

which may automagically start working with MinGW32 if they add the declaration? However, as the experience taught us, having just the interface class declaration may not be enough, sometimes a GUID or a function in a library is still missing. :(

2. GetPath()/GetPaths() vs DD_MULTIPLE
I expressed my opinion before, Ian disagrees and presents his arguments. The decision needs to be made.

3. Using wxPLURAL() for reporting the error
Is it a good fit here? We do not have an actual number, all we know is whether the programmer allowed selecting multiple folders or not. There seems to be no error reporting to the user in wxDirDialog code either in OSX or GTK implementations, so I could not take a hint from there. FWIW, the only error message in the MSW code for wxFileDialog seems to be generic "File dialog failed with error code %0lx.".

4. Implementing wxDD_SHOW_HIDDEN on MSW
I somehow managed to miss that this style was introduced as well. There is FOS_FORCESHOWHIDDEN option for IFileDialog so it should be simple to implement but I have not tried this yet.

4. Missing FOS_NOCHANGEDIR
I am sorry for not being clear, I mean that the CWD does NOT seem to get changed despite this option missing, so there is not a bug there. But I would still add the option: it seems correct and should not break anything.

5. Handling failure to convert a IShellItem to path with DD_MULTIPLE
If an item could not be converted, I bail out and do not return any items at all. I wonder if this is the best solution, perhaps ignoring this failure and return at least those items which could be converted would be better. However, there is no way to communicate back to the programmer that only a partial item list was obtained.

As always, sorry for the wall of text and thanks for your patience.

comment:12 Changed 3 months ago by vadz

Ian, just to be clear, if you can integrate all the changes and make a PR, this would be great, I don't plan to make any changes until then, I might do something after/when merging this PR I'll gladly leave creating it to you.

PB:

  1. I thought we did disable some features for MinGW32, but I can't find anything right now. Even more surprisingly, wxCHECK_W32API_VERSION() doesn't seem to be used neither. And wxCHECK_MINGW32_VERSION() is only used in a single place and doesn't really disable any functionality. But I still think we can disable this for them and in this case checking for __IFileOpenDialog_INTERFACE_DEFINED__ seems reasonable, if we run into any problems at link time, we'll fix them when we find them.
  1. Hmm, it's true I forgot about this wxFileDialog behaviour. In isolation, I definitely would prefer to assert, but API consistency is important. Also, calling GetPaths() for single-selection case is not too bad, it's calling GetPath() for multiple selection case which is really problematic and, AFAICS, should never be what you really want to do. So I'd suggest allowing GetPaths() in all cases and add an assert to GetPath() (and GetFilename[s]() too, of course, for consistency) for both classes. This would have to be mentioned in the "incompatible changes" section of the changelog, of course.
  1. Sorry, I didn't look at the code carefully enough and didn't realize we didn't have the number of directories. I guess we could use HasFlag(wxDD_MULTIPLE) ? 2 : 1 or something, but it's probably not worth it, I'm fine with replacing it with a generic message too. It's pretty low priority though, as this should basically never happen anyhow.
  1. OK, I misunderstood this too, sorry again. It's very strange that the directory doesn't get changed without FOS_NOCHANGEDIR though, why does this flag exist at all then? But let's add it, why not.
  1. I think failing completely is fine, this is again something that, AFAICS, should never happen in normal circumstances, and if something is so wrong that it does, it's probably more important to get an error back with the least amount of/simplest code possible (to minimize chances of anything else going wrong) than trying to be clever about it.

Thanks again for looking into all this!

comment:13 Changed 3 months ago by pb101

GetPath() and multiple selection

Just for the record, on MSW wxFileDialog returns the file/path which is sorted as the first among the selected items based on how the items were displayed (it can be sorted by name, date, size etc.) in the dialog. I.e., it does not reflect the order the user selected the items, which can be seen in the "File Name" edit control below the list. I do not use any desktop OS besides Windows, so I do not know what happens on GTK/OSX/Qt...

When the check for wxDD_MULTIPLE in wxDirDialog::GetPath() fails, what should be returned: an empty string or a path? Currently it returns the path of the first item in MSW (see above) and of the last item in GTK and OSX?


wxDD_CHANGE_DIR and wxDD_MULTIPLE incompatibility

I think the best way to check for this would be in wxDirDialogBase::Create(). The question is: Should the failed check return false? I assume returning false means the dialog will not be displayed? If this is too "harsh", Create() could still return true and the incompatibility must be handled also in the platform-specific code; I already do that in MSW, will revert if needed. I suppose Vadim knows how to generally handle such situations in wxWidgets codebase and will tell us what to do.


Committed MSW-specific changes

The commits are here: https://github.com/PBfordev/wxWidgets/commits/dirdialog

I am afraid I did not think through not supporting compilers that do not have required interfaces introduced in Windows Vista. Before, the code used IFileDialog and provided all declarations and definitions needed. The new code uses IFileOpenDialog (whether the new styles are used or not) and does not provide any declarations or definitions needed for it.

This means that if you were to use wxWidgets including this proposed change e.g. with MinGW32, you would get the old ::SHBrowseForFolder-based dialog where you had a Vista-style one before. I am not sure if such regression is acceptable and I suppose that in the end I will have to add all the interfaces...

Regarding the error message, I left it as it was for now. Not sure that adding the "(s)" is worth the work needed from the translators (and the current implementation does not have the error code as there are many places where it can fail: it would have to be changed). But of course, this is not my decision to make.

Last edited 3 months ago by pb101 (previous) (diff)

comment:14 Changed 3 months ago by vadz

I think we should bite the bullet and return an empty string if GetPath() is called for a dialog with wx[FD]D_MULTIPLE style, as long as we add the assert. Yes, it would be a regression but I just can't think of any situation in which it would make sense to let the user select multiple files and then ignore all of them but (arbitrarily chosen!) one. If you're in a very forgiving mood, we could skip the assert/return if there is just one file in the array anyhow.

Concerning incompatible styles, I'd indeed just add an assert in Create(), no need to return false -- just ignore wxDD_CHANGE_DIR in this case.

As I said, I'm ok with disabling/dropping/regressing MinGW32 support here. We can't keep supporting them forever, and I don't want to ask you to waste your time on this.

Also, please don't hesitate to take the decisions unless there is some violent opposition to them, this is the principle of the thing: you're doing the work, so it's your burden (but also your prerogative) to decide what's the best way to do it. If it's really badly wrong, someone will let you know, trust me :-)

Thanks again!

comment:15 Changed 3 months ago by pb101

I have realized that check for the invalid combination of wxDD_CHANGE_DIR and wxDD_MULTIPLE cannot be in the base class Create(), as this method is not necessarily called by the derived classes. For now, I have added the check to the MSW class ctor instead, not sure if this is the best idea.

I have also updated the documentation.

All my commits are still here: https://github.com/PBfordev/wxWidgets/commits/dirdialog

I wonder about support for the new wxDirDialog styles for other implementations, notably the generic and Qt one? These (or at least the generic one) do not have to support all the flags the native implementations do?

Regarding updating wxFileDialog to match wxDirDialog API. I think that should be done in a separate PR. BTW, if the APIs between the two should be very similar; how comes that wxDirDialog has an option for showing hidden items while wxFileDialog has not?

@Ian
How should we proceed next regarding GIT? Once the changes I made are reviewed and finalized, my commits should be squashed together and a PR against your branch made? I know next to nothing about GIT...

Last edited 3 months ago by pb101 (previous) (diff)

comment:16 Changed 2 months ago by imcinerney

I have made a PR here https://github.com/wxWidgets/wxWidgets/pull/1884 for implementing the OSX and GTK multiple selection and hidden folder viewing.

I have created PR https://github.com/wxWidgets/wxWidgets/pull/1883 to add in the checks for wxFileDialog to see if GetPath/GetFilename is being called when using the wxFD_MULTIPLE style


I wonder about support for the new wxDirDialog styles for other implementations, notably the generic and Qt one?

I have usually left other ports alone unless I need to use them or I can test them in someway. This means that I wasn't going to be making changes to the generic or QT version of the dialogs.


BTW, if the APIs between the two should be very similar; how comes that wxDirDialog has an option for showing hidden items while wxFileDialog has not?

wxFileDialog already supports hidden files using the wxFD_SHOW_HIDDEN style flag, that is why I am adding hidden support here as well.


@pb101
I don't know exactly how we want to combine our implementations, but I was thinking that we could settle on the API in my PR with OSX and GTK, and then you could make a new PR with the MSW implementation based off of that. Hopefully we can get this in before 3.1.4 is released.

comment:17 Changed 2 months ago by pb101

  • Cc pbfordev@… removed

Sorry for having missed that wxFD_SHOW_HIDDEN was added in 3.1.3.

I think it would be best if I pushed my MSW implementation against your PR. Hopefully I will be able be to manage that despite being GIT illiterate.

comment:18 Changed 3 weeks ago by Vadim Zeitlin <vadim@…>

  • Owner set to Vadim Zeitlin <vadim@…>
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 0918ab679/git-wxWidgets:

Merge branch 'dirdialog-multi-hidden'

Add wxDD_MULTIPLE and wxDD_SHOW_HIDDEN support to wxDirDialog.

See https://github.com/wxWidgets/wxWidgets/pull/1884

Closes #18736.

Note: See TracTickets for help on using tickets.