After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747627 - Close and Save not saving drafts
Close and Save not saving drafts
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: composer
0.10.x
Other Linux
: High critical
: 0.11.3
Assigned To: Michael Gratton
Geary Maintainers
: 749813 749943 757992 759285 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-04-10 15:04 UTC by pvanbmail-guard@yahoo.com
Modified: 2016-12-20 14:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
--debug log (151.36 KB, text/x-log)
2015-04-21 00:17 UTC, Giovanni Campagna
  Details
Only enable the close-and-save button when the draft manager is available (1.33 KB, patch)
2015-05-18 15:25 UTC, Robert Schroll
none Details | Review
Only enable the close-and-save button when the draft manager is available (1.33 KB, patch)
2015-05-18 15:31 UTC, Robert Schroll
none Details | Review
patch to debug draft saving (1.08 KB, patch)
2015-08-10 08:02 UTC, Federico Bruni
none Details | Review
Trying to save drafts log. (64.66 KB, text/x-log)
2016-06-12 14:46 UTC, Rafael Vega
  Details
initialize draft_manager also when more accounts are configured (806 bytes, patch)
2016-09-20 19:11 UTC, Jiri Cerny
none Details | Review
Patch for master (9.02 KB, patch)
2016-09-21 12:38 UTC, Michael Gratton
none Details | Review
patch proposal v1 (4.71 KB, patch)
2016-10-13 15:11 UTC, Gautier Pelloux-Prayer
none Details | Review
patch proposal v2 (13.56 KB, patch)
2016-10-17 11:08 UTC, Gautier Pelloux-Prayer
none Details | Review
patch proposal v3 (1.48 KB, patch)
2016-10-26 09:19 UTC, Gautier Pelloux-Prayer
none Details | Review
patch proposal v4 (1.42 KB, patch)
2016-11-21 13:44 UTC, Gautier Pelloux-Prayer
committed Details | Review

Description pvanbmail-guard@yahoo.com 2015-04-10 15:04:05 UTC
After writing a message, when I use the option 'close and save' the message is not saved / nowhere to be found. 

I have enabled the option "save drafts on server". Running geary --debug --log-network > geary.log gives not output / empty geary.log file.
Comment 1 Robert Schroll 2015-04-10 17:04:41 UTC
What mail provider are you using?

