Closed
Bug 301298
Opened 17 years ago
Closed 17 years ago
nsFilePicker ignores display directory setting on OS X
Categories
(Core Graveyard :: Widget: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tony, Assigned: tony)
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 2 obsolete files)
1.66 KB,
patch
|
mikepinkerton
:
review+
sfraser_bugs
:
superreview+
jaas
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
Attachment #189762 -
Flags: review?(pinkerton)
Updated•17 years ago
|
Whiteboard: DUPEME
Comment 2•17 years ago
|
||
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.
Comment 3•17 years ago
|
||
looks good to me, agreed on javier's comments. some small cleanup and this can be r+'d.
Assignee | ||
Comment 4•17 years ago
|
||
(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)
Comment 5•17 years ago
|
||
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.
Updated•17 years ago
|
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 6•17 years ago
|
||
Comment on attachment 189812 [details] [diff] [review] Response to review comments r=pink
Attachment #189812 -
Flags: review?(pinkerton) → review+
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
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+
![]() |
||
Comment 9•17 years ago
|
||
Did this patch ever get checked in? Or does that still need to happen?
Assignee | ||
Comment 10•17 years ago
|
||
(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 11•17 years ago
|
||
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)
Updated•17 years ago
|
Attachment #192837 -
Flags: superreview?(sfraser_bugs) → superreview+
Comment 12•17 years ago
|
||
I'll land this.
Comment 13•17 years ago
|
||
landed on trunk with bracket cleanup. thanks Tony.
Comment 14•17 years ago
|
||
(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+
Comment 15•17 years ago
|
||
Checked in on MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•