724522 - Don't prompt twice when we can't save a draft message

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Mark Banner (:standard8)]

Attachment: Possible fix

This is an initial part of bug 670801, currently when we try to auto-save a draft message we stick up two prompts:

1) Tells the user that saving the draft failed, and gives them the opportunity to retry.
2) If the user cancels the first dialog, the get a second that tells the user the error (although iirc this is just a generic message atm), and generally points them at the settings dialog.

Of course, if these appearing very frequently, it is annoying to the user.

I've done a draft patch which initially just removes the second dialog. Hence reduces the dialogs if the user agrees to cancel. I'm not sure if we should try and move the error about what failed to the first dialog.

There's some try server builds here of the patch I'm attaching:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=48b502df9814

Most of the stuff in the patch is tidy up. The changing of the first parameter to the Fail function to NS_OK in one instance is the actual fix.

Comment 1

[Mark Banner (:standard8)]

Although I guess the other alternative would be to somehow indicate internally that this is an auto-save and don't put up a modal dialog, but maybe just an alert in the sense of the alerts service/new mail prompts style.

Comment 2

[David :Bienvenu]

(In reply to Mark Banner (:standard8) from comment #1)
> Although I guess the other alternative would be to somehow indicate
> internally that this is an auto-save and don't put up a modal dialog, but
> maybe just an alert in the sense of the alerts service/new mail prompts
> style.

That would have been my suggestion - we know when it's an autosave, I believe.

Comment 3

[David :Bienvenu]

Comment on attachment 594683 [details] [diff] [review]
Possible fix

this line should be two:

   *                         value as well; if passed, the function won't prompt the user     *                         but will still about the session.
+  

I much prefer the idea of putting up a single sliding in the case of autosave failing (or maybe even failing more silently; that's really a question for Blake).

but feedback+ because this would be an improvement.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 594683 [details] [diff] [review]
Possible fix

Yeah, I agree with Bienvenu, this is an improvement.

I think in an ideal world, I'ld like to see it work like the attachment reminder notification, where we have a little bar that lets the user know what's happened, and if they dismiss it, it never returns.  But that's a slightly larger patch.  ;)

Thanks,
Blake.

Comment 5

[Atul Jangra [:atuljangra]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> I think in an ideal world, I'ld like to see it work like the attachment
> reminder notification, where we have a little bar that lets the user know
> what's happened, and if they dismiss it, it never returns.  But that's a
> slightly larger patch.  ;)
But I think this would be a great idea. It would be nice to see this in our Thunderbird.

Regards
Atul Jangra

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Blake Winton (:bwinton) from comment #4)
> Comment on attachment 594683 [details] [diff] [review]
> Possible fix
> 
> Yeah, I agree with Bienvenu, this is an improvement.

Mark, are you sticking with this patch for landing?

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix

Ok, doing this change at least removes one annoying dialog for the users that end up pressing cancel. Most of the changes in this patch are tidy up (I did them ages ago).

The real change is the change in the call to Fail, where the error code is changed from rv to NS_OK, to avoid prompting the second time (we know the copy failed, and we've already prompted about it, so no need to prompt again).

Comment 8

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 727811 [details] [diff] [review]
The fix

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

This patch causes NotifyListenerOnStopCopy() to return NS_OK in some cases; most of the callers ignore the nsresult returned, but nsMsgComposeAndSend::SendToMagicFolder() doesn't. I'm just a little worried that we'll lose some cleanup or other handling in callers up the chain from there if we don't pass the error up.

If you're convinced the there's nothing up that call chain that would break, r+.

::: mailnews/compose/public/nsIMsgSend.idl
@@ +305,5 @@
> +     * Report a send failure.
> +     *
> +     * @param aFailureCode     The failure code of the send operation. See
> +     *                         nsComposeStrings.h for possible values. NS_OK is a possible
> +     *                         value as well; if passed, the function won't prompt the user     *                         but will still about the session.

s/about/abort/ and it looks like this line is wrapped?

::: mailnews/compose/src/nsMsgSend.cpp
@@ +4034,5 @@
> +
> +    // We failed, and the user decided not to retry. So we're just going to
> +    // fail out. However, give Fail a success code so that it doesn't prompt
> +    // the user a second time as they already know about the failure.
> +    Fail(NS_OK, nullptr, &aStatus);

This seems like a place for NS_ERROR_BUT_DONT_SHOW_ALERT; would that be better than NS_OK?

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 727811 [details] [diff] [review]
The fix

So, I just tried saving a draft (from the Save button on the toolbar), and it popped up the error twice, and then hung the compose window, and crashed Daily.  (And I just replicated it.)  So, uh, ui-r-?

(But I like the idea behind it, and this doesn't _really_ seem to change the UI, as much as fixes a bug we currently have, so I think you can assume a ui-r+ on whichever version of this patch doesn't crash…  ;)

Oh, and adding something to mail/test/mozmill/composition/test-drafts.js seems like it would be a good idea.
(I get an error trying to run the mozmill tests, or I'ld take a stab at writing it myself.)

Thanks,
Blake.

Comment 10

[Mark Banner (:standard8)]

(In reply to Blake Winton (:bwinton) from comment #9)
> So, I just tried saving a draft (from the Save button on the toolbar), and
> it popped up the error twice, and then hung the compose window, and crashed
> Daily.  (And I just replicated it.)  So, uh, ui-r-?

I've tried a few times, and I can't reproduce this, can you still reproduce it? (if so, STR and maybe crash stack please!).

> Oh, and adding something to mail/test/mozmill/composition/test-drafts.js
> seems like it would be a good idea.
> (I get an error trying to run the mozmill tests, or I'ld take a stab at
> writing it myself.)

The problem is, this is removing a dialog, and bug 670801 would remove the other dialog. It is also on auto-save. So we currently can't test auto-save works correctly, as we don't have MozMill tests that work with servers (which is what we need for this one). So this seems a bit of a non-test, or else one that would get removed when we did bug 670801.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

(As mentioned in IRC, I can't replicate this either, so ui-r=me, based on my previous comment.)

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/bd5e3381ef6a