89325 - filepicker used to select directories ignores |displayDirectory| attribute

Status: RESOLVED WORKSFORME

(Toolkit :: UI Widgets, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

On Windows, a file picker initialized with |nsIFilePicker.modeGetFolder| 
ignores the |displayDirectory| ignores the attribute.  It always comes up in 
the "My Computer" folder.

Attaching a test XUL file that demonstrates.  Each time the "browse" button is 
selected, the value of the text field is used to initialize the directory.  
However, the picker always comes up in "My Computer".

Also attaching a patch.  After applying the patch, the test correctly defaults 
the dialog to the value in the text box.

The patch also enables the status line on the dialog, as the same technique is 
used to update the status text.

Assigning to bryner as he seems to own filepicker bugs.  Assign back to me if 
you like (possibly with r=:)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Test case demonstrating that the initial directory is ignored.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Patch that corrects the behaviour.

Comment 3

[Brian Ryner (not reading)]

back over to markh, i don't know anything about the windows filepicker

Comment 4

[Boris Zbarsky [:bzbarsky]]

bill, could you check out this patch? It's been rotting for a while now,
unfortunately... :(

Comment 5

[Bill Law]

I can't devote time to it right now unless it gets
nsbeta1+/[adt1]/yadda-yadda-yadda.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attaching a fresh copy of the patch.  To see the bug in Mozilla:

* Go to Preferences -> Advanced -> Cache.
* Click on "Choose Folder" to change the cache directory.
* File chooser opens, but "My Computer" is selected.  We expect the existing
cache directory to be selected.

With this patch applied, the existing cache directory is correctly shown.  A
quick lxr search shows there may also be one or 2 other (fairly obscure)
locations this would show up.

Anyone want to take the "yadda yadda" on? :)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fresher patch

Updated to the trunk, and my new email address used.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Mark, this is very unlikely to get attention from the adt... :(

Dean, can you review this or suggest a non-netscape windows developer who might?

Comment 9

[:deanis74]

Comment on attachment 79362 [details] [diff] [review]
Fresher patch

+// See KB Q179378
(http://support.microsoft.com/support/kb/articles/Q179/3/78.ASP)

MS seems to have changed their directory scheme, again.  Can you change this
to:

  http://support.microsoft.com/default.aspx?scid=kb;EN-US;q179378

+static INT CALLBACK BrowseCallbackProc(HWND hwnd, 

In MS's example they don't declare the function as static.  Do we need to? 
Does it matter?

+	     if (szPath && szPath[0]) {

Aren't these the same?

In retrospect, my questions look dumb and obvious to me.  I blame it being
early-morning and I'm just starting my coffee.	The patch looks good to me, I
just needed something to nit.  I like that we're setting the status text with
this.  r=me with these questions answered.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fix the URL of the MS KB article

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 79555 [details] [diff] [review]
Fix the URL of the MS KB article

Applying last review to this patch

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Dean,
Thanks for the review.

Re "static" - this should have no affect.  As this function is used only within
the source file, "static" is appropriate to keep down the number of publically
visible names.

Re: +	     if (szPath && szPath[0]) {

These are not the same.  First check is that the pointer is non NULL, second is
that the first character is not the null character.

Comment 13

[:deanis74]

Yes, it's all much clearer now that I'm actually awake!

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: directory is now a nsString - use .get() method to get at the sz.

Will assume review still applies - only change is a call to .get() for the
nsString.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

This patch has review but is slowly rotting again :(  Can someone suggest sr= 
for this bug, or should I just mail reviewers?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Mark, try bryner or jag (and cc reviewers, of course)

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 84573 [details] [diff] [review]
directory is now a nsString - use .get() method to get at the sz.

Can you please post this as a unified diff (-u)?  Also, the indenting of the
arguments for BrowseCallbackProc looks off.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: unified diff, and param indents fixed.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

*** Bug 118916 has been marked as a duplicate of this bug. ***

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Just resolved 89325, which blocks 108542 - adding the block here.

Comment 21

[Peter Lairo]

isn't there some keyword, text ("[PATCH]") or flag that can be added to let the
reviewers know there is a patch waiting for them (comment #18)?

Comment 22

[Boris Zbarsky [:bzbarsky]]

You can set the review request flags on the patch to request review from
particular people....

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch

Here is a new patch against the trunk.	It handles the MOZ_UNICODE and not
case, but only MOZ_UNICODE (the default) has been tested.  It has been tested
on Win98 and Win2000.

Note again that the simplest way to see this bug is
Preferences->Advanced->Cache, and select "Choose Directory".  Asking for Seth's
review, as he touched it (significantly) last!

Comment 24

[PikeUK]

This works for me (the folder display part anyway) in both:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050811 Firefox/1.0+
and:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050811 SeaMonkey/1.0a

Perhaps fixed in Bug 232443?

Comment 25

[Wayne Mery (:wsmwk)]

Comment on attachment 120672 [details] [diff] [review]
Updated patch

clearing review ... and closing bug