Switch suite/mailnews to use Services.prompt

RESOLVED FIXED in seamonkey2.5

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Tracking

Trunk
seamonkey2.5
Dependency tree / graph
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

(seamonkey2.5 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 545911 [details] [diff] [review]
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.
Attachment #545911 - Flags: review?(neil)

Comment 1

7 years ago
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

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 551452 [details] [diff] [review]
suite mailnews switch to Services.prompt with better indentation

Changes since last version:
* As per review.
Attachment #545911 - Attachment is obsolete: true
Attachment #551452 - Flags: review?(neil)
Attachment #545911 - Flags: review?(neil)
(Assignee)

Comment 4

7 years ago
Created attachment 551456 [details] [diff] [review]
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.
Attachment #551452 - Attachment is obsolete: true
Attachment #551456 - Flags: review?(neil)
Attachment #551452 - Flags: review?(neil)

Comment 5

7 years ago
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.
Attachment #551456 - Flags: review?(neil) → review+
(Assignee)

Comment 6

7 years ago
(In reply to neil@parkwaycc.co.uk 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
(Assignee)

Comment 7

7 years ago
Created attachment 553136 [details] [diff] [review]
suite mailnews switch to Services.prompt with better indentation v1.2a [Checked in: Comment 7]

http://hg.mozilla.org/comm-central/rev/2f4de0b727c8
Attachment #551456 - Attachment is obsolete: true
Attachment #553136 - Flags: review+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-seamonkey2.5: --- → fixed
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.5

Updated

7 years ago
Blocks: 713671
You need to log in before you can comment on or make changes to this bug.