Last Comment Bug 516453 - Implement Minimal APIs needed for Lightning.
: Implement Minimal APIs needed for Lightning.
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Philip Chee
:
:
Mentors:
Depends on:
Blocks: 509324 313822 516398 516405 516882 517238
  Show dependency treegraph
 
Reported: 2009-09-14 10:07 PDT by Philip Chee
Modified: 2009-12-16 10:31 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 WIP (8.49 KB, patch)
2009-09-14 10:11 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v1.1 Nits Fixed. (9.08 KB, patch)
2009-09-15 08:58 PDT, Philip Chee
neil: superreview+
Details | Diff | Splinter Review
Patch v1.2 folderdisplay/messagedisplay API only sr=neil (6.16 KB, patch)
2009-09-16 07:40 PDT, Philip Chee
philip.chee: superreview+
Details | Diff | Splinter Review
Patch v1.3 additional fix [Checkin: Comment 19] (6.19 KB, patch)
2009-09-18 01:22 PDT, Philip Chee
iann_bugzilla: review+
neil: superreview+
iann_bugzilla: approval‑seamonkey2.0+
Details | Diff | Splinter Review

Description Philip Chee 2009-09-14 10:07:40 PDT
This creates stub implementations for gFolderDisplay and gMessageDisplay with only the methods necessary for Lightning. Also included in this patch is UI for the lightning preferences a tweak to hide some useless lightning UI. This patch is proof of concept only for initial review. I'll move the preferences UI to Bug 516398 later.
Comment 1 Philip Chee 2009-09-14 10:11:57 PDT
Created attachment 400533 [details] [diff] [review]
Patch v1.0 WIP

* Actually attach the patch!
Comment 2 Robert Kaiser 2009-09-14 10:16:25 PDT
Comment on attachment 400533 [details] [diff] [review]
Patch v1.0 WIP

