516453 - Implement Minimal APIs needed for Lightning.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

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]

Attachment: Patch v1.0 WIP

* Actually attach the patch!

Comment 2

[Robert Kaiser]

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]

> 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]

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]

(In reply to comment #4)
> Is it worth doing the ones for Enigmail too?

Definitely.

Comment 6

[Philip Chee]

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]

(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

[[e-mail]]

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

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

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]

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]

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]

>>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]

>>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]

Attachment: 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]

Attachment: 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]

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]

Comment on attachment 401393 [details] [diff] [review]
Patch v1.3 additional fix [Checkin: Comment 19]

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