168231 - Cancel message confirmation should have different buttons

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, enhancement)

Description

[Ben Bucksch (:BenB)]

In the confirmation dialog for Cancelling a news message, you get the buttons:
OK (default) and Cancel
Given that the feature is called "Cancel", this is confusing. Does "Cancel"
perform the action and "OK" then the opposite "OK, I read the warning, I'd
rather not cancel?"

It is a question that's being asked. So, call the buttons "Yes" and "No", with
No being the default.

Comment 1

[Ben Bucksch (:BenB)]

Compare bug 168159

Comment 2

[robinf]

re: wording changes - I agree with Ben.

Comment 3

[Mike Cowperthwaite]

xref bug 130789; however, bug 329640 was just checked in (to the suite) to
change an OK/Cancel to a Yes/No.

Comment 4

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 5

[Phil Ringnalda (:philor)]

Search for cancelConfirm (http://mxr.mozilla.org/comm-central/search?string=cancelConfirm) to get to the spot in the code where we open the dialog, https://developer.mozilla.org/en/NsIPromptService#confirmEx (and searching for actual uses of confirmEx) will show how to use it with STD_YES_NO_BUTTONS and how to select a default, [good first bug]. (And it's entirely confined to nsNNTPProtocol.cpp, so it's mailnews core.)

Comment 6

[Edmund Wong (:ewong)]

Attachment: Cancel message dialog changed to the standard Yes/No dialog.

The cancel news message was a little confusing as it was asking for an OK/Cancel 
response to a "Are you sure you want to cancel this message?" which really 
expects a Yes or No answer.  Changing the dialog to a Yes/No dialog provides a 
clearer UI for the user to answer.

Comment 7

[Edmund Wong (:ewong)]

Attachment: Updated patch for review.

This patch overrides the previous patch.  It does the same thing with the
following changes:
- Removed debug items
- Removed Todo
+ Changed flags to buttonFlags.
+ Added comments.

Comment 8

[Ben Bucksch (:BenB)]

+    unsigned long buttonFlags = nsIPrompt::STD_YES_NO_BUTTONS;

unnecessary and just makes it harder to read.
Just use nsIPrompt::STD_YES_NO_BUTTONS directly in the function call parameter.

+    if (NS_FAILED(rv))  
+    	  confirmCancelResult = 1;  // If the call to ConfirmEx fails, 
+    	                           //  set default answer to No (1). 
should be
+    if (NS_FAILED(rv))  
+      confirmCancelResult = 1;  // Default to No

I.e. comment shorter, like 2 lines below.
Also mind the intention - 2 spaces here.

Comment 9

[Edmund Wong (:ewong)]

Attachment: Cleaned up the previous patch.

Cleaned up the previous patch by changing the comments and
removing the unnecessary variable buttonFlags by putting 
NSIPrompt::STD_YES_NO_BUTTONS directly into the function
call.

Comment 10

[Ben Bucksch (:BenB)]

Comment on attachment 423940 [details] [diff] [review]
Cleaned up the previous patch.

You forgot to remove
+    unsigned long flags = nsIPrompt::STD_YES_NO_BUTTONS;

(Tip: It's useful practice to always look at the patch yourself again immediately before you attach it.)

With that removed, r=BenB . I assume you have tested it (I didn't).

Requesting formal review from NeilAway.

Comment 11

[Joshua Cranmer [:jcranmer]]

Comment on attachment 423940 [details] [diff] [review]
Cleaned up the previous patch.

>-  else
>-    confirmCancelResult = 1;
>-
>-  if (confirmCancelResult != 1) {
>+  else
>+    confirmCancelResult = 1; // Default to No.
>+
>+  if (confirmCancelResult != 0) {

You have just inverted the meaning of the preference. If the meaning on confirmCancelRequest is being inverted, that means that the else branch needs to invert it as well.

Comment 12

[Edmund Wong (:ewong)]

(In reply to comment #10)
> (From update of attachment 423940 [details] [diff] [review])
> You forgot to remove
> +    unsigned long flags = nsIPrompt::STD_YES_NO_BUTTONS;
> 
> (Tip: It's useful practice to always look at the patch yourself again
> immediately before you attach it.)
> 
> With that removed, r=BenB . I assume you have tested it (I didn't).
> 


Sorry about that.   I guess in my nervousness in submitting  a patch,
I overlooked that.  I'll get that fixed.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Cleaned up the previous patch.

Changed the default to Yes for the case if NS_FAILED(rv) was false and
requireConfirmationForCancel was false.  

Removed the unnecessary variable flags definition line.

Comment 14

[[e-mail]]

Comment on attachment 424178 [details] [diff] [review]
Cleaned up the previous patch.

>-    rv = dialog->Confirm(nsnull, confirmText.get(), &confirmCancelResult);
[Eww, I can't believe they got away with a using a PRInt32 for this.]

>+    	confirmCancelResult = 1; // Default to No.
You've got a tab in here, which is wrong, it should be all spaces.

>+    
No spaces on blank lines please.

Comment 15

[Edmund Wong (:ewong)]

(In reply to comment #14)
> (From update of attachment 424178 [details] [diff] [review])
> >-    rv = dialog->Confirm(nsnull, confirmText.get(), &confirmCancelResult);
> [Eww, I can't believe they got away with a using a PRInt32 for this.]

I'm not familiar with this part.  Is it supposed to be something other
than PRInt32?  It took me some time to figure out the right ConfirmEx 
to use.  

> 
> >+    	confirmCancelResult = 1; // Default to No.
> You've got a tab in here, which is wrong, it should be all spaces.
> 
> >+    
> No spaces on blank lines please.

Both fixed.  Will update the patch.

Comment 16

[[e-mail]]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 424178 [details] [diff] [review])
> > >-    rv = dialog->Confirm(nsnull, confirmText.get(), &confirmCancelResult);
> > [Eww, I can't believe they got away with a using a PRInt32 for this.]
> I'm not familiar with this part.  Is it supposed to be something other
> than PRInt32?  It took me some time to figure out the right ConfirmEx 
> to use.  
PRInt32 is correct now! It was supposed to be PRBool before.

Comment 17

[Edmund Wong (:ewong)]

Attachment: Fixed some errors in the previous patch.

Changed the patch in accordance to NeilAway's review.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Redid the patch with some added comments.

Had to re-do the patch due to:

1) the missing comments
2) the fact that it should be dialog->ConfirmEx(...) 
   instead of dialog->Confirm(...).  The parameters were
   correct, but wrong method.

Comment 19

[Edmund Wong (:ewong)]

Attachment: This patch supercedes the previous patch.

- It seems as if the previous patch had ConfirmEx with a wrong
  set of parameters.
- Fixed some spacings.

Comment 20

[Ben Bucksch (:BenB)]

Looks good now. r=BenB
Thanks for your patch and willingness to work with us!
I hope to see more patches from you ;-).

Comment 21

[[e-mail]]

Comment on attachment 428352 [details] [diff] [review]
This patch supercedes the previous patch.  

This patch is unfortunately not an improvement on attachment 424178 [details] [diff] [review] (which already has r+ but could have been improved with some whitespace cleanup.)

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 428352 [details] [diff] [review]
This patch supercedes the previous patch.  

true. The only difference are two more spaces (which shouldn't be there).

Comment 23

[Ben Bucksch (:BenB)]

clarkbw, this patch does what the initial description asks for.
Instead of 'Do you want to cancel this post?' "OK"/"Cancel", it asks "Yes"/"No".

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 424178 [details] [diff] [review]
Cleaned up the previous patch.

I would usually opt for "action verbs" but it seems like it would mean using the word "Cancel Message"/Cancel which isn't right.  If someone has other suggestions that would be great, I had spent time trying to research newsgroup message cancel and didn't find anything else.

So the Yes/No looks pretty good to ui-r+ because there are no new strings and it's an improvement over the previous.

Sorry for the delay.

Comment 25

[Ben Bucksch (:BenB)]

Comment on attachment 424178 [details] [diff] [review]
Cleaned up the previous patch.

sr...

Comment 26

[Ben Bucksch (:BenB)]

Commited <http://hg.mozilla.org/comm-central/rev/25591e6c9e1d>

FIXED

Thanks Edmund!