546853 - Setting the Download Folder in Preferences doesn't stick

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Christopher Henderson]

On 1.9.2-based builds, the Download folder in the Downloads prefpane doesn't persist. Bug 454242 most likely broke it. We need to set the prefs browser.download.folderList and browser.download.dir, as done in the patch for that bug.

Comment 1

[Christopher Henderson]

Attachment: Fix v1.0

Here's my initial try. Works well in my limited testing. Default/fallback directory is ~/Downloads/.

Comment 2

[Stuart Morgan]

For the full fix, we'll want one-time migration code in the preference manager, so people don't all lose their download setting the first time they run 2.1.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Erm, Stuart just said in one sentence what took me 3 pghs to say while commenting on other bugs ;)

If we want to get rid of all that IC code sooner rather than later, might we land for a future 2.0.x both bug 546854 and some code to also write the current download folder to browser.download.dir (and update b.d.d on any change)?

> Default/fallback directory is ~/Downloads/.

Also, do we want to do that on 10.4, too, where the folder doesn't exist by default?  Do we care?  Is Gecko going to create that folder?  If so, is it going to confuse 10.4 users when their downloads move from the Desktop to some "hidden" folder?  

Migration will take care of existing users OK, I guess, but what about any potential new users (or is the IC pref set to ~/Desktop by default on 10.4; I don't remember, but recent Safari versions no longer use/write it, so I'm not sure what state we might find that pref in, or if it's guaranteed to exist--I do recall helping debug issues with that in the past.

Finally, as I mentioned on irc, it looks like we'll need some visibility sprinkles for the static build (unless mine is horked somehow): http://pastebin.mozilla.org/703983

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> If we want to get rid of all that IC code sooner rather than later, might we
> land for a future 2.0.x both bug 546854 and some code to also write the current
> download folder to browser.download.dir (and update b.d.d on any change)?

That seems like more trouble than it's worth, and more prone to errors. Having the IC migration code in 2.1 isn't an issue.

For convenient reference, since I was poking around, the code we'll want to adapt for migration is:
http://mxr.mozilla.org/mozilla/source/xpcom/io/nsDirectoryService.cpp#917

> > Default/fallback directory is ~/Downloads/.
> 
> Also, do we want to do that on 10.4, too, where the folder doesn't exist by
> default?

Gecko does the right thing on 10.4. It asks the OS for the download folder, and if that fails (which it will on 10.4, where there's no such concept) it asks for the desktop folder.

> Migration will take care of existing users OK, I guess, but what about any
> potential new users

Since we have the pref version to help with migration, I think the best thing is just to only run the migration if the version is >0 (i.e., there actually is an existing profile).

Comment 5

[Christopher Henderson]

Attachment: Fix v1.2

Adds pref migration from Internet Config. Relies on Fix v1.1 for Bug 546853. This patch is applied with a fuzz of 1, since I had to hand modify it to interleave it with said fix.

Comment 6

[Stuart Morgan]

Two quick high-level notes:
- There's no need to round-trip the IC pref through nsLocalFile, since this isn't core code dealing with core types. Instead, use CFURLCreateFromFSRef and get the path that way.
- Conditionalizing on the presence of the new pref won't do what you want once all-camino.js is updated, nor would it do the right thing with a new profile now. You should rev the pref version number, and check for < the new version (and > 0 as mentioned above)

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm sure whatever Stuart said covers this, but the net effect of this patch is that we read the value from IC (it seems like) over and over again.  If you select another folder in the UI, we change the UI (at least until you close prefs/switch panes), but downloads continue to end up wherever the IC value specifies.

This seems to happen with either an existing profile or a fresh one.

Comment 8

[Christopher Henderson]

Attachment: Fix v1.3

Updated patch to use CFURL and prefs version check. Tested on a static build, with both new and existing profiles. If a new profile, sets the d/l dir to the system's one.

Comment 9

[Christopher Henderson]

Attachment: Fix v1.4

There were problems setting the pref to directories with spaces in their name. This and Fix v1.2 for Bug 546854 should solve them.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Note that the old FSpMakeFSRef is deprecated in the 10.5 SDK, so we'll need to fix that for 1.9.3.

Comment 11

[Christopher Henderson]

Attachment: Fix v1.5

Uses handles and aliases instead of FSSpecs.

Comment 12

[Christopher Henderson]

Attachment: Fix v1.6

Fix v1.5 was hand-tweaked to untangle it from Bug 546854, but was malformed. This new patch assumes that the patch for that bug is applied.

Comment 13

[Stuart Morgan]

Comment on attachment 432991 [details] [diff] [review]
Fix v1.6

> - (void)setDownloadFolder:(NSString*)inNewFolder
> {
>+  [[PreferenceManager sharedInstance] setDownloadDirectoryPrefToPath:inNewFolder];
> }

No need to keep a method when the implementation is one line and easily readable.

>+// If they clicked ok, change the pref and re-display the new choice in the

Since you are touching this line, s/re-display/display/ (the new pref is being displayed for the first time, not re-displayed).


Not in your patch, this line (in chooseDownloadFolder:):
   NSString* oldDLFolder = [[PreferenceManager sharedInstance] downloadDirectoryPath];
needs to change to downloadDirectoryPath.

>+// the path to the default system download folder
> - (NSString*)downloadDirectoryPath;

This can be made private (and should be to reduce the chance of a client accidentally confusing it with downloadDirectoryPref). Perhaps better, you could also rename it geckoDefaultDownloadDirectory and use downloadDirectoryPath/setDownloadDirectoryPath: for the versions that set the download pref.

>+// the path to the users's desktop folder
>+- (NSString*)desktopDirectoryPath;

Also should be private (and if you rename the other method, renamed geckoDesktopDirectory).

>+  if (mLastRunPrefsVersion == 0) // New profile, so default to Downloads
>+    [self setDownloadDirectoryPrefToPath:[self downloadDirectoryPref]];

As you can see, confusing the two methods as current named is easy ;) This should be what you currently call downloadDirectoryPath. However, there's no need for this--just put the default value in all-camino.js.

>+  if (!aPath || [aPath isEqualToString:[self downloadDirectoryPath]]) {
>+    [self setPref:kGeckoPrefDownloadsFolderList toInt:kDownloadsFolderDownloads];
>+  }
>+  else if ([aPath isEqualToString:[self desktopDirectoryPath]]) {
>+    [self setPref:kGeckoPrefDownloadsFolderList toInt:kDownloadsFolderDesktop];
>+  }
>+  else {
>+    [self setPref:kGeckoPrefDownloadsFolderList toInt:kDownloadsFolderCustom];
>+    [self setPref:kGeckoPrefDownloadsDir toFile:[NSURL fileURLWithPath:aPath]];

Reduce the duplication here:
  int folderType = kDownloadsFolderDownloads;
  if ([aPath isEqualToString:[self desktopDirectoryPath]]) {
    folderType = kDownloadsFolderDesktop;
  }
  else if (aPath && ![aPath isEqualToString:[self desktopDirectoryPath]]) {
    folderType = kDownloadsFolderCustom;
    [self setPref:kGeckoPrefDownloadsDir toFile:[NSURL fileURLWithPath:aPath]];
  }
  [self setPref:kGeckoPrefDownloadsFolderList toInt:folderType];

>+- (NSString*)downloadDirectoryPref
>+{
>+  NSString* prefValue = nil;
>+  BOOL gotPref;
>+  int downloadFolder = [self getIntPref:kGeckoPrefDownloadsFolderList withSuccess:&gotPref];
>+
>+  if (gotPref && (downloadFolder == kDownloadsFolderDesktop))
>+    prefValue = [[PreferenceManager sharedInstance] desktopDirectoryPath];
>+  else if (downloadFolder == kDownloadsFolderDownloads)
>+    prefValue = [[PreferenceManager sharedInstance] downloadDirectoryPath];
>+  else if (downloadFolder == kDownloadsFolderCustom)
>+    prefValue = [[self getFilePref:kGeckoPrefDownloadsDir withSuccess:NULL] path];
>+
>+  return prefValue;
>+}

First off, this method should always return something useful to the caller, even if no pref is set (or the pref value is wrong, or some newfangled value someone read about for Firefox 7 but we don't support). Second, you are reading downloadFolder if gotPref failed in two of the cases.

How about:
  if (gotPref && <it's the desktop folder>)
     ...
  else if (gotPref && <it's custom>)
    ...
  else
    <use the default download directory>

Comment 14

[Christopher Henderson]

Attachment: Fix v1.7

New patch with review comments addressed.

Comment 15

[Stuart Morgan]

Comment on attachment 435301 [details] [diff] [review]
Fix v1.7

r=me with just a couple thigs I missed before:

>+        err = ::ICGetIndPref(icInstance, i, key);
>+        CFStringRef keyStringRef = CFStringCreateWithPascalString(NULL,
>+                                                                  key,
>+                                                                  kCFStringEncodingMacRoman);
>+        NSString* keyString = [(NSString*)keyStringRef autorelease];
>+
>+        if (err == noErr && [keyString isEqualToString:icDownloadFolderKey]) {

You shouldn't use |key| if err indicates failure. You can also reduce the amount of nesting in the loop by using continues. so:
  err = ::ICGetIndPref(icInstance, i, key);
  if (err != noErr)
    continue;
  <do the CFString creation/autoreleasing>
  if (![keyString isEqualToString:...])
    continue;

>+            AliasHandle	aliasHandle = (AliasHandle)NewHandleClear(aliasSize);

There's a tab in the middle of this line.

Comment 16

[Christopher Henderson]

Attachment: Fix v1.8

Fix with review comments addressed.

Comment 17

[Eiichi]

I applied an attachment (id=434809) and an attachment (id=435432), but setting the Download Folder in Preferences doesn't work in my test build on 10.4.

But with an attachment (id=434809) and an attachment (id=429298) my test build works fine on 10.4.

Comment 18

[Eiichi]

Sorry for the Comment 17.
To make sure I retested on the fresh tree, and the patch works as expected.

Sorry again.

Comment 19

[Mike Pinkerton (not reading bugmail)]

+  err = ::ICStart(&icInstance, 'XPCM');

Declare |err| on the same line as initialization.

+      for ( long i = 0; i < numPrefs; ++i ) {

remove extra space before/after paren.

sr=pink

Comment 20

[Christopher Henderson]

Attachment: Fix v1.9, for checkin

Patch with sr review comments addressed, for checkin.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/de718f3614cf