The lack of log output is odd.  We've had trouble with Geary not quitting correctly.  In that situation, running the Geary command a second time will only trigger the first instance to re-show itself.  Please check that you don't have a geary process running before trying to get the debug output.  If there is one running, you can kill it safely.
Comment 2 pvanbmail-guard@yahoo.com 2015-04-10 20:40:44 UTC
(In reply to Robert Schroll from comment #1)
> What mail provider are you using?
> 
> The lack of log output is odd.  We've had trouble with Geary not quitting
> correctly.  In that situation, running the Geary command a second time will
> only trigger the first instance to re-show itself.  Please check that you
> don't have a geary process running before trying to get the debug output. 
> If there is one running, you can kill it safely.

You are right, Geary hadn't quit, Is this because of the option "always watch for email" - I was assuming so because even after closing Geary, I was still getting emails.

After killing the Geary process and restarting, drafts are saved using Gmail and a personal IMAP email account, but not for my Yahoo.
Comment 3 Robert Schroll 2015-04-10 21:09:04 UTC
Yeah, that could be it.

Were you able to capture the debug output when a draft failed to save?  If so, it'd be a great help if you can post that here.  (Feel free to redact any personal information for the log.)
Comment 4 Giovanni Campagna 2015-04-21 00:07:28 UTC
I'm having the same problem: if I close a draft message using the save button, the message is not saved.

OTOH, it gets saved if I wait long enough that "Saved" pops up under the To/Cc fields, but further edits after that are lost.

Note that I don't need to restart Geary to experience the loss of the message: just moving around between folders (to trigger the reload of Drafts) is enough to lose the email content.

This is on a custom IMAP server, configured to save sent and drafts email on the server. I don't know the server side software or version, but I can try and look it up if necessary.
The client side is geary-0.10.0-5-g6eb437d.
Comment 5 Giovanni Campagna 2015-04-21 00:17:46 UTC
Created attachment 302034 [details]
--debug log

The setup here is 3 accounts, one Gmail, one CUSTOM_IMAP_SERVER for gcampagn@stanford.edu, and one OTHER_CUSTOM_IMAP_SERVER for PRIVATE_EMAIL.

The reproducer is: I run geary, it loads CUSTOM_FOLDER on the GMail account, I switch to the gcampagn@stanford.edu Inbox, click "new", type address and subject, body is autofilled with signature, I click "save" and I jump back to the @stanford.edu Inbox, I switch to Drafts and back to Inbox, the email is lost,
I quit geary from the app menu.
Comment 6 Robert Schroll 2015-04-21 00:48:08 UTC
From that log, I don't see anything that suggests Geary even tried to save the draft.

I looked at the handler for the close-and-save action.  It checks a can_save() boolean.  If that's true, it runs the draft-saving code.  If it's false, it just exits right away.  Sometimes this is obviously the right thing to do (nothing in the draft to be saved, for example.  But in other cases (the account doesn't support drafts), it's less clear.

However, you are able to save drafts via the timer, which checks to the same can_save() method.  If you're willing and able, could you add some debug statements to the save_draft() and save_and_exit_async() methods in src/client/composer/composer-widget.vala?  If you can check on the value of can_save() at those points, it'd be great.  But just finding out what path the code is taking when you hit the save-and-exit button would be helpful.
Comment 7 Giovanni Campagna 2015-05-03 23:24:02 UTC
Actually, I was wrong in my previous description: for new messages there is no working Saved timer. That works for replies though.

I'll add the debug code to geary and I'll let you know.
Comment 8 Robert Schroll 2015-05-04 01:21:54 UTC
That actually makes more sense.  For some reason, can_save() is returning false.  There are four tests in that method.  If you can tell us which is failing, that'll narrow things down quite a bit.
Comment 9 freddi34 2015-05-18 10:07:00 UTC
If I understand right, my email is also forever lost now?

> But in other cases (the account doesn't support drafts), it's less clear.
I think the label gives (to the user) an apparently clear promise that it exits and saves. There is no obvious "maybe" or at least an "abort" if unsupported. If it is known that the feature is incomplete, the feature should not be enabled or at least correctly labeled.

An option for future consideration would be to save the draft locally...
Comment 10 Robert Schroll 2015-05-18 15:25:57 UTC
Created attachment 303538 [details] [review]
Only enable the close-and-save button when the draft manager is available

Yeah, that email is gone forever.  Sorry.

This patch should disable the close-and-save button when the account 
does not support saving drafts.  I assume this is what's causing 
problems in this bug, but I can't be sure.  If those affected can try 
it, let me know whether it disables this button for your problematic 
account.

There's a separate issue of why this account doesn't support drafts, but 
let's get to that later.
Comment 11 Robert Schroll 2015-05-18 15:31:08 UTC
Created attachment 303540 [details] [review]
Only enable the close-and-save button when the draft manager is available

Let's try it with another semicolon.
Comment 12 Federico Bruni 2015-05-24 16:11:38 UTC
I'm having the same problem described by Giovanni in comment #4.
But my account does support saving drafts: if I reply to a message, the message is autosaved in the server. Only if I create a _new_ message, there's no autosave and when I click on Save and quit button I lose the message.
Comment 13 Robert Schroll 2015-05-25 02:22:44 UTC
Federico, do you have multiple accounts set up in Geary?  (The code path is a bit different in that case, so I'm wondering if that could be the culprit.)  If you run my patch, is the close-and-save button disabled?
Comment 14 Federico Bruni 2015-05-25 06:00:41 UTC
Yes, on my laptop I have also a gmail account (with drafts on server disabled because of the gmail mobile client bug).
I've applied your patch: the close-and-save button is not disabled.
Comment 15 Federico Bruni 2015-05-25 07:01:52 UTC
On my desktop, where I have only one IMAP account, I have the same problem.
Let me know if you need more info
Comment 16 Robert Schroll 2015-05-25 14:38:40 UTC
Well, so much for that idea.

If you feel like doing some debugging, can you put some code in the ComposerWidget.can_save() method to tell us which of the four conditions is returning false?
Comment 17 Federico Bruni 2015-06-13 09:04:35 UTC
Robert, who are you asking? :-)
Maybe Giovanni? I'm not a developer...

I believe that these are the 4 conditions:

    private bool can_save() {
        return draft_manager != null
            && draft_manager.is_open
            && editor.can_undo()
            && account.information.save_drafts;
    }

How can I test which is failing? Should I print the booleans of each condition in the terminal? How?

The fourth condition should be ok:
$ cat ~/.local/share/geary/########/geary.ini | grep drafts
drafts_folder=
save_drafts=true

The third condition also, as I can undo.

No idea how to test the first two.
Comment 18 Robert Schroll 2015-06-14 22:43:39 UTC
I'm asking anyone who sees this problem and feels like mucking about a bit.  I haven't been able to reproduce it, so it's hard for me to debug.

What you can do is add some lines like

    debug("draft_manager is not null: %s", (draft_manager != null) ? "true" : "false");
    debug("draft_manager is open: %s", draft_manager.is_open ? "true" : "false");
    debug(editor can undo: %s", editor.can_undo() ? "true" : "false");
    debug("account can save drafts: %s", account.information.save_drafts ? "true" : "false");

before that return statement.  If you compile geary and run it with the --debug switch, you should get four lines printed each time this function is called, letting us know the state of each condition.
Comment 19 Federico Bruni 2015-06-16 19:54:50 UTC
Thanks, here's the output:

[deb] 21:51:45 0,871563 composer-widget.vala:1085: draft_manager is not null: false
![crt] 21:51:45 0,000006 geary_app_draft_manager_get_is_open: assertion 'GEARY_APP_IS_DRAFT_MANAGER (self)' failed
 [deb] 21:51:45 0,000002 composer-widget.vala:1086: draft_manager is open: false
 [deb] 21:51:45 0,000005 composer-widget.vala:1087: editor can undo: true
 [deb] 21:51:45 0,000004 composer-widget.vala:1088: account can save drafts: true
Comment 20 Robert Schroll 2015-06-16 22:07:09 UTC
Ok.  The draft manager isn't getting set up correctly for some reason.

Can you check your logs to see if you have an message that begins, "Unable to open draft manager"?  If so, please post the the whole message here.
Comment 21 icasdri 2015-07-17 02:04:40 UTC
weffewwef
Comment 22 icasdri 2015-07-17 02:05:51 UTC
I apologize for the previous comment. The "Save Changes" button for Add Comments appeared to be disabled and I was trying to figure out what was going on.

Here was my attempted message:

Hello. I am having this issue as well -- after clicking "Save and Close" on a new draft (draft created using "Compose"), the composer closes but the draft is no where to be found. 

Geary currently manages two of my email accounts: one from gmail and one from a imap provider. The draft does not get saved in both cases: whether I try "Compose" from the gmail or the account, does not matter.

Reading the previous responses above, performing a draft save with `geary --debug --log-network` -- a grep on the ensuing logs for "save" or "Unable to" results in no results.

Observing the UI, when the save button is clicked, the composer disappears real fast, with no sort of indication that any sort of io or network traffic is going on -- so can_save() could very well be the culprit.

Some hopefully useful version information:
geary 0.10.0
webkitgtk 2.4.9
gtk3 3.16.4

On a side note, it was stated above that the current behaviour is "If can_save() is false, it just exits right away." This seems very odd, as the user doesn't get the impression that something is awry and data loss (the draft is gone) ensues. I feel like a dialog indicating an error an save would be more appropriate.
Comment 23 Federico Bruni 2015-08-10 08:01:01 UTC
For the records, I could not find any message "Unable to open...". I sent my log file privately to Robert in June.

icasdri, did you build geary using the debug lines suggested by Robert?
Find attached a git patch which you can easily apply with:

git am 0001-bug-747627-debug-draft-saving-error.patch
Comment 24 Federico Bruni 2015-08-10 08:02:12 UTC
Created attachment 309001 [details] [review]
patch to debug draft saving
Comment 25 Federico Bruni 2015-08-10 08:34:29 UTC
I forgot to add another behaviour I noticed: when I open a new message, typing is very slow if I keep the composer "inline" and it's normal if I detach it.

I don't know if this could be somehow related.
Anyway, the drafts is not saved in both situations (detached or not).
Comment 26 Adam Dingle 2016-03-26 11:17:08 UTC
*** Bug 749813 has been marked as a duplicate of this bug. ***
Comment 27 Michael Gratton 2016-05-16 11:59:10 UTC
*** Bug 759285 has been marked as a duplicate of this bug. ***
Comment 28 Michael Gratton 2016-05-16 12:04:20 UTC
This is separate, but probably related, to bug #749943 - that's about Save not even being presented as an option (and autosave not working). This is about the Save & Close button not actually working.
Comment 29 Michael Gratton 2016-05-16 12:07:26 UTC
*** Bug 757992 has been marked as a duplicate of this bug. ***
Comment 30 Georg Hackel 2016-05-25 16:30:35 UTC
I also got this bug in version 0.11 on Arch Linux, using a single mail-provider (uberspace.de)
Comment 31 Rafael Vega 2016-06-12 14:46:36 UTC
Created attachment 329638 [details]
Trying to save drafts log.
Comment 32 Rafael Vega 2016-06-12 14:47:07 UTC
I'm compiling from master branch in https://github.com/GNOME/geary.git and I get this bug. I added the debug statements suggested in this thread, recompiled, installed and did the following:

1. Ran `geary --debug > geary.log`, my imap account had "save drafts on server" disabled.
2. Composed new message, clicked "close and save", nothing was saved.
3. Checked the "save drafts on server" option in the account settings.
4. Composed another message, clicked "close and save", nothing was saved.
5. Quit geary.

FWIW: The using the same account (server) with other email clients (thunderbird) allows me to save drafts and the drafts saved from there show up on geary under the Drafts folder.

Attached the log.

This is the only issue preventing me to switch completely to geary! :)
Comment 33 Michael Gratton 2016-06-16 02:38:52 UTC
Hi Rafael, thanks for the extra details. A few questions:

Can I confirm that you did indeed have "Save drafts on server" unchecked for that account? If so, I would expect that Geary would not give you the option to save it anywhere. This might be what Robert's patch is supposed to do.

Does saving drafts work if you start Geary with "Save drafts on server" checked?

Also, do you only have one account configured?

Your server seems to be based on Dovecot, do you know what version?
Comment 34 Michael Gratton 2016-06-16 02:40:43 UTC
Hi dev@haeckle.de,

(In reply to dev from comment #30)
> I also got this bug in version 0.11 on Arch Linux, using a single
> mail-provider (uberspace.de)

Can you provide a few more details? Is "Save drafts on server" checked in that account's settings? What is the host name you use for your IMAP server? Thanks!
Comment 35 Michael Gratton 2016-06-16 02:43:28 UTC
Hey Federico,

(In reply to Federico Bruni from comment #25)
> I forgot to add another behaviour I noticed: when I open a new message,
> typing is very slow if I keep the composer "inline" and it's normal if I
> detach it.

I've seen this as well, not not for a while. Have you had this problem using 0.11? Do you have "Save drafts on server" checked for your (accounts)? What hostname or server software are your accounts using? Ta!
Comment 36 Michael Gratton 2016-06-16 02:45:50 UTC
Actually, I guess I want to ask everyone CC'ed on this bug:

- Are you having this problem when running Geary 0.11?
- How many accounts do you have configured?
- Do you have "Save drafts on server" for any/all/none of these accounts?
- What is the host name and/or IMAP server used for each of these accounts?

Thanks!
Comment 37 Federico Bruni 2016-06-16 06:17:06 UTC
I cannot remember the last time the composer was slow when typing text in a new message. I think that this issue was fixed in Geary 0.11.

But I still have 100% of times the main problem described in this issue.
The problem occurs either with one or two accounts.
I always have "Save drafts on server" enabled.
The IMAP server of my @inventati.org account is Dovecot. I don't know which version of Dovecot... I can send an email to Autistici team and ask.

You may find some information here:
http://www.autistici.org/en/services/mail.html
Comment 38 Georg Hackel 2016-06-16 07:41:09 UTC
(In reply to Michael Gratton from comment #34)
> Hi dev@haeckle.de,
> 
> (In reply to dev from comment #30)
> > I also got this bug in version 0.11 on Arch Linux, using a single
> > mail-provider (uberspace.de)
> 
> Can you provide a few more details? Is "Save drafts on server" checked in
> that account's settings? What is the host name you use for your IMAP server?
> Thanks!

"Save drafts on server" is checked.
The hostname is "corvus.uberspace.de"
Comment 39 Khurshid Alam 2016-08-19 09:56:53 UTC
Same here.

If "Save drafts on server" is unchecked geary should at-least try to save it locally or present user a option to actually save it on server before closing.

"Save drafts on server" has a old bug which creates lots of in-progress draft messages which appears within inline conversation on Gmail Android app. That is why I always keep it unchecked for gmail accounts. But then it can not save new drafts at all.

And what's with "close & save"? Shouldn't it be "save & close"?
Comment 40 Federico Bruni 2016-08-19 10:12:49 UTC
For local drafts folder see issue #713581

I haven't used Gmail in Geary for a while, but the problem you describe was fixed more than 2 years ago, see issue #725238

I agree that "Save and close" would be better.
Comment 41 Jiri Cerny 2016-09-20 19:08:33 UTC
I have the same problem "Save and close" does not work and no drafts are saved on the server automatically even if "save_drafts" option is true. 

I did some debugging (with the current git master) starting with the hint in comment #17. Note that I have two accounts in geary, one gmail and one private. 

What I observe that draft_manager is null so can_save() return false.
The only place where draft_manager is initiated is in 
open_draft_manager_async() but this function is actually never called.

It can be called either from on_from_changed() or directly from the code on line 609 in composer/composer-widget.vala , where it stands
if (!from_multiple.visible)
            open_draft_manager_async.begin(null);

With two accounts from_multiple.visible is true, so this condition always fail. 
On the other hand, on_from_changed() is called only if from really changes. And indeed, if I use the combo box to change the from address, the saving drafts and "close and save" button both start working. 

The following patch solves the problem for me, but I did not test it extensively.
Comment 42 Jiri Cerny 2016-09-20 19:11:27 UTC
Created attachment 335949 [details] [review]
initialize draft_manager also when more accounts are configured

The patch from comment #41
Comment 43 Michael Gratton 2016-09-20 23:30:40 UTC
Hey Jiri, thanks for the patch! Your analysis is the same conclusion I came to. The patch is for master, right?

Everyone: Can you test out the patch in attachment 335949 [details] [review] and report back here if it fixes this issue for you?

I've got some patches baking that takes the same approach to fix the saving issue, cleans up the draft manager code path gymnastics a bit,  updates the state of the Close and Save button, and fixes a memory leak. If there's some positive feedback about Jiri's patch above, I'll hurry up and land this stuff on both master and the stable 0.11 branch.
Comment 44 Jiri Cerny 2016-09-21 06:41:21 UTC
Michael, yes the patch is for master, but it applies to 0.11 as well.
Comment 45 Federico Bruni 2016-09-21 11:15:53 UTC
The patch cannot be applied with 'git am'. Not created with 'git format-patch'...
And I cannot remember the correct patch command to apply it. i tried `patch -p0 < file.patch` and also -p1, but didn't work.
Comment 46 Jiri Cerny 2016-09-21 12:01:36 UTC
Federico,

the patch does not apply to the today master, because of commits that landed in the last 12h. But it is trivial to make the modification, since the patch (modulo formating) only deletes the line

if (!from_multiple.visible)

preceding 
            open_draft_manager_async.begin(null);

in the file src/client/composer/composer-widget.vala
Comment 47 Michael Gratton 2016-09-21 12:36:48 UTC
(In reply to Jiri Cerny from comment #46)
> the patch does not apply to the today master, because of commits that landed
> in the last 12h.

This was my bad, sorry! I'll attach something here in a moment that should apply cleanly to master. Jiri's patch should still apply cleanly to the geary-0.11 branch however.
Comment 48 Michael Gratton 2016-09-21 12:38:32 UTC
Created attachment 335990 [details] [review]
Patch for master

A slightly more elaborate version of Jiri's patch, which should cleanly apply to master using `git am`.
Comment 49 Federico Bruni 2016-09-22 03:36:08 UTC
Thanks Mike!
I  confirm that fixes the problem: I can close and save a new mail now.
Comment 50 Michael Gratton 2016-09-23 00:54:43 UTC
Thanks for the feedback Federico.

Since that makes three of us, I have just pushed two patches for this to master: 0b299e fixes the underlying problem saving drafts, and c9acc6 ensures the Close and Save button's state accurately reflects the account's settings and the draft manager's state.

I have a backport of these to the stable branch, I'll push them once the above have received some wider testing master.
Comment 51 Gautier Pelloux-Prayer 2016-10-04 21:18:58 UTC
I personally still have the issue on master: draft manager is NULL. Will investigate further.
Comment 52 Michael Gratton 2016-10-05 00:00:41 UTC
(In reply to Gautier Pelloux-Prayer from comment #51)
> I personally still have the issue on master: draft manager is NULL. Will
> investigate further.

Yeah, same here, on master. I have multiple accounts, and I think all I need to reproduce it is do Compose New Message. If I then change the From address to a different account, then it starts saving drafts.

So perhaps the code that updates the initialises the From combobox in the case of multiple accounts is clearing the draft manager that is being set up when ComposerWidget is first constructed?
Comment 53 Federico Bruni 2016-10-05 07:06:46 UTC
Same here on 0.11.0+212~ge7ace76

Something happened between 22nd of September and now...
Comment 54 Gautier Pelloux-Prayer 2016-10-06 15:23:15 UTC
> If I then change the From address to a different account, then it starts saving drafts.

From my experience that's not exactly sufficient. Here's what happens with my 2 accounts:

1) Compose new mail 
    a) Mail cannot be saved because draft manager is not initialized
2) Switch the from Field
    a) For a few seconds, this.draft_manager.is_open returns False, probably because it is opened asynchronously. During that time, you cannot save your draft (it might be OK, if opening takes less than a few seconds which seems to be the case right now)
    b) But even after that, mail cannot be saved because now this.editor.can_undo() returns False for whatever reason I did not found yet
3) Type something in the mail body
    a) Finally this.editor.can_undo() returns now True and you can successfully save your draft!

There are at least 2 problems:

- 1a) obviously
- 2b) obviously too
- Why saving draft requires to have entered something in mail body? Having entered recipients + subject could be worth saving too in my opinion, I am not sure why can_save() checks for this.editor.can_undo()
- 2a) might be a problem or at least in that specific scenario Geary could inform the user that retrying in a few moment should be OK?
Comment 55 Gautier Pelloux-Prayer 2016-10-13 15:11:39 UTC
Created attachment 337615 [details] [review]
patch proposal v1

