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

Status: RESOLVED DUPLICATE of bug 814000

(Thunderbird :: Preferences, defect)

Description

[Jeff Beckley]

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.

Comment 1

[Jeff Beckley]

Attachment: Proposed patch

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.

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Jeff Beckley]

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

Comment 4

[[e-mail]]

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

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 6

[Jeff Beckley]

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

Comment 7

[[e-mail]]

No, we're currently using a completely different nsIDownloadManager:

http://mxr.mozilla.org/mozilla-central/find?string=nsIDownloadManager.idl&tree=mozilla-central

Comment 8

[Jeff Beckley]

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

Comment 9

[Justin Wood (:Callek)]

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 10

[David :Bienvenu]

Comment on attachment 364880 [details] [diff] [review]
Proposed patch

clearing review request based on standard8's minus.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 14

[Wayne Mery (:wsmwk)]

Jeff is longer active.
THomas, do you see anything left to do here?

Comment 15

[Thomas D. (:thomas8)]

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