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

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Jim Mathies [:jimm]]

Attachment: library warn

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.

Comment 2

[Jim Mathies [:jimm]]

Attachment: disable libs patch

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch

Comment 6

[Jim Mathies [:jimm]]

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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 8

[[e-mail]]

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 9

[Jared Wein [:jaws] (please needinfo? me)]

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)

Comment 10

[Jim Mathies [:jimm]]

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.

Comment 11

[Jim Mathies [:jimm]]

(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.

Comment 12

[[e-mail]]

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);

Comment 13

[Jim Mathies [:jimm]]

Attachment: patch

Various review comments address.  Also I removed the sdk gunk and touched up WinUtils SHCreateItemFromParsingName.

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch

minor cleanup.

Comment 15

[Jim Mathies [:jimm]]

Attachment: patch

Comment 16

[[e-mail]]

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.]

Comment 17

[Jim Mathies [:jimm]]

Attachment: patch

updated per comments.

Comment 18

[Jim Mathies [:jimm]]

Ugh, just noticed that 

NS_ENSURE_TRUE(aItem, 0);

in GetShellItemPath should be 

NS_ENSURE_TRUE(aItem, false);

will update locally.

Comment 19

[[e-mail]]

Comment on attachment 597829 [details] [diff] [review]
patch

Yeah, I noticed that too. r=me with that fixed.

Comment 20

[Jim Mathies [:jimm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8e5e54e70600

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e5e54e70600

Comment 22

[Paul Silaghi, QA [:pauly]]

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?

Comment 23

[Jim Mathies [:jimm]]

(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.

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed this on Nightly 13.0a1 (2012-02-20) on Win 7/64, Mac OS X 10.7 and Ubuntu 11.10