232443 - Modifying Windows Folder Picker to use the new folder dialog

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Scott MacGregor]

Windows has a better folder picker dialog than the one we currently use. Ben's
firebird installer already uses it.

We would just need to follow what he did here:

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c#510

On an unrelated note, is it not possible to have the folder picker open with a
default folder path already traversed? It always shows us at the desktop root.

Comment 1

[:deanis74]

Taking a quick look at the docs, I think to set the default folder you need to:

1. pass in a BrowseCallbackProc in BROWSEINFO.lpfn
2. respond to the BFFM_INITIALIZED message
3. send a BFFM_SETSELECTION message to the dialog, passing in the folder to select

Comment 2

[Scott MacGregor]

thanks for the tip Dean. Patch coming up to make our folder picker smart enough
to to preselect the passed in initial directory path if there is one.

Comment 3

[Scott MacGregor]

Attachment: make the folder picker on windows pre-select the passed in initial folder Path

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... does this work with non-ascii names? shouldn't you use NS_CopyUnicodeToNative?

Comment 5

[Scott MacGregor]

Yeah I was not sure what to use use there.....I agree UTF8 seems like the wrong
thing to pass in.

Do you think ConvertToNative is the right one to use or converttoAscii?

I'd lean towards your suggestion of ConvertToNative

Comment 6

[:deanis74]

Comment on attachment 140612 [details] [diff] [review]
make the folder picker on windows pre-select the passed in initial folder Path

>     browserInfo.ulFlags        = BIF_RETURNONLYFSDIRS;//BIF_STATUSTEXT | BIF_RETURNONLYFSDIRS;

Do you want to add in BIF_USENEWUI?

>+    browserInfo.lpfn           = &BrowseCallbackProc;
>+    if (initialDir.Length()) // convert folder path to ascii, the strdup copy will be released in BrowseCallbackProc
>+      browserInfo.lParam       = (LPARAM) 

Maybe only set the callback if initialDir is passed in.

Comment 7

[Scott MacGregor]

Attachment: updated patch to use the new ui flag and to convert to native instead of uf8

Comment 8

[Scott MacGregor]

Attachment: don't set the callback unless we have an initial folder path to set

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

from msdn:
BIF_USENEWUI
    Version 5.0. Use the new user interface, including an edit box. This flag is
equivalent to BIF_EDITBOX | BIF_NEWDIALOGSTYLE. To use BIF_USENEWUI, you must
call OleInitialize or CoInitialize before calling SHBrowseForFolder.

is one of those two functions always called before SHBrowseForFolder?

Comment 10

[:deanis74]

It's called in nsToolkit's constructor (and a shwack of other places).  That
should suffice, shouldn't it?

Comment 11

[[e-mail]]

Comment on attachment 140622 [details] [diff] [review]
don't set the callback unless we have an initial folder path to set

This won't correctly set the initial folder on Windows NT, unless said folder
only contains ANSI characters. Compare bug 218242.

Comment 12

[Scott MacGregor]

can we track the windows NT specific issue in a separate bug? In the worst case,
a windows NT user would get the exact behavior we have today where the folder
picker does not get pre-selected. In the best case, the folder path only
contains ANSI characters and it works like it would work for Win9x, Win2k and
Win XP. It is quite possible Malcom Rowe might be interested in championing this
change for WinNT since he has interest in the bug Neil cited as well. 

Comment 13

[Scott MacGregor]

hmmm, I don't know who the module owner for widget is anymore. Doesn't look like
we have one. Looks like Pavlov initially created this class I'll ask him for moa
if one of you cares to do the review.

Comment 14

[:deanis74]

Comment on attachment 140622 [details] [diff] [review]
don't set the callback unless we have an initial folder path to set

>+    browserInfo.ulFlags        = BIF_USENEWUI | BIF_RETURNONLYFSDIRS;//BIF_STATUSTEXT | BIF_RETURNONLYFSDIRS;

Can toast the stuff after the comment.

>+      browserInfo.lpfn           = &BrowseCallbackProc;

Should probably fix the spacing before the "=".  Man, that's ugly spacing!

r=me

Comment 15

[Stuart Parmenter]

looks good to me

Comment 16

[Scott MacGregor]

Comment on attachment 140622 [details] [diff] [review]
don't set the callback unless we have an initial folder path to set

I got an moa=pavlov on this over aim.

I also incorporated Dean's review comments into the patch I'll be checking in.

Comment 17

[Ere Maijala (slow)]

I think it would have been better to handle UNICODE paths correctly from the
beginning. As far as I know, only minor changes are needed (SendMessageW,
BFFM_SETSELECTIONW and the path in UCS2). Anyway, if it goes in like this,
please open the separate bug and add it as a dependency.

Comment 18

[Scott MacGregor]

spin off Bug #234946 for Windows NT issues.

Fix checked in. Thanks for the help everyone.

Comment 19

[[e-mail]]

(In reply to comment #17)
>I think it would have been better to handle UNICODE paths correctly from the
>beginning. As far as I know, only minor changes are needed (SendMessageW,
>BFFM_SETSELECTIONW and the path in UCS2).
Changes? You can't get rid of the code that was checked in, because it's needed
for 9x... you could add new code for NT, of course.

Comment 20

[Ere Maijala (slow)]

>Changes? You can't get rid of the code that was checked in, because it's needed
>for 9x... you could add new code for NT, of course.

Sorry, that's what I actually meant.