1498866 - "Edit Template": template removed by Auto-save (implemented fix: keep template and manage auto-saved drafts)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

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

STR:
Edit Template.
Wait for Auto-save.
Check the template folder, template is gone.

It comes back when the template is saved, but if the user sends or doesn't save, the template is lost.

Aceman came across this issue looking at bug 1498097.

Comment 1

[Jorg K (CEST = GMT+2)]

Actually, the autosave saves it as draft, and the template gets removed.

Comment 2

[Jorg K (CEST = GMT+2)]

OK, here are a few cases:

Edit template
wait, auto-save saves a draft, template deleted
Save template, draft is deleted, template saved
All OK.

Edit template
wait, auto-save saves a draft, template deleted
Close template without saving, draft is kept
Sort of OK, user can move the draft back to the templates folder

Edit template
wait, auto-save saves a draft, template deleted
Send (which is not the intended action here), draft is deleted, set message is kept
Sort of OK, user can move the draft back to the templates folder

Here are the problems:
1) When editing a template, we cannot auto-save intermediate versions into the template
   folder since the user might want to cancel the edit. So auto-save must save a draft.
2) If auto-save saves a draft and leaves the original in the template folder, then
   manually saving can only remove one message, not two. Well, that requires further investigation.
   If we can only open one message, which one do we delete? The intermediate draft or the
   original template? The result will always be that one copy too many.

Comment 3

[Jorg K (CEST = GMT+2)]

Like for editing drafts, editing templates now carries around the "draft" ID, so the old one can be deleted:
https://hg.mozilla.org/comm-central/rev/5dd349d1efbc#l7.91

That the overall approach for drafts doesn't quite fit for templates can be seen in this bug.

Open to good ideas!!

Comment 4

[Jorg K (CEST = GMT+2)]

Easiest way: Disable auto-save when editing a template. Auto-save is for editing new message and drafts where you don't want to lose all the content you're adding. Tweaking a template and losing this due to, well, power failure or some other sever incident, is not that big of a problem. I think the issue that auto-save causes when editing templates is more severe.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1498866-no-auto-save-for-template.patch

That's the best I can come up with.

Any other solution would involve more logic:
0) Detect auto-save during edit template and *not* delete the old version,
   only save a draft.
1) Save after auto-save would have to delete the draft and the old template
   and save the new version to the template folder.
   The current design only allows to delete one message, not two.
2) Abort edit template after auto-save would have to delete the draft.
3) Send after auto-save could just delete the draft, the sent version
   was never saved. The old template remains according to 0).

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> Created attachment 9017044 [details] [diff] [review]
> 1498866-no-auto-save-for-template.patch
> 
> That's the best I can come up with.
> 
> Any other solution would involve more logic:
> 0) Detect auto-save during edit template and *not* delete the old version,
>    only save a draft.

Yes, this would be OK.

> 1) Save after auto-save would have to delete the draft and the old template
>    and save the new version to the template folder.
>    The current design only allows to delete one message, not two.

Why deleting the draft on Save? While the compose is open, it could still exist. Then the draft can be deleted upon send or close of composition.

> 2) Abort edit template after auto-save would have to delete the draft.

Yes.

> 3) Send after auto-save could just delete the draft, the sent version
>    was never saved. The old template remains according to 0).

Yes, OK.

Comment 7

[Jorg K (CEST = GMT+2)]

Thinking about this some more:
If you edit a draft and auto-save kicks in, you lose the previous version. So all we should do for edit template is auto-save the new version to the template folder, and done. That will eliminate all problems. We've already fixed bug 1498097 where sending deleted the template.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1498866-auto-save-template.patch

The definition of auto-save is:
A ghost comes out of a box and clicks "Save" for the user. When editing a draft, that gets saved as a draft, when editing a template, it gets saved as a template.

In both cases the previous version is overwritten. Blame it on the ghost.

I think we can keep it simple and not introduce
  Ci.nsIMsgCompDeliverMode.AutoSaveAsTemplate

That fixes all problems, and after fixing bug 1498097, no template will be deleted after sending.

Comment 9

[Magnus Melin [:mkmelin]]

IIRC filters using templates are tied to a specific template, so if you delete one, you get into trouble. The user can manually delete and get these problems, but be careful with auto-delete.

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks for the hint. The auto-reply filter qualifies the template to use like so:
actionValue="mailbox://nobody@Local%20Folders/Templates?messageId=[e-mail]&subject=French 2"

Editing a template maintains that ID. So as long as auto-save doesn't remove templates and stores drafts instead, we're fine.

A potential trouble is that auto-reply could be sending a template which is being edited and has been auto-saved. So the answer to this would be disabling auto-save for templates as I had previously suggested. I'll recover the patch now.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9017044 [details] [diff] [review]
1498866-no-auto-save-for-template.patch

