Closed Bug 654007 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1final

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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
Attached patch Patch v1.0 (obsolete) — Splinter Review
> +      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.
Attachment #529372 - Flags: review?(neil)
Attachment #529372 - Flags: feedback?(jh)
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.
(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 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!
Attachment #529372 - Flags: feedback?(jh) → feedback+
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.
> Why did you keep the now unused "folder" parameter?
I forgot!
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.
Attachment #529372 - Flags: review?(neil) → review+
Nits fixed.
Requesting moa/sr from module owner.
Attachment #529372 - Attachment is obsolete: true
Attachment #529498 - Flags: superreview?(mnyromyr)
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.
Attachment #529498 - Flags: superreview?(mnyromyr) → superreview+
> moa=me if fixing NNTP here is too complex; please file a new bug for it then.
Err, Do NNTP accounts have template folders?
Pushed:
http://hg.mozilla.org/comm-central/rev/7f684b5a41a8
http://hg.mozilla.org/releases/comm-2.0/rev/920ec0fdecc7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
> 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?
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.
Blocks: 654966
You need to log in before you can comment on or make changes to this bug.