654007 - Can't save as template via File > Save As Menu [SeaMonkey Part]

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

+++ This bug was initially created as a clone of Bug #532050 +++

> Using IMAP and Thunderbird 3.0rc1:
> 
> * Search for a message.
> * Save the selected message as a tab via File > Save As > Template -> FAILS
> 
> * Open selected message in a new tab
> * Try again to save the opened message via File > Save As > Template -> FAILS
> 
> Expected:
> 
> * Saving message as template works from within tabs

Comment 1

[Philip Chee]

Attachment: Patch v1.0

> +      let isImap = templates.server.type == "imap";
> +      templates.createStorageIfMissing(new saveAsUrlListener(uri, identity));
> +      if (isImap)
> +        return;
> +    }
>      messenger.saveAs(uri, false, identity, null);
>    }
>  }

I don't understand why templates.createStorageIfMissing() isn't inside the if (isImap) block.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 529372 [details] [diff] [review]
Patch v1.0

Drive-by nits (cannot test this now, I'm currently building the transplant of the other six patches to the branch):

>+  OnStartRunningUrl: function(aUrl) {
>+  },
>+  OnStopRunningUrl: function(aUrl, aExitCode) {
>+    messenger.saveAs(this.uri, false, this.identity, null);
>+  }

and

>+    if (!templates.parent) {

Curly braces on new lines, or Karsten will hunt you down. This. Is. MailNews!

>+  if (GetNumSelectedMessages() == 1) {
>+    SaveAsTemplate(GetFirstSelectedMessage());
>+  }

If you're changing those lines and get this past the reviewers, you should get rid of these curly braces altogether.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> I don't understand why templates.createStorageIfMissing() isn't inside the if
> (isImap) block.

Do you think it should not be called for local folders? Keep in mind that this function also works for those:
http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js#1266

I haven't checked, but maybe there could be a situation where the Templates folder server is set to Local Folders but the folder doesn't exist (e.g. after switching the Templates folder definition back and forth or something).

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 529372 [details] [diff] [review]
Patch v1.0

Seems to work as advertised, i.e. only with the patch a missing Templates folder is created on the IMAP server (given the IMAP server has been defined to be hosting the Templates folder for messages on the IMAP server).

Why did you keep the now unused "folder" parameter? TB has removed it, too, so it'd only give you some backwards compatibility, right?

Nit: I'd define the isImap local variable exactly one line before it is used. Of course there's no real need for the variable in the first place since it's only used once, but name/scheme is used elsewhere, too, so +1 for consistency!

Comment 5

[David :Bienvenu]

CreateStorageIfMissing creates the actual templates folder on disk, if it doesn't exist, even for local folders. If you remove that line, you won't create a local templates folder.

Comment 6

[Philip Chee]

> Why did you keep the now unused "folder" parameter?
I forgot!

Comment 7

[[e-mail]]

Comment on attachment 529372 [details] [diff] [review]
Patch v1.0

Looks good to me, but I don't know how much attention you need to pay to the nits as mentioned by Jens.

Comment 8

[Philip Chee]

Attachment: Patch v1.1 f=InvisibleSmiley r=Neil

Nits fixed.
Requesting moa/sr from module owner.

Comment 9

[Karsten Düsterloh]

Comment on attachment 529498 [details] [diff] [review]
Patch v1.1 f=InvisibleSmiley r=Neil

>+function SaveAsTemplate(uri)
> {
>   if (uri) {

You're rewriting most of the function, so you might as well fix this stray curly brace position. ;-)


Actually, this patch doesn't seem to work completely:
It does work if the message to be saved is local/Movemail/RSS/POP3 or IMAP, but it's still broken if I try to template an NNTP message.

moa=me if fixing NNTP here is too complex; please file a new bug for it then.

Comment 10

[Philip Chee]

> moa=me if fixing NNTP here is too complex; please file a new bug for it then.
Err, Do NNTP accounts have template folders?

Comment 11

[Philip Chee]

Pushed:
http://hg.mozilla.org/comm-central/rev/7f684b5a41a8
http://hg.mozilla.org/releases/comm-2.0/rev/920ec0fdecc7

Comment 12

[Philip Chee]

> It does work if the message to be saved is local/Movemail/RSS/POP3 or IMAP, but
> it's still broken if I try to template an NNTP message.

That's because |nsIMsgFolder::createStorageIfMissing()| isn't implemented for NNTP accounts :P. Do you still want me to file a NNTP bug?

Comment 13

[Karsten Düsterloh]

Saving (In reply to comment #12)
> > It does work if the message to be saved is local/Movemail/RSS/POP3 or IMAP,
> > but it's still broken if I try to template an NNTP message.
> 
> That's because |nsIMsgFolder::createStorageIfMissing()| isn't implemented for
> NNTP accounts :P.

That's not the full story. For me, 
| MailUtils.getFolderForURI(identity.stationeryFolder, false);
delivers "Local Folders/Templates".

Maybe NNTP accounts are just "too special", because I doubt that Template management for NNTP via Local Folders would be particularily useful. :-/
You got a point here, true.

> Do you still want me to file a NNTP bug?

Yes, because the menu item shouldn't be active if it doesn't do anything useful.