Add support for closing inactive databases (folders) [SeaMonkey part for Thunderbird bug 723248]

RESOLVED FIXED

Status

SeaMonkey
MailNews: General
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Philip Chee, Assigned: ewong)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
From Bug 767210 comment 5:
> part of the fix for bug 723248 is TB-specific, though it would be easy for
> SM to port the code.

1. Move this file
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l3.2>
From /mail/base/modules/ to /mailnews/base/util/
1a. Adjust makefiles to suit.
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l2.1>

2. Port this bit from Thunderbirds msgMail3PaneWindow.js
to SeaMonkeys msgMail3PaneWindow.js
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l1.1>
(Assignee)

Comment 1

5 years ago
Created attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #640503 - Flags: review?(iann_bugzilla)

Comment 2

5 years ago
Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

Looks good to me, you will need a TB review due to the moving of the file and may be worth an sr from Neil to make sure he is happy we're importing and init'ing from the correct place.
Attachment #640503 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

5 years ago
Attachment #640503 - Flags: review?(bwinton)
Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

Redirecting the review to Standard8, since I'm am totally not a Makefile guy, and even the 3Pane change doesn't look like it would impact UI…
Attachment #640503 - Flags: review?(bwinton) → review?(mbanner)
Attachment #640503 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Attachment #640503 - Flags: superreview?(neil)

Comment 4

5 years ago
Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

It looks to me that if you open the mail window a second time (which is of course much easier to do in SeaMonkey because you can easily close the mail window and open it again without quitting) then the init will throw an exception because the module is already initialised. (But I haven't actually checked.)

One approach might be to move the init to somewhere that really only happens once, such as nsSuiteGlue.js (final-ui-startup, I guess).

Another approach might be to add an initialised check to the module so that you can't accidentally initialise it twice.
(Reporter)

Comment 5

5 years ago
> Another approach might be to add an initialised check to the module so that you can't
> accidentally initialise it twice.

I think bienvenu had the right idea but forgot to complete it:
http://hg.mozilla.org/comm-central/annotate/8e441f4abb45/mail/base/modules/msgDBCacheManager.js#l51

>   init: function dbcachemgr_init()
>   {

<<Insert check for this._initialized here>>

>     // we listen for "quit-application-granted" instead of
>     // "quit-application-requested" because other observers of the
>     // latter can cancel the shutdown.
>     var observerSvc = Cc["@mozilla.org/observer-service;1"]
>                       .getService(Ci.nsIObserverService);
>     observerSvc.addObserver(this, "quit-application-granted", false);
> 
>     this.startPeriodicCheck();
> 
>     this._initialized = true;
>   },
(Assignee)

Comment 6

5 years ago
Created attachment 644154 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v2)
Attachment #640503 - Attachment is obsolete: true
Attachment #640503 - Flags: superreview?(neil)
Attachment #644154 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 7

5 years ago
> +    if (!this._initialized) {
won't it be easier and less messy to just do:

if (this._initialized)
  return;

Comment 8

5 years ago
Comment on attachment 644154 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v2)

>+++ b/mailnews/base/util/msgDBCacheManager.js
>@@ -38,17 +38,18 @@ var msgDBCacheManager =
>    */
>   init: function dbcachemgr_init()
>   {
>+    if (!this._initialized) {
As Philip said would be easier to do:
if (this._initialized)
  return;

> 
>-    // we listen for "quit-application-granted" instead of
>-    // "quit-application-requested" because other observers of the
>-    // latter can cancel the shutdown.
>-    var observerSvc = Cc["@mozilla.org/observer-service;1"]
>-                      .getService(Ci.nsIObserverService);
>-    observerSvc.addObserver(this, "quit-application-granted", false);
As we already have Services.jsm imported, is it worth changing this to:
Services.obs.addObserver(this, "quit-application-granted", false);

r=me with those fixed/addressed.
Attachment #644154 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Comment 9

5 years ago
> As we already have Services.jsm imported...

> Cu.import("resource:///modules/IOUtils.js");
> Cu.import("resource:///modules/iteratorUtils.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
I suspect that we need neither IOUtils.js nor XPCOMUtils.js. But perhaps this should go into a followup bug.
(Assignee)

Comment 10

5 years ago
Created attachment 645296 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v3)
Attachment #644154 - Attachment is obsolete: true
Attachment #645296 - Flags: superreview?(neil)
Attachment #645296 - Flags: review+
Comment on attachment 645296 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v3)

>+    Services.obs.addObserver(this, "quit-application-granted", false);
[On an unrelated note, this never gets properly removed...]
Attachment #645296 - Flags: superreview?(neil) → superreview+
(Reporter)

Updated

5 years ago
Blocks: 777787
(Assignee)

Comment 12

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a3b46de2b086
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You might also need to do a seamonkey port of some of bug 1135310 which fixes flaws of bug 723248
Flags: needinfo?(philip.chee)
Summary: Add support for closing inactive databases (folders) [SeaMonkey part] → Add support for closing inactive databases (folders) [SeaMonkey part for Thunderbird bug 723248]

Comment 14

2 years ago
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #13)
> You might also need to do a seamonkey port of some of bug 1135310 which
> fixes flaws of bug 723248

I think as this bug moved msgDBCacheManager.js from mail/ to mailnews/ we have automatically picked up the fixes from bug 1135310.
(Reporter)

Comment 15

2 years ago
(In reply to Ian Neal from comment #14)

> I think as this bug moved msgDBCacheManager.js from mail/ to mailnews/ we
> have automatically picked up the fixes from bug 1135310.
Agreed.
Flags: needinfo?(philip.chee)
You need to log in before you can comment on or make changes to this bug.