Closed
Bug 459021
Opened 16 years ago
Closed 12 years ago
Invite Attendees dialog: automatically expand address book mailing lists
Categories
(Calendar :: Dialogs, enhancement)
Calendar
Dialogs
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: dbo, Assigned: WSourdeau)
References
Details
Attachments
(2 files, 4 obsolete files)
9.19 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
Would be nice if the attendee pane could auto expand lists defined in the address book.
Updated•15 years ago
|
Component: General → Dialogs
OS: Mac OS X → All
QA Contact: general → dialogs
Hardware: x86 → All
Updated•14 years ago
|
Summary: Attendee pane support for address lists → Invite Attendees dialog: automatically expand address book mailing lists
Comment 1•13 years ago
|
||
This patch should pretty much apply as-is on trunk. reddragon will port it for sure.
Updated•13 years ago
|
Assignee: nobody → mohit.kanwal
Updated•13 years ago
|
Flags: wanted-calendar1.0+
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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-
Comment 4•13 years ago
|
||
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)
Comment 6•13 years ago
|
||
Ludovic, any updates here?
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Comment 8•13 years ago
|
||
(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.
Updated•13 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•13 years ago
|
||
Assignee: mohit.kanwal → WSourdeau
Attachment #567266 -
Attachment is obsolete: true
Attachment #568757 -
Flags: review?
Attachment #567266 -
Flags: review?(philipp)
Assignee | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #568757 -
Flags: review? → review?(philipp)
Comment 11•13 years ago
|
||
Comment on attachment 568757 [details] [diff] [review] Final version? r=philipp, thanks for your work!
Attachment #568757 -
Flags: review?(philipp) → review+
Comment 12•13 years ago
|
||
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/595eb542fdd9>
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Comment 13•13 years ago
|
||
Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/64e08f18ee1b>
Target Milestone: Trunk → 1.0b9
Comment 14•13 years ago
|
||
Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/1980e02f69fb>
Target Milestone: 1.0b9 → 1.0b8
Comment 15•13 years ago
|
||
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.
Comment 16•12 years ago
|
||
We should see that a patch for comment 15 is created. Wolfgang, care to put it together?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•12 years ago
|
||
Attachment #581261 -
Flags: review?(philipp)
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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 20•12 years ago
|
||
Comment on attachment 581261 [details] [diff] [review] patch for comment 15 Looks good to me, r=philipp
Attachment #581261 -
Flags: review?(philipp) → review+
Comment 21•12 years ago
|
||
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)
Comment 22•12 years ago
|
||
Pushed to comm-central changeset de2f178475b9
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: 1.0 → 1.3
Comment 23•12 years ago
|
||
Backported to releases/comm-aurora changeset 374402aa2520
Target Milestone: 1.3 → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•