Last Comment Bug 517238 - Implement Minimal APIs needed for Enigmail.
: Implement Minimal APIs needed for Enigmail.
Status: RESOLVED FIXED
[patch in comment 15]
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.0
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 516453
Blocks: 509324 517996 786189 786200
  Show dependency treegraph
 
Reported: 2009-09-17 09:50 PDT by Philip Chee
Modified: 2012-08-28 03:06 PDT (History)
14 users (show)
iann_bugzilla: blocking‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch eV1.0 APIs for Enigmail (MQ patch) (3.06 KB, patch)
2009-09-18 01:31 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 Fix Nits. (2.82 KB, patch)
2009-09-19 03:27 PDT, Philip Chee
iann_bugzilla: review+
Details | Diff | Splinter Review
Patch v1.2 Use numSelected (via this.selectedCount) r=iann (2.85 KB, patch)
2009-09-20 10:18 PDT, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Splinter Review
[for checkin] Patch 1.3 r=iann_bugzilla sr=neil (2.83 KB, patch)
2009-09-21 01:50 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
kairo: approval‑seamonkey2.0+
Details | Diff | Splinter Review

Description Philip Chee 2009-09-17 09:50:44 PDT
Followup to Bug 516453. Extend gFolderDisplay and gMessageDisplay to support Enigmail.
Comment 1 Philip Chee 2009-09-18 01:31:42 PDT
Created attachment 401395 [details] [diff] [review]
Patch eV1.0 APIs for Enigmail (MQ patch)

Note: this patch applies on top of attachment 401393 [details] [diff] [review] from Bug 516453.

What Enigmail apparently needs:

> gFolderDisplay.selectedCount
> gFolderDisplay.selectedMessages
> gFolderDisplay.selectedMessage
> gFolderDisplay.selectedMessageUris
> gFolderDisplay.messageDisplay.visible
> gFolderDisplay.selectedMessageIsNews
> gFolderDisplay.selectedMessageIsImap

I also did:

gFolderDisplay.selectedMessageIsFeed
gFolderDisplay.selectedMessageIsExternal
gFolderDisplay.selectedIndices

Not tested with Enigmail but I exercised all these methods on the 3pane tab and they all returned what looked like reasonable data without throwing.

Tobias, do you have the necessary setup to test these patches? Otherwise I can send you two files (messenger.xul and folderDisplay.js) that you can insert into messenger.jar from the current 2.0pre nightly.
Comment 2 neil@parkwaycc.co.uk 2009-09-18 02:28:08 PDT
Comment on attachment 401395 [details] [diff] [review]
Patch eV1.0 APIs for Enigmail (MQ patch)

>+var nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
Ugh. For once, I don't like polluting the global scope!

>+  get messageDisplay()
>+  {
>+    return gMessageDisplay;
>+  },
If gMessageDisplay was declared first, you could write this as
messageDisplay: gMessageDisplay,
Or you could set it as a property at the end of the file.

>+    if (!gDBView)
>+      return 0;
>+    return gDBView.numSelected;
Nit: could use gDBView ? gDBView.numSelected : 0;

>     if (!gDBView.QueryInterface(Components.interfaces.nsITreeView).selection.count)
Hmm, should this be numSelected?

>+    return Boolean(message && message.folder &&
Bah, is some idiot actually going to compare this === to "false"?

>+    message.folder.server.type == "rss");
Nit: indentation wrong.

>+    if (!gDBView)
>+      return [];
>+    return gDBView.getIndicesForSelection({});
?: again.

>+      var enum = gDBView.getMsgHdrsForSelection().enumerate();
getMsgHdrsForSelection returns an nsIMutableArray, so use a for loop and queryElementAt. (Sadly fixIterator hasn't been fixed to work with an nsIMutableArray.)

>+    return messageArray.length ? messageArray : null;
:-)

