Closed Bug 459021 Opened 16 years ago Closed 12 years ago

Invite Attendees dialog: automatically expand address book mailing lists

Categories

(Calendar :: Dialogs, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: WSourdeau)

References

Details

Attachments

(2 files, 4 obsolete files)

Would be nice if the attendee pane could auto expand lists defined in the address book.
Component: General → Dialogs
OS: Mac OS X → All
QA Contact: general → dialogs
Hardware: x86 → All
Summary: Attendee pane support for address lists → Invite Attendees dialog: automatically expand address book mailing lists
Attached file Patch that applies to 1.0b2. (obsolete) —
This patch should pretty much apply as-is on trunk.
reddragon will port it for sure.
Assignee: nobody → mohit.kanwal
Flags: wanted-calendar1.0+
Patch ported to the current comm-central repo. 

The way to test this feature out is as follows:
*create a "list" in your tb contacts
*add a few list members
*invite the list, by typing its name
*if it decomposes to your members, you're all set
Attachment #564082 - Attachment is obsolete: true
Attachment #564084 - Flags: review?(philipp)
Comment on attachment 564084 [details] [diff] [review]
patch that applies to the current comm-central repo.

Review of attachment 564084 [details] [diff] [review]:
-----------------------------------------------------------------

r- to get a new patch with the following nits fixed:

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +946,2 @@
>          var attendees = document.getElementById("attendees-list");
> +        if (event.newValue == "true") {

So list values not new? Maybe you could explain this part to me :-)

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
@@ +629,5 @@
> +                  while (!mailingListURI
> +                         && allAddressBooks.hasMoreElements()) {
> +                      let abDir = allAddressBooks.getNext()  
> +                                   .QueryInterface(Components.interfaces.nsIAbDirectory);
> +		      if (abDir instanceof Components.interfaces.nsIAbDirectory) {

calling QueryInterface will throw an exception if its not of the correct interface, so instead of checking with if() and instanceof you should rather try/catch the QueryInterface and then check for null with if().

@@ +634,5 @@
> +		          let abCardsEnumerator = abDir.childCards;
> +			  while (!mailingListURI &&
> +			     abCardsEnumerator.hasMoreElements()) {
> +			     
> +			     let card = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);

can you guarantee that this will always return an nsIAbCard? If so, please wrap the line a bit better, if not, please wrap with try/catch.

@@ +636,5 @@
> +			     abCardsEnumerator.hasMoreElements()) {
> +			     
> +			     let card = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);
> +			  
> +			     if (card && card.isMailList && (card.displayName == names.value[0])) {

Is this the way that Thunderbird also detects mailing list names? I was thinking maybe its possible to name a Thunderbird list like an email, that would break this logic.

@@ +661,5 @@
> +              var continueSearch = true;
> +              var abCardsEnumerator = mailingList.childCards;
> +              try {
> +                  while (abCardsEnumerator.hasMoreElements()) {
> +                      var abCard = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);

The try/catch block around this is better, but it means that if one card is not an ab card, then the whole loop will break out. Also, it might unintentionally catch errors in the following if() block that are unrelated to the QueryInterface call.

@@ +696,5 @@
> +              if (this.mLDAPSession && this.mSessionAdded) {
> +                  input.syncSessions(document.getElementById('attendeeCol2#1'));
> +              }
> +
> +              dump("assigning value: " + entry + "\n");

Please remove the debug statements

@@ +713,5 @@
> +              icon.removeAttribute("disabled");
> +              icon.setAttribute("id", "attendeeCol1#" + rowNumber);
> +              icon.setAttribute("class", "role-icon");
> +              icon.setAttribute("role", newAttendee.role);
> +              // this.arrowHit(listitem, rowNumber - 1);

Is this still needed? If not, please remove.

@@ +721,5 @@
> +      <method name="_adjustRowIds">
> +          <parameter name="parentNode"/>
> +          <parameter name="rowNumber"/>
> +          <body><![CDATA[
> +              var row = parentNode.childNodes[rowNumber];

I'd appreciate if you could use let instead of var :)

@@ +745,5 @@
> +                          var template = document.getAnonymousElementByAttribute(this, "anonid", "item");
> +                          var currentNode = template.parentNode.childNodes[currentIndex];
> +                          var nextNode = template.parentNode.childNodes[currentIndex+1];
> +                          this._fillListItemWithEntry(currentNode, entries[0], currentIndex);
> +                          for (var i = 1; i < entries.length; i++) {

Unless this is a DOM list, you can use for each()
Attachment #564084 - Flags: review?(philipp) → review-
Attached patch Version 3 (obsolete) — Splinter Review
Maybe Ludovic can comment about the first question, he did the patch on Inverse' trunk.

I have attached the patch with the style changes suggested by Philipp. Also one cannot use for each here:
>                          this._fillListItemWithEntry(currentNode, entries[0], currentIndex);
>                          for (var i = 1; i < entries.length; i++) {
>                              currentNode = template.cloneNode(true);
>                              currentIndex++;
>                              this._fillListItemWithEntry(currentNode, entries[i], currentIndex);
>                              template.parentNode.insertBefore(currentNode, nextNode);
>                          }

as the first entry is used to replace the mailing list entry row.

cheers
mohit
Attachment #564084 - Attachment is obsolete: true
Attachment #567266 - Flags: review?(philipp)
Ludovic, any updates here?
(In reply to Philipp Kewisch [:Fallen] from comment #3)
> Comment on attachment 564084 [details] [diff] [review] [diff] [details] [review]

Hi Philipp, I will reply instead since I am the original author of this code...

> 
> ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
> @@ +946,2 @@
> >          var attendees = document.getElementById("attendees-list");
> > +        if (event.newValue == "true") {
> 
> So list values not new? Maybe you could explain this part to me :-)

I don't remember why I put that there, but after a few tests, it indeed seems useless and can be rearranged by leeting the code in the else clause execute unconditionally.
> calling QueryInterface will throw an exception if its not of the correct
> interface, so instead of checking with if() and instanceof you should rather
> try/catch the QueryInterface and then check for null with if().

The test is useless anyway as nothing else than an abDirectory can be returned for that enumerator.

> 
> @@ +634,5 @@
> > +		          let abCardsEnumerator = abDir.childCards;
> > +			  while (!mailingListURI &&
> > +			     abCardsEnumerator.hasMoreElements()) {
> > +			     
> > +			     let card = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);
> 
> can you guarantee that this will always return an nsIAbCard? If so, please
> wrap the line a bit better, if not, please wrap with try/catch.

All the children of an abDirectory are nsiAbCard, even lists. Don't ask.

> @@ +636,5 @@
> > +			     abCardsEnumerator.hasMoreElements()) {
> > +			     
> > +			     let card = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);
> > +			  
> > +			     if (card && card.isMailList && (card.displayName == names.value[0])) {
> 
> Is this the way that Thunderbird also detects mailing list names? I was
> thinking maybe its possible to name a Thunderbird list like an email, that
> would break this logic.

So we would add an "or" clause that tests card.displayName against emailAddresses.value[0], right ?

> @@ +661,5 @@
> > +              var continueSearch = true;
> > +              var abCardsEnumerator = mailingList.childCards;
> > +              try {
> > +                  while (abCardsEnumerator.hasMoreElements()) {
> > +                      var abCard = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);
> 
> The try/catch block around this is better, but it means that if one card is
> not an ab card, then the whole loop will break out. Also, it might
> unintentionally catch errors in the following if() block that are unrelated
> to the QueryInterface call.

The nsIAbCard interface is the parent interface of all children of any addressbook. So this cannot fail, at least until this changes.

> @@ +713,5 @@
> > +              icon.removeAttribute("disabled");
> > +              icon.setAttribute("id", "attendeeCol1#" + rowNumber);
> > +              icon.setAttribute("class", "role-icon");
> > +              icon.setAttribute("role", newAttendee.role);
> > +              // this.arrowHit(listitem, rowNumber - 1);
> 
> Is this still needed? If not, please remove.

Of course it's needed, those are clones of the template entry, so they need to be reenabled and assigned a proper attendee icon.

> 
> 
> I'd appreciate if you could use let instead of var :)

(Note that this code was originally written for Thunderbird 2.)


> @@ +745,5 @@
> > +                          var template = document.getAnonymousElementByAttribute(this, "anonid", "item");
> > +                          var currentNode = template.parentNode.childNodes[currentIndex];
> > +                          var nextNode = template.parentNode.childNodes[currentIndex+1];
> > +                          this._fillListItemWithEntry(currentNode, entries[0], currentIndex);
> > +                          for (var i = 1; i < entries.length; i++) {
> 
> Unless this is a DOM list, you can use for each()

Agreed.
(In reply to Wolfgang Sourdeau from comment #7)

Hey Wolfgang, thank you for your comments!

> > @@ +636,5 @@
> > > +			     abCardsEnumerator.hasMoreElements()) {
> > > +			     
> > > +			     let card = abCardsEnumerator.getNext().QueryInterface(Components.interfaces.nsIAbCard);
> > > +			  
> > > +			     if (card && card.isMailList && (card.displayName == names.value[0])) {
> > 
> > Is this the way that Thunderbird also detects mailing list names? I was
> > thinking maybe its possible to name a Thunderbird list like an email, that
> > would break this logic.
> 
> So we would add an "or" clause that tests card.displayName against
> emailAddresses.value[0], right ?
I don't know how mailing list names are parsed by the headerparser, you'd have to test what value emailAddresses.value[0] has. I could imagine that either names.value[0] or emailAdresses.value[0] is empty when naming a mailinglist test@example.com

If mailing lists as an email work fine with the code as it is, then you don't have to change anything.

> > The try/catch block around this is better, but it means that if one card is
> > not an ab card, then the whole loop will break out. Also, it might
> > unintentionally catch errors in the following if() block that are unrelated
> > to the QueryInterface call.
> 
> The nsIAbCard interface is the parent interface of all children of any
> addressbook. So this cannot fail, at least until this changes.
Ok, then ideally you could change to only catch NS_NOINTERFACE, i.e

} catch (e if e.result == Components.results.NS_NOINTERFACE) {}

(untested)


> Of course it's needed, those are clones of the template entry, so they need
> to be reenabled and assigned a proper attendee icon.
Ah right! Maybe you can remove the commented out line though.

I'd also appreciate a new patch with the nits fixed so we can check it in.
Status: NEW → ASSIGNED
Attached patch Final version?Splinter Review
Assignee: mohit.kanwal → WSourdeau
Attachment #567266 - Attachment is obsolete: true
Attachment #568757 - Flags: review?
Attachment #567266 - Flags: review?(philipp)
(In reply to Philipp Kewisch [:Fallen] from comment #8)
> (In reply to Wolfgang Sourdeau from comment #7)
> 


> I don't know how mailing list names are parsed by the headerparser, you'd
> have to test what value emailAddresses.value[0] has. I could imagine that
> either names.value[0] or emailAdresses.value[0] is empty when naming a
> mailinglist test@example.com

Ok, I tested it and nothing was needed no that side, therefore I left that code as is. However, there was a check in resolvePotentialList for the absence of the "@" sign in the record name, which I had to remove.

> 
> I'd also appreciate a new patch with the nits fixed so we can check it in.

Done. I also fixed the indentation of a few lines as well as an issue with the computed element ids, which was only visible after a second list was inserted. So, in theory we should be good.

Regarding the exceptions, it seems that the code was already handling them in the "Version 3" uploaded by Mohit, so I left things as they were, except for an invocation to cal.WARN. Anyway, I don't see how catching that exception can be useful ecept for telling the developer "the API has changed, please rewrite this code", especially since I don't think this will change in the near future.
Attachment #568757 - Flags: review? → review?(philipp)
Comment on attachment 568757 [details] [diff] [review]
Final version?

r=philipp, thanks for your work!
Attachment #568757 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/595eb542fdd9>
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/64e08f18ee1b>
Target Milestone: Trunk → 1.0b9
Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/1980e02f69fb>
Target Milestone: 1.0b9 → 1.0b8
I've tried with tb8/ln1.0.
When i create an event and i add a mailing list (from address book) in the  attendees list, only the first user is inserted.

in the file calendar-event-dialog-attendees.xml
if I change (line 743):

this._fillListItemWithEntry(currentNode, entry, currentIndex);
template.parentNode.insertBefore(currentNode, nextNode);

by :

template.parentNode.insertBefore(currentNode, nextNode);
this._fillListItemWithEntry(currentNode, entry, currentIndex);

all attendees are correctly inserted.
We should see that a patch for comment 15 is created. Wolfgang, care to put it together?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #581261 - Flags: review?(philipp)
This patch add support for nested lists (lists that contain other lists).

For exemple :
list 'L1' contains 'User1', 'User2'. 
list 'L2' contains 'L1'.
 adding 'L2' add 'User1', 'User2'. 

The implementation prevents loops. 
For exemple :
List L3 contains L4 and L4 contains L3
Attachment #581268 - Flags: review?(philipp)
Comment on attachment 581268 [details] [diff] [review]
a patch that add support for nested lists

Philippe, could you please file a new bug report for this patch to separate the issues? Thanks for the patch! :)
Comment on attachment 581261 [details] [diff] [review]
patch for comment 15

Looks good to me, r=philipp
Attachment #581261 - Flags: review?(philipp) → review+
Comment on attachment 581268 [details] [diff] [review]
a patch that add support for nested lists

(In reply to Martin Schröder [:mschroeder] from comment #19)
> Philippe, could you please file a new bug report for this patch to separate
> the issues? Thanks for the patch! :)

Filed bug 711994
Attachment #581268 - Attachment is obsolete: true
Attachment #581268 - Flags: review?(philipp)
Pushed to comm-central changeset de2f178475b9
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: 1.0 → 1.3
You need to log in before you can comment on or make changes to this bug.