671554 - Switch suite/mailnews to use Services.prompt

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Ian Neal]

Attachment: suite mailnews switch to Services.prompt

We import Services.jsm but we need to make better use of prompt provided by it.
This patch:
* Switches suite code to use prompt provided by Services.jsm
* Done some minor code simplification.

Comment 1

[[e-mail]]

Comment on attachment 545911 [details] [diff] [review]
suite mailnews switch to Services.prompt

>+          if (Services.prompt.prompt(
>                     window,
>                     sComposeMsgsBundle.getString("sendMsgTitle"),
>                     sComposeMsgsBundle.getString("subjectDlogMessage"),
>                     result,
>                     null,
>                     {value:0}))
>             {
>               msgCompFields.subject = result.value;
>               var subjectInputElem = document.getElementById("msgSubject");
>               subjectInputElem.value = result.value;
>             }
>             else
>               return;
>-          }
>         }
Nit: some reindentation required.

>   if (gSendOrSaveOperationInProgress)
>   {
>-    var result;
>-
>-    if (gPromptService)
>-    {
>       var brandShortName = sBrandBundle.getString("brandShortName");
Nit: more reindentation.

>-      {
>         case 0: //Save
>           // we can close immediately if we already autosaved the draft
>           if (!gContentChanged && !gMsgCompose.bodyModified)
>             break;
>           gCloseWindowAfterSave = true;
>           GenericSendMessage(nsIMsgCompDeliverMode.AutoSaveAsDraft);
>           return false;
>         case 1: //Cancel
>           return false;
>         case 2: //Don't Save
>           // only delete the draft if we didn't start off editing a draft
>           if (!gEditingDraft && gAutoSaveKickedIn)
>             RemoveDraft();            
>           break;
>-      }
Ditto.

>+  var buttonPressed = Services.prompt.confirmEx(
>+    window, 
Nit: trailing space. Also, 4 spaces for line continuation indent please.

>+    case 1:
>+    default:
>+      return false;
>   }
>-  return false;
Nit: unnecessary change.

Haven't looked at the prompt messages offline bits yet.

Comment 2

[[e-mail]]

Comment on attachment 545911 [details] [diff] [review]
suite mailnews switch to Services.prompt

>+  aString += "MessagesOffline";
>+  var checkValue = {value:false};
>+  return Services.prompt.confirmEx(
>+    window, 
>+    gOfflinePromptsBundle.getString(aString + 'WindowTitle'), 
Nit: trailing space. Also, no point building the string up in bits, might as well use aPrefix + "MessagesOfflineWindowTitle" etc. Also, 4 spaces of indent.

Comment 3

[Ian Neal]

Attachment: suite mailnews switch to Services.prompt with better indentation

Changes since last version:
* As per review.

Comment 4

[Ian Neal]

Attachment: suite mailnews switch to Services.prompt with better indentation v1.2

Changes since last version:
* Spotted another return false that could be left as is.

Comment 5

[[e-mail]]

Comment on attachment 551456 [details] [diff] [review]
suite mailnews switch to Services.prompt with better indentation v1.2

>+    if (Services.prompt.confirmEx(window, promptTitle, promptMsg,
>+      (Services.prompt.BUTTON_TITLE_IS_STRING * Services.prompt.BUTTON_POS_0) +
>+      (Services.prompt.BUTTON_TITLE_IS_STRING * Services.prompt.BUTTON_POS_1),
>+      waitButtonLabel, quitButtonLabel, null, null, {value:0}) == 1)
Nit: continuation indentation should be at least 4 spaces. (This is just one example; there are others.) r=me with that fixed. The rest of the comments may be dealt with in followup bugs if you feel they are worthy.

> function InitPrompts()
I wonder why we define this again in mailWindowOverlay.js, since mailWindowOverlay.xul already includes mail-offline.js - in fact, messenger.xul then includes it again...

>+
>+    case 1:
>+    default:
Personally I would have removed the blank cases too.

>+          null, null, {value: 0}) == 0;
We're very inconsistent with what we pass in here; strictly speaking {value: false} is most correct. (We only need to use a variable if we're actually interested in the result.)

>+          if (!Services.prompt.confirmEx(window, null, promptText, 
>+                                         Services.prompt.STD_YES_NO_BUTTONS, 
>+                                         null, null, null, null, {}))
confirmEx returns an integer, so we should compare it to one, otherwise it's unclear that this is actually checking for the OK button.

Comment 6

[Ian Neal]

(In reply to [e-mail] from comment #5)
> Comment on attachment 551456 [details] [diff] [review]
> suite mailnews switch to Services.prompt with better indentation v1.2
> > function InitPrompts()
> I wonder why we define this again in mailWindowOverlay.js, since
> mailWindowOverlay.xul already includes mail-offline.js - in fact,
> messenger.xul then includes it again...
Spun off into bug 678945

Comment 7

[Ian Neal]

Attachment: suite mailnews switch to Services.prompt with better indentation v1.2a [Checked in: Comment 7]

http://hg.mozilla.org/comm-central/rev/2f4de0b727c8