Closed Bug 480899 Opened 11 years ago Closed 1 month ago

Attachments folder pref isn't used by saves from attachment context menu (and similar)

Categories

(Thunderbird :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 814000

People

(Reporter: beckley, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In the "Attachments" pref panel there is a setting for whether or not the user should be asked where to save attachments.  Even if the "Save all attachments to this folder" pref is selected, saving an attachment still prompts the user.
Attached patch Proposed patchSplinter Review
Here's an implementation.  Added a new GetAttachmentLocation() function which gets the place to save an attachment based on user prefs (either pref location or ask the user).  This simplified the code a bit as there was some duplication in several places.

I also changed some confirmation dialog button text from OK/Cancel to Yes/No because it made more sense given the question in the dialog.
Assignee: nobody → beckley
Status: NEW → ASSIGNED
Attachment #364880 - Flags: superreview?(bienvenu)
Attachment #364880 - Flags: review?(bugzilla)
Attachment #364880 - Flags: ui-review?(clarkbw)
Attachment #364880 - Flags: review?(bugzilla)
Attachment #364880 - Flags: review-
Comment on attachment 364880 [details] [diff] [review]
Proposed patch

Wow, I'm really surprised there isn't a bug on this already (I couldn't see one).

I kinda like the idea, but I think we need to improve the UI at the same time - I set the pref to "save all attachments to this folder" and when I right-clicked, I still got the "Save As..." option, which if I wasn't thinking would make me expect a dialog etc. There also didn't appear to be any user feedback. Therefore adding clarkbw to the ui-review as I think this needs some extra comments.

Also I think this would currently break SeaMonkey as well - they aren't using the new download manager yet and hence it hasn't got the required code - maybe some careful ifdef to preserve the SM code and allow the changes to the TB code?

>+  if (!lastSaveDir)
>+  {
>+    nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+
>+    // First check if the pref to use a specific attachment directory is set,
>+    // and if so use nsIDownloadManager to get that directory.
>+    if (NS_SUCCEEDED(rv))

nit: just use NS_ENSURE_SUCCESS and it saves indenting plus gives us a warning in debug if something is really wrong.

>+      PRBool useDownloadDir = PR_FALSE;
>+      rv = prefBranch->GetBoolPref(BROWSER_USE_DOWNLOAD_DIR, &useDownloadDir);
>+      if (NS_SUCCEEDED(rv) && useDownloadDir)
>+      {
>+        nsCOMPtr<nsIDownloadManager> downloadManager = do_GetService("@mozilla.org/download-manager;1", &rv);

nit: you're not checking this for failure. Also, please wrap the do_GetService... onto the next line so its two space indented from nsCOMPtr.
(In reply to comment #2)
> Wow, I'm really surprised there isn't a bug on this already (I couldn't see
> one).

Yeah, I looked and couldn't find one.  I was quite shocked myself.  I can't believe a specific pref in the UI has never worked (it appears there was never any code to attempt this), and nobody noticed!


> I kinda like the idea, but I think we need to improve the UI at the same time -
> I set the pref to "save all attachments to this folder" and when I
> right-clicked, I still got the "Save As..." option, which if I wasn't thinking
> would make me expect a dialog etc. There also didn't appear to be any user
> feedback. Therefore adding clarkbw to the ui-review as I think this needs some
> extra comments.

What if we just removed the ellipsis on the menu item if the pref to save to a folder was on?


> nit: just use NS_ENSURE_SUCCESS and it saves indenting plus gives us a warning
> in debug if something is really wrong.

Can't do that because we want the code to fall down below to the case where the user is asked for where to put the attachment.


> >+      PRBool useDownloadDir = PR_FALSE;
> >+      rv = prefBranch->GetBoolPref(BROWSER_USE_DOWNLOAD_DIR, &useDownloadDir);
> >+      if (NS_SUCCEEDED(rv) && useDownloadDir)
> >+      {
> >+        nsCOMPtr<nsIDownloadManager> downloadManager = do_GetService("@mozilla.org/download-manager;1", &rv);
> 
> nit: you're not checking this for failure. Also, please wrap the
> do_GetService... onto the next line so its two space indented from nsCOMPtr.

OK, will do on these.  I think if I handle the case where the download manager isn't found, then SeaMonkey will work correctly because the code will just fall back on the ask-the-user code below it.
(In reply to comment #3)
> I think if I handle the case where the download manager
> isn't found, then SeaMonkey will work correctly because the code will just fall
> back on the ask-the-user code below it.
Unfortunately we can't actually compile that code until bug 381157 is fixed.
I was working on the attachment prefs w/ magnus in bug 446291.  I have to admit that I didn't fully understand the actions of the options, which are much more apparent now that I've seen this bug.

Much of the work in that bug was to clean the prefs panel so it was more understandable.  I had worked on a mockup in attachment 352629 [details] to show how we could essentially remove this option, using the FF download pref to accomplish similar things.  I like the direction of the last mockup, but it would seem to conflict with the goals of this bug.  I'll try to work on something new.  Suggestions wanted.
Summary: Attachments folder pref isn't used → Attachments folder pref isn't used by saves from attachment context menu (and similar)
(In reply to comment #4)
> Unfortunately we can't actually compile that code until bug 381157 is fixed.

Isn't IDownloadManager in the toolkit that SeaMonkey uses?  I would think it
would be, it's just that SeaMonkey doesn't *use* it.  Seems like it would
compile, but maybe I'm just being naive.  Also, it appears that the work in
that bug is geared toward using the download manager for web browsing, not
email attachments.  And that it's mainly addressing UI issues, not the service
on the back end.
No, we're currently using a completely different nsIDownloadManager:

http://mxr.mozilla.org/mozilla-central/find?string=nsIDownloadManager.idl&tree=mozilla-central
(In reply to comment #7)
> No, we're currently using a completely different nsIDownloadManager:

OK.  So maybe I can just #ifdef out that section for SM.  I could also conditionally include it using SUITE_USING_TOOLKIT_DM, which seems to be in vogue for bug 381157.  Sound good?
ifdef with SUITE_USING_XPFE_DM is good imo;

see: http://hg.mozilla.org/comm-central/file/ca0c7ad9df65/suite/app-config.mk#l43

We should hopefully switch to toolkit soon anyway. (last patch on Bug 381157 is old diff by accident, new one will be posted there tonight)
Comment on attachment 364880 [details] [diff] [review]
Proposed patch

clearing review request based on standard8's minus.
Attachment #364880 - Flags: superreview?(bienvenu)
Attachment #364880 - Flags: ui-review?(clarkbw) → ui-review-
Comment on attachment 364880 [details] [diff] [review]
Proposed patch

We need to make the preference option clearer than it is right now.

I think if we implemented the mockup from bug 446291 we could have something workable even though that UI isn't quite finished.
Component: Message Reader UI → Preferences
QA Contact: message-reader → preferences
Duplicate of this bug: 372529
Hey Jeff, just checking in.  bug 446291 has landed the new attachments pref pane and I have a little time to work on this.  Ping me if you're free.
Jeff is longer active.
THomas, do you see anything left to do here?
Assignee: beckley → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(thomasd.bugzilla)

(In reply to Wayne Mery (:wsmwk) from comment #14)

Jeff is longer active.
THomas, do you see anything left to do here?
ni?thomasd.bugzilla(at)...

Unfortunately, I think this needinfo has never reached me because it was not using my bmo user name / email address (bugzilla2007@...), so to this day it has never showed up in my BMO dashboard.

Yes, the problem of this bug is still around.
Duplicate Bug 814000 seems to have more information.

Status: NEW → RESOLVED
Closed: 1 month ago
Flags: needinfo?(thomasd.bugzilla)
Resolution: --- → DUPLICATE
Duplicate of bug: 814000
See Also: → 814000
See Also: 814000
You need to log in before you can comment on or make changes to this bug.