Closed Bug 718374 Opened 12 years ago Closed 12 years ago

The Options dialog allows a Library to be set as a Download folder but doesn't preserve the selection

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13

People

(Reporter: jaws, Assigned: jimm)

Details

Attachments

(1 file, 7 obsolete files)

STR:
1) Open Firefox options window
2) In General pane, within Downloads, choose "Browse..." to change the default download folder
3) Choose a Library, such as Music

Expected results:
The Library is not an accepted option when selecting a folder.

Actual results:
The Library is accepted and the user can click "Select folder". This choice is not preserved.

It's probably because a Library is basically read-only if I understand them correctly, but it's odd that it is allowed to be selected as a folder.
Attached image library warn (obsolete) —
One way to address this is to force file system paths for the folder picker. Selecting a library would then result in this warning dialog. (I don't see a way to hide the libs from the interface.)

Jared, is selecting a library a feature you expect to work, and if so where would you expect a downloaded file to end up? Libraries are an amalgamation of folders so I'm not sure which path we would choose as the result.
Attached patch disable libs patch (obsolete) — Splinter Review
Ah, never mind, there's a "default save location" check box for each folder in the properties of each library. Now I just need to figure out how I can query that.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #589502 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #589501 - Attachment is obsolete: true
Attachment #589537 - Attachment is obsolete: true
Comment on attachment 589540 [details] [diff] [review]
patch

This patch adds Win7 Library support to the new file/folder pickers. The old pickers didn't display these so the user couldn't chose them. The new picker code simply errors out. What this does is resolve these virtual folders to their corresponding default save folders, which are defined in library property sheets. You can view these through explorer via a right click on a library and selecting properties.
Attachment #589540 - Flags: review?(neil)
Component: Preferences → Widget: Win32
Product: Firefox → Core
QA Contact: preferences → win32
(In reply to Jim Mathies [:jimm] from comment #3)
> Ah, never mind, there's a "default save location" check box for each folder
> in the properties of each library. Now I just need to figure out how I can
> query that.

Great success! I didn't know about this, but this is a great solution. Thanks for taking this bug Jim.
Status: NEW → ASSIGNED
Comment on attachment 589540 [details] [diff] [review]
patch

Sorry for the delay, I've been suffering with a cold in the head.

>+    item = temp;
.swap perhaps?
Also, does this need to be in an #ifdef for older SDKs?

>+    // For modeSave, if the user chose a Win7 Library, resolve to the
>+    // library's default save folder.
But you can't actually save a folder, can you? You just end up saving a file, which I assume gets resolved to a name within the default save folder.

>+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
MOZ_NTDDI_LONGHORN isn't actually new enough for IShellItem, you want at least MOZ_NTDDI_WIN7

>+  if (x)
>+    return false;
>+  return true;
Equivalent to return !(x); i.e.
return shellLib &&
       SUCCEEDED(shellLib->LoadLibraryFromItem(item, STGM_READ)) &&
       SUCCEEDED(...);
Comment on attachment 589540 [details] [diff] [review]
patch

Review of attachment 589540 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsFilePicker.cpp
@@ +1002,5 @@
> +    // For modeSave, if the user chose a Win7 Library, resolve to the
> +    // library's default save folder.
> +    if (mMode == modeSave) {
> +      nsRefPtr<IShellItem> temp;
> +      if (QueryForShellLibraryFolder(item, temp))

I don't think this function will exist in some builds due to the #ifdefs around it's declaration and definition. Shouldn't this call to the function be guarded as well? I looked but didn't see #ifdef guards around nsFilePicker::ShowFilePicker. (I did see guards around nsFilePicker::ShowFolderPicker)
Everything in here is wrapped in -

#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN

so yes, I need to individually wrap all of this in 

#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7

my mistake. I'll update over the weekend.
(In reply to Jim Mathies [:jimm] from comment #10)
> Everything in here is wrapped in -
> 
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> 
> so yes, I need to individually wrap all of this in 
> 
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7
> 
> my mistake. I'll update over the weekend.

Note now that bug 699385 has landed, all this crazy ifdef'ing can come out.
Attachment #589540 - Flags: review?(neil)
Comment on attachment 589540 [details] [diff] [review]
patch

>+    item = temp;
Nit: consider using swap

>+    // For modeSave, if the user chose a Win7 Library, resolve to the
>+    // library's default save folder.
This doesn't make sense. The filepicker already does this, it's just the folderpicker that allows you to pick a library rather than a folder.

>+nsFilePicker::QueryForShellLibraryFolder(nsRefPtr<IShellItem>& item,
>+                                         nsRefPtr<IShellItem>& folder)
I'm not a fan of references to nsRefPtr, but maybe you can inline this if you're only going to need it for folder pickers.

>+  if (!shellLib ||
>+      FAILED(shellLib->LoadLibraryFromItem(item, STGM_READ)) ||
>+      FAILED(shellLib->GetDefaultSaveFolder(DSFT_DETECT, IID_IShellItem,
>+                                            getter_AddRefs(folder))))
Another approach is to null-check folder instead i.e.
if (shellLib) {
  shellLib->LoadLibraryFromItem(item, STGM_READ);
  shellLib->GetDefaultSaveFolder(DSFT_DETECT, IID_IShellItem,
                                 getter_AddRefs(temp));
}
if (temp)
  temp.swap(item);
Attached patch patch (obsolete) — Splinter Review
Various review comments address.  Also I removed the sdk gunk and touched up WinUtils SHCreateItemFromParsingName.
Attachment #589540 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
minor cleanup.
Attachment #595849 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #595851 - Attachment is obsolete: true
Attachment #596047 - Flags: review?(neil)
Comment on attachment 596047 [details] [diff] [review]
patch

>+  if (!VistaCreateItemFromParsingNameInit())
>+    return E_FAIL;
>   NS_ENSURE_TRUE(sCreateItemFromParsingName, E_FAIL);
Nit: VistaCreateItemFromParsingNameInit can't successfully return without setting sCreateItemFromParsingName, so you don't need to check it again.

>+WinUtils::GetShellItemPath(nsRefPtr<IShellItem>& aItem,
Can this be just IShellItem* aItem?

>+  aResultString.Assign(nsDependentString(str));
.Assign(str) should work.

>+  return aResultString.Length() > 0;
!aResultString.IsEmpty()

>           WinUtils::SHCreateItemFromParsingName(aInitialDir.get(), NULL,
>                                                 IID_IShellItem,
>                                                 getter_AddRefs(folder)))) {
...
>+  if (QueryForShellLibraryFolder(item, temp))
[I'd like this to use getter_AddRefs, but that would make folder a void**... the only other alternative would be to inline the function (only 1 caller).]

>+nsFilePicker::QueryForShellLibraryFolder(nsRefPtr<IShellItem>& item,
At least could you make this an IShellItem*?

>-#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>-// For Vista IFileDialog interfaces
>+// For Vista IFileDialog interfaces which aren't exposed
>+// unless _WIN32_WINNT >= _WIN32_WINNT_LONGHORN.
> #if _WIN32_WINNT < _WIN32_WINNT_LONGHORN
> #define _WIN32_WINNT_bak _WIN32_WINNT
> #undef _WIN32_WINNT
> #define _WIN32_WINNT _WIN32_WINNT_LONGHORN
> #define _WIN32_IE_bak _WIN32_IE
> #undef _WIN32_IE
> #define _WIN32_IE _WIN32_IE_IE70
> #endif
>-#endif
[I wonder whether we need any of this any more.]
Attached patch patchSplinter Review
updated per comments.
Attachment #596047 - Attachment is obsolete: true
Attachment #596047 - Flags: review?(neil)
Attachment #597829 - Flags: review?(neil)
Ugh, just noticed that 

NS_ENSURE_TRUE(aItem, 0);

in GetShellItemPath should be 

NS_ENSURE_TRUE(aItem, false);

will update locally.
Comment on attachment 597829 [details] [diff] [review]
patch

Yeah, I noticed that too. r=me with that fixed.
Attachment #597829 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/8e5e54e70600
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/13.0a1 Firefox/13.0a1

So the user is able to select libraries (documents, music, pictures, videos) and files are saved to:
C:\Users\username\Documents
C:\Users\username\Music
C:\Users\username\Pictures
C:\Users\username\Videos
Is this correct?
(In reply to Paul Silaghi [QA] from comment #22)
> Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1)
> Gecko/13.0a1 Firefox/13.0a1
> 
> So the user is able to select libraries (documents, music, pictures, videos)
> and files are saved to:
> C:\Users\username\Documents
> C:\Users\username\Music
> C:\Users\username\Pictures
> C:\Users\username\Videos
> Is this correct?

The paths depend on a setting for the library that defines the default save-to folder. In the file picker, right-click the library and select Properties to view this setting.
Verified fixed this on Nightly 13.0a1 (2012-02-20) on Win 7/64, Mac OS X 10.7 and Ubuntu 11.10
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: