Closed Bug 168231 Opened 17 years ago Closed 10 years ago

Cancel message confirmation should have different buttons

Categories

(MailNews Core :: Networking: NNTP, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: BenB, Assigned: ewong)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 6 obsolete files)

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.
Compare bug 168159
re: wording changes - I agree with Ben.
Product: MailNews → Core
xref bug 130789; however, bug 329640 was just checked in (to the suite) to
change an OK/Cancel to a Yes/No.
Severity: normal → enhancement
Component: Networking: News → MailNews: Main Mail Window
Product: Core → Mozilla Application Suite
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Assignee: nobody → mail
QA Contact: stephend
Assignee: mail → nobody
QA Contact: message-display
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.)
Component: MailNews: Message Display → Networking: NNTP
Product: SeaMonkey → MailNews Core
QA Contact: message-display → networking.nntp
Whiteboard: [good first bug]
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.
Assignee: nobody → edmund
Status: NEW → ASSIGNED
Attached patch Updated patch for review. (obsolete) — Splinter 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.
Attachment #423917 - Attachment is obsolete: true
+    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.
Attached patch Cleaned up the previous patch. (obsolete) — Splinter Review
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.
Attachment #423926 - Attachment is obsolete: true
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.
Attachment #423940 - Flags: review?(neil)
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.
(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.
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.
Attachment #423940 - Attachment is obsolete: true
Attachment #423940 - Flags: review?(neil)
Attachment #424178 - Flags: review?(neil)
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.
Attachment #424178 - Flags: review?(neil) → review+
(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.
(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.
Changed the patch in accordance to NeilAway's review.
Attachment #424178 - Attachment is obsolete: true
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.
Attachment #424458 - Attachment is obsolete: true
- It seems as if the previous patch had ConfirmEx with a wrong
  set of parameters.
- Fixed some spacings.
Attachment #424953 - Attachment is obsolete: true
Attachment #428352 - Flags: review?(ben.bucksch)
Attachment #428352 - Flags: review?(dmose)
Attachment #428352 - Flags: review?(ben.bucksch)
Attachment #428352 - Flags: review+
Looks good now. r=BenB
Thanks for your patch and willingness to work with us!
I hope to see more patches from you ;-).
Attachment #428352 - Flags: review?(dmose) → review?(neil)
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.)
Attachment #428352 - Flags: review?(neil)
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).
Attachment #428352 - Flags: review+ → review-
Attachment #424178 - Attachment is obsolete: false
Attachment #428352 - Attachment is obsolete: true
Attachment #424178 - Flags: ui-review?(clarkbw)
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 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.
Attachment #424178 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 424178 [details] [diff] [review]
Cleaned up the previous patch.

sr...
Attachment #424178 - Flags: superreview?(bienvenu)
Attachment #424178 - Flags: superreview?(bienvenu) → superreview+
Keywords: checkin-needed
Commited <http://hg.mozilla.org/comm-central/rev/25591e6c9e1d>

FIXED

Thanks Edmund!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3.2a1
You need to log in before you can comment on or make changes to this bug.