Closed Bug 301298 Opened 16 years ago Closed 15 years ago

nsFilePicker ignores display directory setting on OS X

Categories

(Core Graveyard :: Widget: Mac, defect)

PowerPC
macOS
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tony, Assigned: tony)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-ca) AppleWebKit/412 (KHTML, like Gecko) Safari/412
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

Setting displayDirectory on an nsFilePicker in OS X has no effect. The dialog defaults to the last directory 
used instead of respecting the displayDirectory choice.

Reproducible: Always

Steps to Reproduce:
1. Create and initialise a file picker (nsFilePicker).
2. Set its displayDirectory to an arbitrary directory.
3. Show the file picker.

Actual Results:  
The file picker displays the last directory in which it was opened.

Expected Results:  
The file picker displays the directory assigned to displayDirectory.
Attached patch Patch to fix described bug (obsolete) — Splinter Review
Attachment #189762 - Flags: review?(pinkerton)
Comment on attachment 189762 [details] [diff] [review]
Patch to fix described bug

+      // Set the directory to mDisplayDirectory if available
+      nsCOMPtr<nsILocalFile> displayDir = self->mDisplayDirectory;
+      if (!displayDir)
+	 return;
+      nsCOMPtr<nsILocalFileMac> localDisplay = do_QueryInterface(displayDir);
+      if (!localDisplay)

Don't see why you need a local variable for that.  You could just do:
  nsCOMPtr<nsILocalFileMac> localDisplay =
	    do_QueryInterface(self->mDisplayDirectory);
  if (localDisplay) { ....

Also, all those returns don't look good.  Might be better to nest the success
cases.
looks good to me, agreed on javier's comments. some small cleanup and this can
be r+'d.
Attached patch Response to review comments (obsolete) — Splinter Review
(In reply to comment #2)
> Don't see why you need a local variable for that.  You could just do:
>   nsCOMPtr<nsILocalFileMac> localDisplay =
>	      do_QueryInterface(self->mDisplayDirectory);
>   if (localDisplay) { ....

I wasn't sure if do_QueryInterface was safe for null values (I'm still an XPCOM
neophyte). I've eliminated the local variable.

> Also, all those returns don't look good.  Might be better to nest the success

> cases.

Done.
Attachment #189762 - Attachment is obsolete: true
Attachment #189812 - Flags: review?(pinkerton)
Oh, yeah, you are correct: |do_QueryInterface| does seem to work for NULL.  Just
to make it more clear, though, you may just want to wrap everything with a |if
(self->mDisplayDirectory != nsnull) { .... }|.  Also, to make it more legible,
separate the new code from the old with a blank line.
Assignee: nobody → tony
Status: UNCONFIRMED → NEW
Component: General → Widget: Mac
Ever confirmed: true
Flags: review?(pinkerton)
Product: Firefox → Core
QA Contact: general → mac
Version: unspecified → Trunk
Comment on attachment 189812 [details] [diff] [review]
Response to review comments

r=pink
Attachment #189812 - Flags: review?(pinkerton) → review+
There's a mistake in my last patch involving use of the Carbon function
AECreateDesc: The function doesn't actually create the object, it merely
initializes it. Sorry! I added Javier's formatting suggestions while I was at
it.
Attachment #189812 - Attachment is obsolete: true
Attachment #192837 - Flags: review?(pinkerton)
Comment on attachment 192837 [details] [diff] [review]
Fixes problem with previous patch

ah yes, good catch. i missed that.

r=pink
Attachment #192837 - Flags: review?(pinkerton) → review+
Did this patch ever get checked in?  Or does that still need to happen?
(In reply to comment #9)
> Did this patch ever get checked in?  Or does that still need to happen?
> 

I'm assuming this patch still needs super-review. Am I supposed to set this flag myself? If so, from whom do I request it?
Comment on attachment 192837 [details] [diff] [review]
Fixes problem with previous patch

Please put brackets on the same line as the test. That is Mozilla coding style.

+      if (self->mDisplayDirectory != nsnull)
+      {

should be

+      if (self->mDisplayDirectory != nsnull) {
Attachment #192837 - Flags: superreview?(sfraser_bugs)
Attachment #192837 - Flags: superreview?(sfraser_bugs) → superreview+
I'll land this.
Flags: blocking1.8.1?
landed on trunk with bracket cleanup. thanks Tony.
(In reply to comment #5)
>Oh, yeah, you are correct: |do_QueryInterface| does seem to work for NULL. Just
>to make it more clear, though, you may just want to wrap everything with a |if
>(self->mDisplayDirectory != nsnull) { .... }|.
IIRC neither null-checking the parameter of do_QueryInterface nor explicitly comparing anything to null are considered good style.
Attachment #192837 - Flags: approval-branch-1.8.1+
Flags: blocking1.8.1?
Checked in on MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
Product: Core → Core Graveyard
Whiteboard: DUPEME
You need to log in before you can comment on or make changes to this bug.