Closed Bug 516453 Opened 11 years ago Closed 11 years ago

Implement Minimal APIs needed for Lightning.

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch v1.0 WIP (obsolete) — Splinter Review
* Actually attach the patch!
Attachment #400533 - Flags: ui-review?(kairo)
Attachment #400533 - Flags: review?(mnyromyr)
Attachment #400533 - Flags: review?(neil)
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!
> 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.
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
(In reply to comment #4)
> Is it worth doing the ones for Enigmail too?

Definitely.
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
(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 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 :]
Attachment #400533 - Flags: review?(neil) → review-
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.
Attached patch Patch v1.1 Nits Fixed. (obsolete) — Splinter Review
> >+% 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.
Attachment #400533 - Attachment is obsolete: true
Attachment #400781 - Flags: superreview?(neil)
Attachment #400781 - Flags: review?(mnyromyr)
Attachment #400533 - Flags: ui-review?(kairo)
Attachment #400533 - Flags: review?(mnyromyr)
Attachment #400781 - Flags: superreview?(neil) → superreview+
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 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 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?
>>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?
>>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 .
Blocks: 516883
Blocks: 516882
No longer blocks: 516883
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.
Attachment #400781 - Attachment is obsolete: true
Attachment #401016 - Flags: superreview+
Attachment #401016 - Flags: review?(mnyromyr)
Attachment #400781 - Flags: review?(mnyromyr)
Blocks: 517238
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.
Attachment #401016 - Attachment is obsolete: true
Attachment #401393 - Flags: superreview?(neil)
Attachment #401393 - Flags: review?(mnyromyr)
Attachment #401016 - Flags: review?(mnyromyr)
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.
Attachment #401393 - Flags: review?(mnyromyr) → review?(iann_bugzilla)
Attachment #401393 - Flags: superreview?(neil) → superreview+
Flags: blocking-seamonkey2.0?
Attachment #401393 - Flags: review?(iann_bugzilla) → review+
Attachment #401393 - Flags: approval-seamonkey2.0?
Attachment #401393 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Flags: blocking-seamonkey2.0?
Keywords: checkin-needed
Attachment #401393 - Attachment description: Patch v1.3 additional fix. → [for checkin] Patch v1.3 additional fix.
Comment on attachment 401393 [details] [diff] [review]
Patch v1.3 additional fix [Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/5e1f01b111d9
Attachment #401393 - Attachment description: [for checkin] Patch v1.3 additional fix. → Patch v1.3 additional fix [Checkin: Comment 19]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 313822
Blocks: 509324
You need to log in before you can comment on or make changes to this bug.