Here's a patch that:

- initialize draft manager on launch
- display a warning to the user if he hits "Close and Save", if the draft cannot be actually saved (TODO: it would be better to disable the button?)
- Change text "Close and Save" -> "Save and Close"
- Don't ask user to "Close all opened drafts" in case they are empty

There are still a few things missing:

- Review behavior of close draft popups (Save and close vs close and discard vs close) as seen in https://bugzilla.gnome.org/show_bug.cgi?id=772514#c9 Does that require an extra bug or can we deal with it here?
- Disable "save draft" button if we actually cannot (eg when can_save() returns false).
Comment 56 Michael Gratton 2016-10-15 07:30:56 UTC
Review of attachment 337615 [details] [review]:

Hey Gautier, thank for the patch.

Looking through the code, I think my commit 6b58da6 re-broke this. Specifically, the if-test at geary-controller.vala:2289 is incorrect: The call to widget.open_draft_manager_async() should always happen, regardless of the value of is_draft, but referred.id should only be passed in if is_draft is true.

So I have pushed ec2fdb0 with a fix for that. I also pushed 41f47bb, with the Close/Save swap (also Close/Discard for consistency).

However! Not all is lost for this patch - the remainder is actually applicable to Bug 743970, so if you could rebase it off current master, then we can get that fixed. Something I'd like to see as part of that is maybe dropping one of ComposerWidget.blank or ComposerWidget.can_save(), since they seem mostly redundant.
Comment 57 Michael Gratton 2016-10-15 07:49:27 UTC
(In reply to Gautier Pelloux-Prayer from comment #55)
> There are still a few things missing:
> 
> - Review behavior of close draft popups (Save and close vs close and discard
> vs close) as seen in https://bugzilla.gnome.org/show_bug.cgi?id=772514#c9
> Does that require an extra bug or can we deal with it here?

I'd open a new bug. This one is just about drafts not being saved.

> - Disable "save draft" button if we actually cannot (eg when can_save()
> returns false).

That's worth addressing here, since it's a visual cue about whether drafts will be saved at all.
Comment 58 Gautier Pelloux-Prayer 2016-10-17 11:08:53 UTC
Created attachment 337829 [details] [review]
patch proposal v2

> I'd open a new bug. This one is just about drafts not being saved.
I'm still thinking about that. Yet I modified the popup title and the button from "Keep" to "Save"; I find this clearer. Any issue with that change? 
I also modified a bit AlertDialog API (reordering fields), to allow having 2 style for TertionaryDialog, useful here (save = suggested action, discard = destructive action). The tertionary choice also moved to the 2nd position so that Cancel button is always the most left one.

> That's worth addressing here, since it's a visual cue about whether drafts will be saved at all.
I have added support for that by connecting multiples signals. I did not found any more robust way, can you confirm?

I removed editor.can_undo() which looks really broken or at least does not fit our use here; instead checking that the editor contains some non blank text makes the trick here. Unfortunately the Webview editor automatically adds some <div> and/or <br> so a regex \s\R is needed to check if it is empty. Does that matter?
Comment 59 Michael Gratton 2016-10-19 01:00:23 UTC
Review of attachment 337829 [details] [review]:

I'm not against cleaning up the dialogs, but again it is completely unrelated to this bug. Apologies for being so obstinate about this, but I don't want mix up UI rework with what should have otherwise been a straight-forward, but critical feature bug. I'm happy to review the dialogs, but in a separate bug — if it's worth doing then we should examine them all, at the same time, to ensure they are consistent for both users and translators.

Isn't it worth tying the state of the Save & Close button to the state of the drafts manager, rather than whether the app thinks the message is worth saving or not? In the end that's what determines if a draft can be saved or not, and hence whether clicking the button will actually do anything. There's already some code in there for that, so maybe that's just a bit buggy? This would also make it unnecessary to hook up to all those entry signals.

Anyway, there are a lot of changes to composer-widget.vala in this patch that won't work with the WebKit2 branch: We won't have any idea if we can undo or get access to the text of the WebView without async, inter-process calls, the WebView::user-changed-contents signal has been removed, and the composer won't have access to the DOM at all, for example. Again, if the changes relating to Bug 743970 can be spit out into a separate patch for that bug, can at least look at landing that part, and any changes to manage the state of the Save & Close button will need to be pretty minimal as well.
Comment 60 Gautier Pelloux-Prayer 2016-10-26 09:19:52 UTC
Created attachment 338489 [details] [review]
patch proposal v3

OK, I reduced the patch to a minimum version:

1. Disable "draft button" until draft manager is ready, so that if it cannot be opened user cannot to save.
2. There is a "watch dog" in on_close_and_save: even if user manages to activate it somehow, and if draft cannot be saved: do NOT discard the draft silently, but instead show the alert popup "This draft cannot be saved…".
Comment 61 Gautier Pelloux-Prayer 2016-11-21 12:26:33 UTC
Does it look fine Michael?
Comment 62 Michael Gratton 2016-11-21 12:48:48 UTC
Review of attachment 338489 [details] [review]:

Very happy with part (2), just one small issue to iron out for (1):

::: src/client/composer/composer-widget.vala
@@ +722,3 @@
+
+        close_and_save.set_enabled(false);
+        this.header.save_and_close_button.hide();

I think this is causing a flicker when changing between accounts in the From combobox - when choosing a different account, the draft manager is closed, hiding the button, then a new draft manager opened for the new account it, re-showing the close button if the account has drafts enabled.

Did you mean to call set_senitive(false) here instead of hide()? I wonder if that automatically happens since you're disabling the button's action on the line above anyway.
Comment 63 Gautier Pelloux-Prayer 2016-11-21 13:44:00 UTC
Created attachment 340421 [details] [review]
patch proposal v4

> Did you mean to call set_senitive(false) here instead of hide()? I wonder if that automatically happens since you're disabling the button's action on the line above anyway.
Oh, yes my bad it is indeed not required. Removed, thanks!
Comment 64 Michael Gratton 2016-11-21 14:06:47 UTC
Review of attachment 340421 [details] [review]:

All good, pushed to master as commit c623a4e. Thanks for massaging that through, Gautier.
Comment 65 Michael Gratton 2016-11-21 14:14:32 UTC
Okay, we should now have the drafts manager's state being correctly managed, and the UI correctly updated to reflect both that that and whether the currently selected account has drafts enabled.

Can anyone CC'ed here using master pull this last change, test it out for a while and let me know if they encounter any more situations where a draft wasn't saved when it should be? Either by using the Close and Save button, or the Keep button on the confirmation dialog?

I'll look into getting this ported back to 0.11 for LTS users after it's been tested for a while.
Comment 66 Federico Bruni 2016-11-21 16:02:01 UTC
It seems to work fine on one of my accounts, while the other one cannot save drafts anymore (save draft button is always greyed out). The problematic account is the same "account 2" described in [bug #727686] (see https://bugzilla.gnome.org/show_bug.cgi?id=727686#c17)
Comment 67 Federico Bruni 2016-11-21 16:44:02 UTC
I should have said that I can save the draft of a reply in both accounts; I cannot save the draft of a new message in account 2.

Here's the error message if I run geary with -d option from a console:

 [deb] 17:38:09 0,008007 composer-widget.vala:736: Unable to open draft manager Other:fede@domain.it DraftManager: Other:fede@domain.it DraftManager: No drafts folder found

So geary cannot find the drafts folder when I start a new email with this account.
This does not happen with the other account.
The drafts folder is called "bozze" (italian term) in both accounts and "bozze" is defined as the special folder for drafts in Roundcube in both accounts.

Let me know if you need more information.
Comment 68 Michael Gratton 2016-11-21 23:07:16 UTC
(In reply to Federico Bruni from comment #66)
> It seems to work fine on one of my accounts, while the other one cannot save
> drafts anymore (save draft button is always greyed out). The problematic
> account is the same "account 2" described in [bug #727686] (see
> https://bugzilla.gnome.org/show_bug.cgi?id=727686#c17)

I know it doesn't help, but this is a good thing - it means the fixes for this bug are working properly. :)

(In reply to Federico Bruni from comment #67)
> I should have said that I can save the draft of a reply in both accounts; I
> cannot save the draft of a new message in account 2.
> 
> Here's the error message if I run geary with -d option from a console:
> 
>  [deb] 17:38:09 0,008007 composer-widget.vala:736: Unable to open draft
> manager Other:fede@domain.it DraftManager: Other:fede@domain.it
> DraftManager: No drafts folder found

Okay, that's the problem here - let's take it up over in Bug 727686.
Comment 69 Michael Gratton 2016-11-21 23:15:51 UTC
Actually Federico, Bug 727686 doesn't really fit the "No drafts folder found " problem either. Can you please either take it up on the mailing list or open a new bug so we can look into it?
Comment 70 Federico Bruni 2016-11-30 11:20:45 UTC
Webfaction moved my problematic account to a new server where UIDPLUS is supported and now I can save a new message as draft.
So bug #775263 should take care of this problem (but I won't be able to test it, as now all my accounts run on servers with UIDPLUS enabled).

Strange.. I cannot get any debugging output from Geary now..
Anyway, I guess that "No drafts folder found" was just a sporadic error.
Comment 71 Federico Bruni 2016-11-30 11:41:22 UTC
I could not see the logs because I was running geary in background.

Now I can see that the error about the drafts folder is still present. In fact if I start a new email as soon as I've launched Geary the save button is disabled. But if I wait for a while, the drafts fodler is found and I can save.

So the only issue left is folder synchronization, which is maybe a bit slow.
Comment 72 Michael Gratton 2016-12-15 00:55:10 UTC
I haven't seen any problems saving drafts on accounts where it enabled and servers where it is supported, so I'm going to close this.

Relevant patches have been ported to the LTS geary-0.11 branch and pushed as commit 2e66b64.

Thanks everyone who helped out getting this fixed, if you encounter any further specific issues saving drafts, probably best to file a new bug.
Comment 73 Michael Gratton 2016-12-20 14:19:24 UTC
*** Bug 749943 has been marked as a duplicate of this bug. ***