The default bug view has changed. See this FAQ.

Implement Minimal APIs needed for Lightning.

RESOLVED FIXED

Status

SeaMonkey
MailNews: General
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug, {fixed-seamonkey2.0})

Trunk
fixed-seamonkey2.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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

8 years ago
Created attachment 400533 [details] [diff] [review]
Patch v1.0 WIP

* Actually attach the patch!
Attachment #400533 - Flags: ui-review?(kairo)
Attachment #400533 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Attachment #400533 - Flags: review?(neil)

Comment 2

8 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

8 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.

Comment 4

8 years ago
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

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

Definitely.
(Assignee)

Comment 6

8 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

8 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

8 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

8 years ago
Attachment #400533 - Flags: review?(neil) → review-

Comment 9

8 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

8 years ago
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.
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

8 years ago
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 12

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

8 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

8 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 .

Updated

8 years ago
Blocks: 516883

Updated

8 years ago
Blocks: 516882

Updated

8 years ago
No longer blocks: 516883
(Assignee)

Comment 16

8 years ago
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.
Attachment #400781 - Attachment is obsolete: true
Attachment #401016 - Flags: superreview+
Attachment #401016 - Flags: review?(mnyromyr)
Attachment #400781 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Blocks: 517238
(Assignee)

Comment 17

8 years ago
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.
Attachment #401016 - Attachment is obsolete: true
Attachment #401393 - Flags: superreview?(neil)
Attachment #401393 - Flags: review?(mnyromyr)
Attachment #401016 - Flags: review?(mnyromyr)
(Assignee)

Comment 18

8 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

8 years ago
Attachment #401393 - Flags: superreview?(neil) → superreview+

Updated

8 years ago
Flags: blocking-seamonkey2.0?

Updated

8 years ago
Attachment #401393 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

8 years ago
Attachment #401393 - Flags: approval-seamonkey2.0?

Updated

8 years ago
Attachment #401393 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+

Updated

8 years ago
Flags: blocking-seamonkey2.0?
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Attachment #401393 - Attachment description: Patch v1.3 additional fix. → [for checkin] Patch v1.3 additional fix.

Comment 19

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

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED

Updated

7 years ago
Blocks: 313822

Updated

7 years ago
Blocks: 509324
You need to log in before you can comment on or make changes to this bug.