Closed Bug 682580 Opened 11 years ago Closed 11 years ago

Remove suite's dependency on nsTryToClose.js

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(seamonkey2.7 fixed)

RESOLVED FIXED
seamonkey2.7
Tracking Status
seamonkey2.7 --- fixed

People

(Reporter: standard8, Unassigned)

References

()

Details

Attachments

(2 files, 2 obsolete files)

Firefox is removing nsTryToClose.js from Firefox because it registers as app-startup and hence slows down starting up.

We should just switch these to observer registrations in appropriate places.
Note: I've said we'll do this before the next merge point, so they can land their patch before then. It should be simple enough to do before then.
Attached patch Editor fix (obsolete) — Splinter Review
* Turns EditorCanClose into an nsIObserver function (fortunately we don't have to use objects these days) that can cancel the quit request
* Adds it in EditorStartup rather than EditorOnLoad so that the plaintext editor can prompt on quit request as well (and also so that we can remove it!)
* Removes it in EditorShutdown to avoid leaks
Attachment #568157 - Flags: review?(iann_bugzilla)
Attachment #568157 - Flags: review?(iann_bugzilla) → review+
* Added check to see whether quit has already been cancelled.
Attachment #568157 - Attachment is obsolete: true
Attachment #568196 - Flags: review?(iann_bugzilla)
Attachment #568196 - Flags: review?(iann_bugzilla) → review+
Attached patch Rest of Suite fix (obsolete) — Splinter Review
Following a similar tack to editor patch
Attachment #568245 - Flags: review?(neil)
Comment on attachment 568245 [details] [diff] [review]
Rest of Suite fix

>-    return false;
>+    canClose = false;
>   }
> 
>   // Returns FALSE only if user cancels save action
>-  if (gContentChanged || gMsgCompose.bodyModified || (gAutoSaveKickedIn && !gEditingDraft))
>+  if (canClose && (gContentChanged || gMsgCompose.bodyModified ||
>+                   (gAutoSaveKickedIn && !gEditingDraft)))
This particular function is sufficiently complex that it might be worth leaving it and reusing the (conveniently) existing observer to handle all the quit-related code, along the lines of
case "quit-application-requested":
  if (!aSubject.data)
    aSubject.data =!ComposeCanClose();

>     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),
>         continueButtonLabel, stopButtonLabel, null, null, {value:0}))
>       gFilterListMsgWindow.StopUrls();
>-    else
>+    else {
>+      if (!reallyClose && aTopic == "quit-application-requested")
>+        aCancelQuit.data = true;
>       return false;
>+    }
This return-in-else block is really against our coding style, the original code should have said something along the lines of
if (Services.prompt.confirmEx(...) == 0)
  return false;
gFilterListMsgWindow.StopUrls();
Changes since last version:
* Use existing observer in MsgComposeCommands.js
* Switch if clause around in FilterListDialog.js
Attachment #568245 - Attachment is obsolete: true
Attachment #568245 - Flags: review?(neil)
Attachment #568559 - Flags: review?(neil)
Comment on attachment 568559 [details] [diff] [review]
Rest of Suite fix v1.1 [Checked in: Comment 8]

[Existing behaviour that I noticed: you can only "Don't Save" a message once; if you cancel the quit for some other reason, the message window doesn't prompt you next time you close or quit. Other windows prompt every time.]

>+    else if (aTopic == "quit-application-requested" &&
>+             aSubject instanceof Components.interfaces.nsISupportsPRBool &&
>+             !aSubject.data)
>+      aSubject.data = !ComposeCanClose();
[I also came up with a slightly different version, but I can't decide:
else if (aTopic == "quit-application-requested" &&
         aSubject instanceof Components.interfaces.nsISupportsPRBool &&
         !aSubject.data &&
         !ComposeCanClose())
  aSubject.data = true;
]

Two actual bugs though; r=me with these fixed:
>-  }
Compose window fails to load because of this deletion.

>+      if (!reallyClose && aTopic == "quit-application-requested")
The filter dialog doesn't use reallyClose.
Attachment #568559 - Flags: review?(neil) → review+
Comment on attachment 568559 [details] [diff] [review]
Rest of Suite fix v1.1 [Checked in: Comment 8]

http://hg.mozilla.org/comm-central/rev/8e21a323a4bb
Attachment #568559 - Attachment description: Rest of Suite fix v1.1 → Rest of Suite fix v1.1 [Checked in: Comment 8]
Blocks: 698011
(In reply to neil@parkwaycc.co.uk from comment #7)
> [Existing behaviour that I noticed: you can only "Don't Save" a message
> once; if you cancel the quit for some other reason, the message window
> doesn't prompt you next time you close or quit. Other windows prompt every
> time.]
Spun off to bug 698011
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.7
Version: unspecified → Trunk
Comment on attachment 568196 [details] [diff] [review]
Editor fix [Checked in: Comment 10 & 11]

http://hg.mozilla.org/comm-central/rev/b06edef5f722
Attachment #568196 - Attachment description: Editor fix [Checked in: Comment 10] → Editor fix [Checked in: Comment 10 & 11]
You need to log in before you can comment on or make changes to this bug.