>+  set visible()
>+  {
>+    return; // Fake setter for the time being.
Nit: setter should return the value being set.
Comment 3 Philip Chee 2009-09-19 03:27:08 PDT
Created attachment 401616 [details] [diff] [review]
Patch v1.1 Fix Nits.

Also in this patch, I changed all top level lets to vars.

> >+var nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
> Ugh. For once, I don't like polluting the global scope!

Change it to a property of gMessageDisplay.

> >+  get messageDisplay()
> >+  {
> >+    return gMessageDisplay;
> >+  },
> If gMessageDisplay was declared first, you could write this as
> messageDisplay: gMessageDisplay,
> Or you could set it as a property at the end of the file.

Since this is all rather temporary I set the property at the end of the file.

> >+    if (!gDBView)
> >+      return 0;
> >+    return gDBView.numSelected;
> Nit: could use gDBView ? gDBView.numSelected : 0;

Fixed.

> >     if (!gDBView.QueryInterface(Components.interfaces.nsITreeView).selection.count)
> Hmm, should this be numSelected?

I don't think so. numSelected is affected by |mail.operate_on_msgs_in_collapsed_threads| which I don't think is what we want here.

> >+    return Boolean(message && message.folder &&
> Bah, is some idiot actually going to compare this === to "false"?

Removed Boolean() globally.

> >+    message.folder.server.type == "rss");
> Nit: indentation wrong.

Fixed.

> >+    if (!gDBView)
> >+      return [];
> >+    return gDBView.getIndicesForSelection({});
> ?: again.

Fixed.

> >+      var enum = gDBView.getMsgHdrsForSelection().enumerate();
> getMsgHdrsForSelection returns an nsIMutableArray, so use a for loop and
> queryElementAt. (Sadly fixIterator hasn't been fixed to work with an
> nsIMutableArray.)

Fixed.

> >+  set visible()
> >+  {
> >+    return; // Fake setter for the time being.
> Nit: setter should return the value being set.

Fixed. Note Thunderbird returns null, when it bothers to return anything at all. :-?
Comment 4 Ian Neal 2009-09-19 07:54:47 PDT
(In reply to comment #2)
> (From update of attachment 401395 [details] [diff] [review])
> >+      var enum = gDBView.getMsgHdrsForSelection().enumerate();
> getMsgHdrsForSelection returns an nsIMutableArray, so use a for loop and
> queryElementAt. (Sadly fixIterator hasn't been fixed to work with an
> nsIMutableArray.)
Hmmm, TB has:
// getMsgHdrsForSelection returns an nsIMutableArray.  We want our callers
//  to have a user-friendly JS array and not have to worry about
//  QueryInterfacing the values (or needing to know to use fixIterator).
return [msgHdr for each
          (msgHdr in fixIterator(
                      this.view.dbView.getMsgHdrsForSelection().enumerate(),
                      Components.interfaces.nsIMsgDBHdr))];
Comment 5 Philip Chee 2009-09-19 07:59:03 PDT
The result is the same, we return a bog standard js array of nsIMsgDBHdrs.
Comment 6 Philip Chee 2009-09-19 08:03:38 PDT
gFolderDisplay: Properties for object:
[object Object]
nsMsgFolderFlags: nsMsgFolderFlags
selectedCount: 4
selectedMessage: [xpconnect wrapped nsIMsgDBHdr]
selectedMessageIsFeed: false
selectedMessageIsImap: false
selectedMessageIsNews: true
selectedMessageIsExternal: false
selectedIndices: 8,9,10,11
selectedMessages: [xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr]
selectedMessageUris: news-message://news.mozilla.org/mozilla.test#3762,news-message://news.mozilla.org/mozilla.test#3755,news-message://news.mozilla.org/mozilla.test#3761,news-message://news.mozilla.org/mozilla.test#3760
messageDisplay: [object Object]

gFolderDisplay.selectedMessages: Properties for object:
[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr]
0: [xpconnect wrapped nsIMsgDBHdr]
1: [xpconnect wrapped nsIMsgDBHdr]
2: [xpconnect wrapped nsIMsgDBHdr]
3: [xpconnect wrapped nsIMsgDBHdr]
Comment 7 Philip Chee 2009-09-19 08:06:12 PDT
On Shredder.

gFolderDisplay.selectedMessages: Properties for object:
[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr],[xpconnect wrapped nsIMsgDBHdr]
0: [xpconnect wrapped nsIMsgDBHdr]
1: [xpconnect wrapped nsIMsgDBHdr]
2: [xpconnect wrapped nsIMsgDBHdr]
3: [xpconnect wrapped nsIMsgDBHdr]
4: [xpconnect wrapped nsIMsgDBHdr]
Comment 8 Ian Neal 2009-09-19 10:08:26 PDT
Comment on attachment 401616 [details] [diff] [review]
Patch v1.1 Fix Nits.

r=me, having discussed queries on IRC.
Comment 9 neil@parkwaycc.co.uk 2009-09-19 10:11:16 PDT
(In reply to comment #3)
> > >     if (!gDBView.QueryInterface(Components.interfaces.nsITreeView).selection.count)
> > Hmm, should this be numSelected?
> I don't think so. numSelected is affected by
> |mail.operate_on_msgs_in_collapsed_threads| which I don't think is what we want
> here.
I don't suppose it matters much since you're only worried about whether or not
a selection exists.
Comment 10 Philip Chee 2009-09-20 10:18:16 PDT
Created attachment 401719 [details] [diff] [review]
Patch v1.2 Use numSelected (via this.selectedCount) r=iann

Carrying forward r=iann

>>> Hmm, should this be numSelected?
>> I don't think so. numSelected is affected by
>> |mail.operate_on_msgs_in_collapsed_threads| which I don't think is what we want
>> here.
> I don't suppose it matters much since you're only worried about whether or not
> a selection exists.
OK. Fixed to use numSelected (via this.selectedCount)
Comment 11 neil@parkwaycc.co.uk 2009-09-20 11:55:23 PDT
Comment on attachment 401719 [details] [diff] [review]
Patch v1.2 Use numSelected (via this.selectedCount) r=iann

>+           (message.folder.flags & this.nsMsgFolderFlags.ImapBox) > 0;
Nit: != (both times)

>+        let hdr = array.queryElementAt(i, Components.interfaces.nsIMsgDBHdr);
>+        msgHdrs.push(hdr);
Nit: not worth a separate variable
Comment 12 Philip Chee 2009-09-20 12:48:54 PDT
>>+        let hdr = array.queryElementAt(i, Components.interfaces.nsIMsgDBHdr);
>>+        msgHdrs.push(hdr);
> Nit: not worth a separate variable
How best to wrap the resulting line @80 then?
Comment 13 Ian Neal 2009-09-20 13:05:18 PDT
(In reply to comment #12)
> >>+        let hdr = array.queryElementAt(i, Components.interfaces.nsIMsgDBHdr);
> >>+        msgHdrs.push(hdr);
> > Nit: not worth a separate variable
> How best to wrap the resulting line @80 then?

+        msgHdrs.push(array.queryElementAt(i,
                                           Components.interfaces.nsIMsgDBHdr));
The above fits, but looks strange, sometimes things end up being more than 80.
Comment 14 neil@parkwaycc.co.uk 2009-09-20 13:33:36 PDT
We can live with the odd 81-char line now and then!
Comment 15 Philip Chee 2009-09-21 01:50:15 PDT
Created attachment 401800 [details] [diff] [review]
[for checkin] Patch 1.3 r=iann_bugzilla sr=neil

Carrying forward r=iann_bugzilla, sr=neil

> (From update of attachment 401719 [details] [diff] [review])
> >+           (message.folder.flags & this.nsMsgFolderFlags.ImapBox) > 0;
> Nit: != (both times)
Fixed. Fixed.

> >+        let hdr = array.queryElementAt(i, Components.interfaces.nsIMsgDBHdr);
> >+        msgHdrs.push(hdr);
> Nit: not worth a separate variable
Fixed.
Comment 16 Frank Wein [:mcsmurf] 2009-09-22 12:23:37 PDT
http://hg.mozilla.org/comm-central/rev/3dbbd9a979b4

Note You need to log in before you can comment on or make changes to this bug.