The commit message should be changed to:
don't auto-save when editing template to conflict with auto-reply filter.

Comment 12

[Jorg K (CEST = GMT+2)]

Oops:
The commit message should be changed to:
don't auto-save when editing template to *avoid* conflict with auto-reply filter.

Comment 13

[:aceman]

Comment on attachment 9017044 [details] [diff] [review]
1498866-no-auto-save-for-template.patch

Review of attachment 9017044 [details] [diff] [review]:
-----------------------------------------------------------------

We can use this as a quick fix for TB60.3, but then please file the bug for the more involved solution (autosaving template to a draft).

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1498866-proper-fix.patch - WIP

Working, but needs some tidying.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9017363 [details] [diff] [review]
1498866-proper-fix.patch - WIP

Review of attachment 9017363 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +3873,5 @@
> +
> +      // Only do this if it's a drafts or templates folder.
> +      uint32_t flags;
> +      msgFolder->GetFlags(&flags);
> +      if (!(flags & (nsMsgFolderFlags::Drafts | nsMsgFolderFlags::Templates)))

You could use isSpecialFolder instead, in case people organize their templates

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1498866-proper-fix.patch - WIP (v2)

I've tidied it up a bit. It mostly works apart from:
- Abandoning edit template after auto-save has happened doesn't
  remove the draft

Also funny: When abandoning edit template, the prompt offers to save as draft or discard changes.

Not tested on IMAP yet.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1498866-proper-fix.patch - v3

This is the Rolls Royce solution. Auto-saved draft now also removed.

Comment 18

[:aceman]

Comment on attachment 9017457 [details] [diff] [review]
1498866-proper-fix.patch - v3

Review of attachment 9017457 [details] [diff] [review]:
-----------------------------------------------------------------

This looks and works beautifully for me.
I tested all cases I could think up on local/pop3 and on IMAP too (gmail). I couldn't find a problem.
The code changes aren't as bad, mostly just keeping the templateId too.
I like it, thanks!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4147,5 @@
>          // don't delete the draft if we didn't start off editing a draft
>          // and the user hasn't explicitly saved it.
>          if (!gEditingDraft && gAutoSaveKickedIn)
>            RemoveDraft();
> +        // Remove auto-saved draftes created duting "edit template".

draft?

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9017457 [details] [diff] [review]
1498866-proper-fix.patch - v3

I need a review to land it. I promise to fix the comment.

Comment 20

[:aceman]

Comment on attachment 9017457 [details] [diff] [review]
1498866-proper-fix.patch - v3

Review of attachment 9017457 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4147,5 @@
>          // don't delete the draft if we didn't start off editing a draft
>          // and the user hasn't explicitly saved it.
>          if (!gEditingDraft && gAutoSaveKickedIn)
>            RemoveDraft();
> +        // Remove auto-saved draftes created duting "edit template".

And "during".

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a312c7cf1c0e
Only delete original template when saving new template, manage auto-saved versions. r=aceman

Comment 22

[Jorg K (CEST = GMT+2)]

TB 63 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/df0d79b1cb646e7ebe57c348e52adc35b5ea025b

Comment 23

[Thomas D. (:thomas8)]

Adjust bug summary to be clearer about what is the actual bug (Auto-Save removes template), and what we have kept/improved (manage auto-saved templates as drafts). Current summary sounds as if "auto-saving edited template as draft in drafts folder" is part of the bug, but this was ultimately not considered a bug here.

This needs more thought, because it'll be hard for users to discover that auto-saved templates end up in drafts folder. We correctly figured that we can't just overwrite the template with its auto-saved version because that might cause filters to send out half-baked templates in undefined states of editing. In an ideal world, auto-save should probably NOT act as a ghost which overwrites the original or creates undiscoverable drafts in some other folder, but offer to restore or dump the auto-saved version when an abandoned message gets re-opened (especially for the template case, due to the filter issue). The auto-saved version should be saved in some plausible location; probably in the same folder where the original lives, maybe invisible to the user (as we currently hide deleted pop messages), and maybe just indicated somehow on the existing template.

Comment 24

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/614a66fc2dd6f8dd94cabd853599e723c15f02bd

Comment 25

[Thomas D. (:thomas8)]

Fwiw, I had mentioned this on the original bug already (albeit unaware of the filter issue), but somehow we didn't pick that up at the time...
(In reply to Thomas D. from bug 1389062 comment #35)
> 1 Bug found (probably pre-existing):
> Auto-Save moves the template msg to drafts folder, but should just overwrite
> existing template.

Thanks Aceman and Jörg for fixing this!