Last Comment Bug 664832 - Page info should use the same last dir for both single and multiple file saves
: Page info should use the same last dir for both single and multiple file saves
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Page Info (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.6
Assigned To: Ian Neal
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-16 13:58 PDT by Ian Neal
Modified: 2011-09-11 15:01 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed
fixed


Attachments
Rewrite saveMedia function (3.20 KB, patch)
2011-09-05 16:02 PDT, Ian Neal
no flags Details | Diff | Review
Patch without whitespace changes (1.99 KB, patch)
2011-09-05 16:04 PDT, Ian Neal
no flags Details | Diff | Review
Do same no matter the count (3.38 KB, patch)
2011-09-06 13:30 PDT, Ian Neal
no flags Details | Diff | Review
Ask download manager for location (4.46 KB, patch)
2011-09-08 15:06 PDT, Ian Neal
no flags Details | Diff | Review
Use download manager and pref but not auto (4.44 KB, patch)
2011-09-09 06:52 PDT, Ian Neal
no flags Details | Diff | Review
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16] (1.19 KB, patch)
2011-09-09 16:15 PDT, Ian Neal
neil: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Review

Description Ian Neal 2011-06-16 13:58:40 PDT
As pointed out by neil in bug 664740, when saving media from page info page, you can get given a different dir for when saving single or multiple files.
Comment 1 Ian Neal 2011-09-05 16:02:32 PDT
Created attachment 558356 [details] [diff] [review]
Rewrite saveMedia function

This patch:
* Uses saveImage function for both single and multiple image saves
* Always prompts for save location.
Comment 2 Ian Neal 2011-09-05 16:04:53 PDT
Created attachment 558357 [details] [diff] [review]
Patch without whitespace changes
Comment 3 neil@parkwaycc.co.uk 2011-09-06 08:56:19 PDT
Comment on attachment 558356 [details] [diff] [review]
Rewrite saveMedia function

>+  if (tree.view.selection.count == 1) {
>+    rowArray.push(tree.currentIndex);
If we're not going to special-case the save code on the number of selected items then I don't see why we need to special-case collecting the items.
Comment 4 Ian Neal 2011-09-06 13:30:18 PDT
Created attachment 558584 [details] [diff] [review]
Do same no matter the count

Changes since last version:
* Do the same thing whether the count is 1 or not
Comment 5 neil@parkwaycc.co.uk 2011-09-07 14:23:49 PDT
I wasn't watching carefully enough. I think the problem turns out to be that single file saves defaults to the downloads folder if you haven't changed it, but multiple file saves has no default. Once you set a download folder, then both single and multiple file saves default to that folder.
Comment 6 neil@parkwaycc.co.uk 2011-09-08 12:52:07 PDT
Well, the code for saving a single file gets complicated. If you have prompting enabled, then it tries to default to the last prompted directory for that domain. Failing that, it tries to read the download directory from the download manager. Failing that, it uses the desktop.

I don't think we care in the multiple file case because they could come from different domains. But we should still read the download directory from the download manager, because the pref won't always have been set.
Comment 7 Ian Neal 2011-09-08 15:06:30 PDT
Created attachment 559298 [details] [diff] [review]
Ask download manager for location

Changes since previous version:
* Ask download manager for location to download files to instead of pref.
Comment 8 neil@parkwaycc.co.uk 2011-09-09 02:25:41 PDT
Comment on attachment 559298 [details] [diff] [review]
Ask download manager for location

>+  let autodownload = getBoolPref("browser.download.useDownloadDir", false);
>+  let dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                          .getService(Components.interfaces.nsIDownloadManager);
>+  let initialDir = dnldMgr.userDownloadsDirectory;
>+  if (autodownload)
>+    return initialDir;
>+
>   const nsILocalFile = Components.interfaces.nsILocalFile;
>   const nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var fp = Components.classes["@mozilla.org/filepicker;1"]
>                      .createInstance(nsIFilePicker);
> 
>   var titleText = gBundle.getString("mediaSelectFolder");
>   fp.init(window, titleText, nsIFilePicker.modeGetFolder);
>-  var initialDir = GetLocalFilePref("browser.download.dir");
>-  if (initialDir)
>-    fp.displayDirectory = initialDir;
>+  fp.displayDirectory = initialDir;
That's better than what we have now, since it does at least try to download to Downloads. But now I'm not sure whether we need the rest of the changes.

Note: An even better improvement would be to set the displayDirectory to the browser.download.lastDir pref if it exists (possibly via DownloadLastDir.jsm).
Comment 9 Ian Neal 2011-09-09 06:52:38 PDT
Created attachment 559441 [details] [diff] [review]
Use download manager and pref but not auto

Changes since previous version:
* Checks lastDir pref first.
* Does not use autodownload pref.
Comment 10 neil@parkwaycc.co.uk 2011-09-09 13:02:23 PDT
Comment on attachment 559441 [details] [diff] [review]
Use download manager and pref but not auto

>+  let initialDir = GetLocalFilePref("browser.download.lastDir");
>+  if (!initialDir) {
>+    let dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                            .getService(Components.interfaces.nsIDownloadManager);
>+    initialDir = dnldMgr.userDownloadsDirectory;
r=me on these five lines. But I think we should restore the special case for a single image; I think people would be surprised to get a directory picker.

>-  var initialDir = GetLocalFilePref("browser.download.dir");
Nit: Now that you're not returning early, you could move the code here.
Comment 11 Ian Neal 2011-09-09 16:15:38 PDT
Created attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

Changes since last version:
* As suggested just fix-up multiple file saving.
Comment 12 neil@parkwaycc.co.uk 2011-09-09 16:28:35 PDT
Comment on attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

>-  var initialDir = GetLocalFilePref("browser.download.dir");
>+  let initialDir = GetLocalFilePref("browser.download.lastDir");
I see you've "let" yourself go over to the dark side ;-)
Comment 13 Ian Neal 2011-09-10 05:04:42 PDT
Comment on attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

Checked in with a little bit less let:
http://hg.mozilla.org/comm-central/rev/cd3a86314bd1
Comment 14 Ian Neal 2011-09-10 05:06:42 PDT
Comment on attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

Simple, low risk fix. Might be a bit late for comm-beta but asking for both aurora and beta.
Comment 15 Justin Wood (:Callek) 2011-09-10 15:50:07 PDT
Comment on attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

It might miss our beta2, but I hear rumor that there will likely be another Gecko beta, so go ahead and land on comm-beta.

Note You need to log in before you can comment on or make changes to this bug.