Page info should use the same last dir for both single and multiple file saves

RESOLVED FIXED in seamonkey2.6

Status

SeaMonkey
Page Info
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

Trunk
seamonkey2.6

SeaMonkey Tracking Flags

(seamonkey2.3 wontfix, seamonkey2.4 fixed, seamonkey2.5 fixed, seamonkey2.6 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #558356 - Flags: review?(neil)
(Assignee)

Comment 2

6 years ago
Created attachment 558357 [details] [diff] [review]
Patch without whitespace changes

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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
Attachment #558356 - Attachment is obsolete: true
Attachment #558357 - Attachment is obsolete: true
Attachment #558356 - Flags: review?(neil)
Attachment #558584 - Flags: review?(neil)

Comment 5

6 years ago
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

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #558584 - Attachment is obsolete: true
Attachment #558584 - Flags: review?(neil)
Attachment #559298 - Flags: review?(neil)

Comment 8

6 years ago
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).
(Assignee)

Comment 9

6 years ago
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.
Attachment #559298 - Attachment is obsolete: true
Attachment #559298 - Flags: review?(neil)
Attachment #559441 - Flags: review?(neil)
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.
(Assignee)

Comment 11

6 years ago
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.
Attachment #559441 - Attachment is obsolete: true
Attachment #559441 - Flags: review?(neil)
Attachment #559611 - Flags: review?(neil)

Updated

6 years ago
Attachment #559611 - Flags: review?(neil) → review+
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 ;-)
(Assignee)

Comment 13

6 years ago
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
Attachment #559611 - Attachment description: Just fix multiple file save → Just fix multiple file save [Checked in: Comment 13]
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-seamonkey2.6: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.6
(Assignee)

Comment 14

6 years ago
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.
Attachment #559611 - Flags: approval-comm-beta?
Attachment #559611 - Flags: approval-comm-aurora?
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.
Attachment #559611 - Flags: approval-comm-beta?
Attachment #559611 - Flags: approval-comm-beta+
Attachment #559611 - Flags: approval-comm-aurora?
Attachment #559611 - Flags: approval-comm-aurora+
(Assignee)

Comment 16

6 years ago
Comment on attachment 559611 [details] [diff] [review]
Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]

http://hg.mozilla.org/releases/comm-aurora/rev/265622251756
http://hg.mozilla.org/releases/comm-beta/rev/e2962b868add
Attachment #559611 - Attachment description: Just fix multiple file save [Checked in: Comment 13] → Just fix multiple file save [Checked in: trunk Comment 13, aurora/beta Comment 16]
(Assignee)

Updated

6 years ago
status-seamonkey2.3: --- → wontfix
status-seamonkey2.4: --- → fixed
status-seamonkey2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.