gFolderDisplay/MessageDisplay should go into suite/ code for sure (though it should have a somewhat more useful file name, possibly even the one that will contain a final gFolderDisplay once it's correctly ported), but I guess
the Lightning preferences stuff should actually end up in calendar/ and be
installed with Lightning itself.
Also, the CSS thing also should probably go into Lightning somewhere.

Of course, this is a proof-of-concept mostly, I understand that. Thanks for working on it, that's very helpful!
Comment 3 Philip Chee 2009-09-14 10:38:17 PDT
> but I guess
> the Lightning preferences stuff should actually end up in calendar/ and be
> installed with Lightning itself.

I was thinking of keeping the pref-bird.xul in suite/mailnews/prefs/ and a suitable override for those Thunderbird extensions that insist on overlaying the Thunderbird Preferences window. But I guess it's better to let the Calendar developers take responsibility for this.
Comment 4 Ian Neal 2009-09-14 13:45:41 PDT
Is it worth doing the ones for Enigmail too? See https://bugzilla.mozilla.org/show_bug.cgi?id=509324#c7 or the newsgroup posting by the Enigmail author Patrick in m.d.a.seamonkey
Comment 5 Karsten Düsterloh 2009-09-14 13:54:28 PDT
(In reply to comment #4)
> Is it worth doing the ones for Enigmail too?

Definitely.
Comment 6 Philip Chee 2009-09-14 18:12:50 PDT
Err, but I don't have Enigmail installed :P

For the record, Enigmail needs:

> gFolderDisplay.selectedCount
> gFolderDisplay.selectedMessages
> gFolderDisplay.selectedMessage
> gFolderDisplay.selectedMessageUris
> gFolderDisplay.messageDisplay.visible
> gFolderDisplay.selectedMessageIsNews
> gFolderDisplay.selectedMessageIsImap
Comment 7 Tobias Fischer 2009-09-15 02:06:24 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Is it worth doing the ones for Enigmail too?
> 
> Definitely.

Seconded. Maybe I can try to test your patch or another one with Enigmal later.
Comment 8 neil@parkwaycc.co.uk 2009-09-15 02:22:06 PDT
Comment on attachment 400533 [details] [diff] [review]
Patch v1.0 WIP

>+% override chrome://messenger/content/preferences/preferences.xul              chrome://communicator/content/pref/pref-bird.xul
Any reason not to create suite/messenger/preferences.xul directly?

>+    content/messenger/kungfu-lightning.js
folderDisplay.js

>+  get selectedMessage _getSelectedMessage()
get selectedMessage() will do fine.

>+    if (!gDBView.QueryInterface(Components.interfaces.nsITreeView).selection.count)
>+  	  return null;
Nit: too much indentation

>+    return (viewIndex != nsMsgViewIndex_None) ?
Nit: don't need the ()s
[I have a slight preference for viewIndex == nsMsgViewIndex_None ? null :]
Comment 9 neil@parkwaycc.co.uk 2009-09-15 06:09:47 PDT
Comment on attachment 400533 [details] [diff] [review]
Patch v1.0 WIP

>+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/"?>
>+<?xml-stylesheet type="text/css" href="chrome://communicator/content/communicator.css"?>
>+<?xml-stylesheet type="text/css" href="chrome://communicator/content/pref/prefpanels.css"?>
>+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/prefpanels.css"?>
>+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/preferences.css"?>
We don't need these; chrome://global/skin/ suffices.

>+<!ENTITY % dtdPlatform SYSTEM "chrome://communicator-platform/locale/pref/platformPrefOverlay.dtd"> %dtdPlatform;
We don't need this either.

>+            title="&prefWindow.title;"
Not exactly the world's most enlightning, I mean enlightening, title...

>+  <stringbundle id="bundle_prefutilities"
>+                src="chrome://communicator/locale/pref/prefutilities.properties"/>
Don't need this either.
Comment 10 Philip Chee 2009-09-15 08:58:27 PDT
Created attachment 400781 [details] [diff] [review]
Patch v1.1 Nits Fixed.

> >+% override chrome://messenger/content/preferences/preferences.xul              chrome://communicator/content/pref/pref-bird.xul
> Any reason not to create suite/messenger/preferences.xul directly?
No reason. I was trying random things and hadn't bothered to clean up. Fixed.

> >+    content/messenger/kungfu-lightning.js
> folderDisplay.js
Fixed.

> >+  get selectedMessage _getSelectedMessage()
> get selectedMessage() will do fine.
Fixed.

> >+    if (!gDBView.QueryInterface(Components.interfaces.nsITreeView).selection.count)
> >+  	  return null;
> Nit: too much indentation
Fixed. Actually several tabs crept in due to too much cut and paste.

> >+    return (viewIndex != nsMsgViewIndex_None) ?
> Nit: don't need the ()s
> [I have a slight preference for viewIndex == nsMsgViewIndex_None ? null :]
Fixed.

> >+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/"?>
> >+<?xml-stylesheet type="text/css" href="chrome://communicator/content/communicator.css"?>
> >+<?xml-stylesheet type="text/css" href="chrome://communicator/content/pref/prefpanels.css"?>
> >+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/prefpanels.css"?>
> >+<?xml-stylesheet type="text/css" href="chrome://communicator/skin/preferences.css"?>
> We don't need these; chrome://global/skin/ suffices.
Fixed.

> >+<!ENTITY % dtdPlatform SYSTEM "chrome://communicator-platform/locale/pref/platformPrefOverlay.dtd"> %dtdPlatform;
> We don't need this either.
Fixed.

> >+            title="&prefWindow.title;"
> Not exactly the world's most enlightning, I mean enlightening, title...
Not sure what to do here. Thunderbird says either "Options" or "&brandShortName Preferences" depending on platform.

> >+  <stringbundle id="bundle_prefutilities"
> >+                src="chrome://communicator/locale/pref/prefutilities.properties"/>
> Don't need this either.
Fixed.
Comment 11 neil@parkwaycc.co.uk 2009-09-15 09:25:18 PDT
Comment on attachment 400781 [details] [diff] [review]
Patch v1.1 Nits Fixed.

>+    return viewIndex == nsMsgViewIndex_None ? null :
>+                        gDBView.getMsgHdrAt(viewIndex);
Nit: Wrong to line up gDBView with nsMsgViewIndex_None.
Either line gDBView up with null or use 4-space indent.

>+    content/messenger/preferences/preferences.xul                              (preferences.xul)
Nit: (leafname) is the default, i.e. if the file lives in the jar.mn folder.

>+<!DOCTYPE prefwindow [
>+<!ENTITY % dtdPrefs    SYSTEM "chrome://communicator/locale/pref/preferences.dtd"> %dtdPrefs;
>+]>
Odd spacing here, but I forgot there's a simpler form for a single DTD anyway:
><!DOCTYPE prefwindow SYSTEM "chrome://communicator/locale/pref/preferences.dtd">

>+<prefwindow id="MailPreferences"
>+            type="prefwindow"
>+            windowtype="Mail:Preferences"
>+            title="&prefWindow.title;"
>+            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+</prefwindow>
<prefwindow .../> when you have no children!
Comment 12 Robert Kaiser 2009-09-15 10:57:08 PDT
Comment on attachment 400781 [details] [diff] [review]
Patch v1.1 Nits Fixed.

>diff --git a/suite/mailnews/messenger.css b/suite/mailnews/messenger.css

>diff --git a/suite/mailnews/preferences.xul b/suite/mailnews/preferences.xul

I still don't like those two bits living in SeaMonkey instead of Lightning, as they're now both fast hacks on our side for something that should be done cleaner and in Lightning itself in the future (contrary to the folderDisplay.js stuff that indeed belongs on our side and will be fixed right here). Could you at least add some XXX comments that make it clear that it's temporary hacks?
Comment 13 Karsten Düsterloh 2009-09-15 15:11:57 PDT
Comment on attachment 400781 [details] [diff] [review]
Patch v1.1 Nits Fixed.

>+var gMessageDisplay =
>+{
>+  get displayedMessage()
>+  {
>+    var viewIndex = gDBView.currentlyDisplayedMessage;
>+    return viewIndex == nsMsgViewIndex_None ? null :
>+                        gDBView.getMsgHdrAt(viewIndex);
>+  }

How do I provoke an error for this missing?
Comment 14 Philip Chee 2009-09-15 19:25:48 PDT
>>diff --git a/suite/mailnews/messenger.css b/suite/mailnews/messenger.css

>>diff --git a/suite/mailnews/preferences.xul b/suite/mailnews/preferences.xul

> I still don't like those two bits living in SeaMonkey instead of Lightning, as
> they're now both fast hacks on our side for something that should be done
> cleaner and in Lightning itself in the future (contrary to the folderDisplay.js
> stuff that indeed belongs on our side and will be fixed right here). Could you
> at least add some XXX comments that make it clear that it's temporary hacks?

I'll add xXx comments in the next patch. I'll also file bugs in Calendar/Lightning for these issues. Although this might be useful for ported Thunderbird extensions that insist on overlaying the Thunderbird Preferences Window.

>>+  get displayedMessage()

> How do I provoke an error for this missing?

You make sure that a message isn't displayed?
Comment 15 Philip Chee 2009-09-15 20:48:09 PDT
>>diff --git a/suite/mailnews/messenger.css b/suite/mailnews/messenger.css
Filed Bug 516882.

>>diff --git a/suite/mailnews/preferences.xul b/suite/mailnews/preferences.xul
Filed Bug 516883 .
Comment 16 Philip Chee 2009-09-16 07:40:25 PDT
Created attachment 401016 [details] [diff] [review]
Patch v1.2 folderdisplay/messagedisplay API only sr=neil

Carrying forward sr=neil

> Nit: Wrong to line up gDBView with nsMsgViewIndex_None.
> Either line gDBView up with null or use 4-space indent.
Fixed.

>>+  get displayedMessage()
> How do I provoke an error for this missing?

Ah, in the 3pane view, you select a message that has an invitation. With the message pane open the header pane should have a lightning toolbar telling you about the invitation; and no js errors of course.
Comment 17 Philip Chee 2009-09-18 01:22:58 PDT
Created attachment 401393 [details] [diff] [review]
Patch v1.3 additional fix [Checkin: Comment 19]

I did some additional testing (independent of lightning) and came across situations where gMessageDisplay.displayedMessage would throw. So I added a null check for gDBView in gMessageDisplay.displayedMessage.
Comment 18 Philip Chee 2009-09-18 01:32:34 PDT
Comment on attachment 401393 [details] [diff] [review]
Patch v1.3 additional fix [Checkin: Comment 19]

Perhaps I'll have better luck changing r? to IanN.
Comment 19 Ian Neal 2009-09-19 08:05:00 PDT
Comment on attachment 401393 [details] [diff] [review]
Patch v1.3 additional fix [Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/5e1f01b111d9

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