Closed
Bug 516453
Opened 15 years ago
Closed 15 years ago
Implement Minimal APIs needed for Lightning.
Categories
(SeaMonkey :: MailNews: General, defect)
SeaMonkey
MailNews: General
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)
6.19 KB,
patch
|
iannbugzilla
:
review+
neil
:
superreview+
iannbugzilla
:
approval-seamonkey2.0+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
* Actually attach the patch!
Attachment #400533 -
Flags: ui-review?(kairo)
Attachment #400533 -
Flags: review?(mnyromyr)
Assignee | ||
Updated•15 years ago
|
Attachment #400533 -
Flags: review?(neil)
Comment 2•15 years ago
|
||
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!
Assignee | ||
Comment 3•15 years ago
|
||
> 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
Comment 5•15 years ago
|
||
(In reply to comment #4)
> Is it worth doing the ones for Enigmail too?
Definitely.
Assignee | ||
Comment 6•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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 :]
Updated•15 years ago
|
Attachment #400533 -
Flags: review?(neil) → review-
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
> >+% 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)
Updated•15 years ago
|
Attachment #400781 -
Flags: superreview?(neil) → superreview+
Comment 11•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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?
Assignee | ||
Comment 14•15 years ago
|
||
>>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?
Assignee | ||
Comment 15•15 years ago
|
||
>>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 .
Assignee | ||
Comment 16•15 years ago
|
||
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)
Assignee | ||
Comment 17•15 years ago
|
||
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)
Assignee | ||
Comment 18•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #401393 -
Flags: superreview?(neil) → superreview+
Updated•15 years ago
|
Flags: blocking-seamonkey2.0?
Attachment #401393 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #401393 -
Flags: approval-seamonkey2.0?
Attachment #401393 -
Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•15 years ago
|
Attachment #401393 -
Attachment description: Patch v1.3 additional fix. → [for checkin] Patch v1.3 additional fix.
Comment 19•15 years ago
|
||
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: 15 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•