Invite attendees doesn't expand mailing lists anymore
Categories
(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: benjamin)
Details
(Keywords: regression)
Attachments
(1 file, 6 obsolete files)
4.81 KB,
patch
|
darktrojan
:
review+
jorgk-bmo
:
feedback+
|
Details | Diff | Splinter Review |
Invite Attendees doesn't expand mailing lists anymore, so it tries to send to the list representation instead, i.e. <testlist>
This broke sometime before start of this year. Forgot to file.
Originally apparently implemented in bug 459021 (for code pointers).
Comment 1•2 years ago
|
||
For which version is this reported? Is this an issue in ESR, too?
Comment 2•2 years ago
|
||
This might be bug 758493 - I need to rebase the patch there to check it.
Reporter | ||
Comment 3•2 years ago
|
||
Not an issue on ESR. I'm seeing it on nightlies, IIRC at least since sometime December.
Reporter | ||
Updated•2 years ago
|
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
•
|
||
Looking at it now
Reporter | ||
Comment 5•2 years ago
|
||
As a pointer, resolvePotentialList is likely what you should be checking - or why that doesn't get called.
https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#950
Maybe the event target is not what it used to be, or something.
anonid is (basically) used only in xbl (anonymous content id).
Assignee | ||
Comment 6•2 years ago
|
||
Awesome, I was going to widen my search to things not included in bug 459021 as nothing has changed in ...attendees.js in a while. However, I did notice the lack of xml files which are referenced in the earlier bug, though adding them back didn't do anything. Just figured I would try the easiest solution first.
Reporter | ||
Comment 7•2 years ago
|
||
In the de-xbl effort we're doing, the xbl (.xml) are usuallyl converted to custom elements (.js)..
Using the developer tools debugger to debug by setting break points can be handy. Or just adding dump statements in the javascript (outputs like printf into the console). E.g. dump("xxxbenamin hehehhey\n");
Assignee | ||
Comment 8•2 years ago
|
||
I have a working fix, will upload patch once I've removed the debug code and add a comment or two.
Assignee | ||
Comment 9•2 years ago
|
||
Let me know if it works, I only removed a few lines and tweaked one or two others.
Comment 10•2 years ago
|
||
The patch is empty. Do a |hg qref| before attaching it.
Updated•2 years ago
|
Assignee | ||
Comment 11•2 years ago
|
||
Well that's irritating, I checked the patch before exporting it. Should I be following the workflow at the bottom of. https://developer.thunderbird.net/contributing/fixing-a-bug/using-mercurial-queues ?
Assignee | ||
Comment 12•2 years ago
|
||
Okay, this one has actual data in it.
Reporter | ||
Comment 13•2 years ago
|
||
Comment on attachment 9068021 [details] [diff] [review] bug-1530213-fix.patch Review of attachment 9068021 [details] [diff] [review]: ----------------------------------------------------------------- The anonid="input" used to be https://dxr.mozilla.org/comm-esr60/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml#36 So we should instead of removing all the checks, check target.classList.contains("textbox-addressingWidget") For commit messages, please use a format like so: Bug 1530213 - make mailing lists used in the invitee dialog expand again. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +933,4 @@ > } > > let roleStatusIcon = listitem.querySelector(".status-icon"); > + if(roleStatusIcon) { what's this for? it doesn't look correct @@ +951,4 @@ > */ > resolvePotentialList(input) { > let fieldValue = input.value; > + if (fieldValue.length > 0) { while here, we usually only check truthy, like if (fieldValue) { @@ +955,5 @@ > let mailingList = this._resolveListByName(fieldValue); > if (mailingList) { > let entries = this._getListEntries(mailingList); > if (entries.length > 0) { > + let currentIndex = this.mMaxAttendees - 2; wow the old code is pretty here, carving out the number from something like attendeeCol3#1 without even a comment ... but the change also seems wrong
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Comment 14•2 years ago
|
||
- Added comments to explain the code changes.
- Changed the description to match the bug.
- Added the check suggested by Magnus Melin
Reporter | ||
Comment 15•2 years ago
|
||
Comment on attachment 9068221 [details] [diff] [review] bug-1530213-fix2.patch Review of attachment 9068221 [details] [diff] [review]: ----------------------------------------------------------------- When a patch is ready, you should flag it for review. You do that by setting the review ? flag on the attachment, and in the field that appears you select a reviewer by email address. Patches without flags risk getting no attention. For WIP patches, use the feedback flag instead. I can confirm the patch works, but it still needs some adjustment. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +933,4 @@ > } > > let roleStatusIcon = listitem.querySelector(".status-icon"); > + // An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue. but it should never be null... if it is, find out why @@ +933,5 @@ > } > > let roleStatusIcon = listitem.querySelector(".status-icon"); > + // An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue. > + if(roleStatusIcon) { for code style this would have been: if (roleStatusIcon) And you'd also indent the part inside the if @@ +951,4 @@ > */ > resolvePotentialList(input) { > let fieldValue = input.value; > + // input.id is no longer a variable, These kind of comments you can leave in the bug, but if you put them in the code that would be confusing to future readers. Only put comments about current state in the code. Also, there's some trailing spaces here @@ +956,5 @@ > let mailingList = this._resolveListByName(fieldValue); > if (mailingList) { > let entries = this._getListEntries(mailingList); > if (entries.length > 0) { > + // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace. Please no comments on what used to be, only the current state. You could note down that index 0 is the organizer, then the rest is maybe as clear as it needs to be @@ +957,5 @@ > if (mailingList) { > let entries = this._getListEntries(mailingList); > if (entries.length > 0) { > + // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace. > + let currentIndex = this.mMaxAttendees -2; nit: space around the - For changed files, run the linter, like ./mach lint comm/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +962,3 @@ > let template = this.querySelector(".addressingWidgetItem"); > let currentNode = template.parentNode.childNodes[currentIndex]; > this._fillListItemWithEntry(currentNode, entries[0], currentIndex); I notice _fillListItemWithEntry doesn't actually take a 3rd argument anymore... so fix the callers.
Assignee | ||
Comment 16•2 years ago
|
||
Gotcha, will fix and resubmit.
Assignee | ||
Comment 17•2 years ago
|
||
I've cleaned up the code, but haven't figured out why the first item on the list always returns null when setting roleStatusIcon. I'll keep working on it till I understand whats going on, even if the fix isn't forthcoming.
Assignee | ||
Comment 18•2 years ago
|
||
Code has been cleaned up per your guidance.
roleStatusIcon is still returning null on the first list item (the one that is replaced when the list is expanded). So the code to get around that issue is still in the patch.
I need some guidance on where to look next for the possible culprit. The odd part of the issue is that the first item returns null, but subsequent "cloned" items do not.
I'll keep looking into it.
Assignee | ||
Comment 19•2 years ago
|
||
Jorg informed me that we don't like tabs. I'll reformat and resubmit.
Reporter | ||
Comment 20•2 years ago
|
||
Comment on attachment 9069377 [details] [diff] [review] bug-1530213-fix.patch Review of attachment 9069377 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +933,4 @@ > } > > let roleStatusIcon = listitem.querySelector(".status-icon"); > + if (roleStatusIcon) { If you look up how status-icon is used, you'll find that is strange. If set, its class will at some places be changed to role-icon. But since we're looking up by class, that won't work after such changes. I wonder if it wouldn't make sense to always just use the role-icon. https://searchfox.org/comm-central/rev/e0c2c4a00a839e66b733dff1671cb18f1b217db3/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#719-724 It's a rare occasion that the organizer would not be participating, so having that info there seems not needed. (If that's what the data is put there for, I didn't look to verify.)
Reporter | ||
Comment 21•2 years ago
|
||
Comment on attachment 9069377 [details] [diff] [review] bug-1530213-fix.patch Review of attachment 9069377 [details] [diff] [review]: ----------------------------------------------------------------- Ok, looking at it some more, let's go with this and file a follow-up for the role-icon, status-icon wrongness. It looks like they should use the same thing as itip-icon where the role and status are overlayed on the icon. That's how it's showing in the event participants area. Also need a folloup for the list size overflowing vertically now... (but that's a different bug.)
Reporter | ||
Updated•2 years ago
|
Updated•2 years ago
|
Comment 22•2 years ago
|
||
Comment on attachment 9069377 [details] [diff] [review] bug-1530213-fix.patch Review of attachment 9069377 [details] [diff] [review]: ----------------------------------------------------------------- I'm not landing this. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +936,5 @@ > + if (roleStatusIcon) { > + roleStatusIcon.removeAttribute("disabled"); > + roleStatusIcon.setAttribute("class", "role-icon"); > + roleStatusIcon.setAttribute("role", newAttendee.role); > + } This is mis-indented since it's still using tabs. @@ +955,5 @@ > let mailingList = this._resolveListByName(fieldValue); > if (mailingList) { > let entries = this._getListEntries(mailingList); > if (entries.length > 0) { > + let currentIndex = this.mMaxAttendees - 2; // start currentIndex at the begining of the address list Can you move this comment onto the line before and really explain what is happening. If you have 10 attendees, you start at 8? @@ +970,4 @@ > currentIndex++; > } > this.mMaxAttendees += entries.length; > + for (let i = currentIndex; i <= this.mMaxAttendees - 1; i++) { Why did that change? Also, why not use `i < this.mMaxAttendees` as for regular loops.
Comment 23•2 years ago
|
||
Magnus, what's happening with that index business here? Also, don't you want a Calendar peer to look at this?
Comment 24•2 years ago
|
||
Comment on attachment 9069377 [details] [diff] [review] bug-1530213-fix.patch Geoff, can to take a look at this please.
Comment 25•2 years ago
|
||
Comment on attachment 9069377 [details] [diff] [review] bug-1530213-fix.patch Review of attachment 9069377 [details] [diff] [review]: ----------------------------------------------------------------- I'm giving this r- for the reasons Jorg gave, and a comment from me. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +933,4 @@ > } > > let roleStatusIcon = listitem.querySelector(".status-icon"); > + if (roleStatusIcon) { You could do querySelector(".role-icon, .status-icon") which would pick up either. I don't think there's any state where the image doesn't have one of those two classes, so the if check is probably unnecessary.
Reporter | ||
Comment 26•2 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #25)
You could do querySelector(".role-icon, .status-icon") which would pick up
either. I don't think there's any state where the image doesn't have one of
those two classes, so the if check is probably unnecessary.
That's a bit of nonsense though, since in the if clause you set the role-icon class...
But like I wrote earlier, there's a bunch of wrongness going on with that.
Assignee | ||
Comment 27•2 years ago
|
||
Sorry for taking so long to get back to this.
I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.
I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.
for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {
I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.
Assignee | ||
Comment 28•2 years ago
|
||
Sorry for taking so long to get back to this.
I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.
I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.
for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {
I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.
Comment 29•2 years ago
|
||
Comment on attachment 9075795 [details] [diff] [review] 1530213.patch Review of attachment 9075795 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +953,5 @@ > let mailingList = this._resolveListByName(fieldValue); > if (mailingList) { > let entries = this._getListEntries(mailingList); > if (entries.length > 0) { > + // We set the current index based on the contact list items location in the attendees list Nit: Full stop missing at end of line. @@ +956,5 @@ > if (entries.length > 0) { > + // We set the current index based on the contact list items location in the attendees list > + // The index isn't given to us through input so we use mMaxAttendees instead > + // subtracting one to count from 0. > + // To replace the contact list entry with the first value in the list we subtract another. You mean "mailing list"? And "member of the mailing list"? @@ +971,5 @@ > this._fillListItemWithEntry(currentNode, entry, currentIndex); > currentIndex++; > } > this.mMaxAttendees += entries.length; > + for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) { I'm totally confused now: Original code: i <= this.mMaxAttendees; Previous patch: currentIndex; i <= this.mMaxAttendees - 1 This patch: currentIndex; i < this.mMaxAttendees - 1
Assignee | ||
Comment 30•2 years ago
•
|
||
Sorry yes mailing list is what I meant. I'll update the comments with proper punctuation and the word changes.
...
This patch:
currentIndex; i < this.mMaxAttendees - 1
It seems that having the - 1 there has no affect on the code even with the < instead of <= so I will remove it.
My initial thought was that by subtracting one from mMaxAttendees it would prevent the loop from running one too many times. Of course that's what removing the "=" would do as well.
I'll do some proofreading and up load an edited patch.
Assignee | ||
Comment 31•2 years ago
|
||
Reworded the comment to hopefully make it more clear.
Removed the - 1 from the loop as it was no longer needed.
Comment 32•2 years ago
|
||
Comment on attachment 9076001 [details] [diff] [review] 1530213.patch Thanks, this looks OK to me now. I'm not a Calendar peer, so formally I can't review this. Geoff has looked at this before, so he'll review it now.
Assignee | ||
Comment 33•2 years ago
|
||
Gotcha, thanks for looking at it Jorg.
Comment 34•2 years ago
|
||
Comment on attachment 9076001 [details] [diff] [review] 1530213.patch Review of attachment 9076001 [details] [diff] [review]: ----------------------------------------------------------------- Okay, but please fix up the comment. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +956,5 @@ > if (entries.length > 0) { > + // We set currentIndex based on the mailing list item location using mMaxAttendees with an offset of -2. > + // The first: So that mMaxAttendees starts counting from 0. > + // The second: To replace the mailing list entry with the first member in the mailing list. > + let currentIndex = this.mMaxAttendees - 2; This comment is indented one space too many, and the first line should be split in two. Plus I think I'd rewrite it to something like this: "Start replacing rows at the currently active row, which is the second-to-last row that isn't a dummy row. (mMaxAttendees being a count of non-dummy rows.)"
Updated•2 years ago
|
Comment 35•2 years ago
•
|
||
A bit disappointing that the patch doesn't apply at all:
$ hg -v qpush
applying 1530213.patch
patching file calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
bad hunk #1 @@ -927,53 +927,57 @@ class MozCalendarEventAttendeesList exte
(54 53 57 57)
Looks like you edited it manually and didn't adjust those numbers :-(
EDIT: Yes, it needs to be @@ -927,53 +927,56 @@ class MozCalendarEventAttendeesList exte
since you're adding three lines not four.
Comment 36•2 years ago
•
|
||
// Start replacing rows at the currently active row, which is the
// second-to-last row that isn't a dummy row. Note that mMaxAttendees
// is a count of non-dummy rows.
Sorry, this is is still not clear, so I had to try it out.
With myself as organiser, another invitee and then the list being replaced on position 3 (counting from 1), I saw a value of 4, so I really have no idea what this is counting. Also, what's a dummy row?
The patch works although it's still not clear to me why we're doing this:
- for (let i = currentIndex; i <= this.mMaxAttendees; i++) {
+ for (let i = currentIndex; i < this.mMaxAttendees; i++) {
Was that wrong before?
I came to land that, but I can't :-(
Comment 37•2 years ago
|
||
It appears that was wrong before.
mMaxAttendees
(which does appear to be an odd name) is the count of rows that contain an icon and a space to accept an email address (or the organiser, I guess, which doesn't accept an email address, but has one). All of the other rows are dummy rows and only contain the bottom border. The bottom borders don't actually line up across the window, but that's a story for another bug.
Comment 38•2 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2f0f868999d8
Make invite attendees expand mailing lists. r=darktrojan DONTBUILD
Comment 39•2 years ago
|
||
Landed with more accurate comment as discussed with Geoff on IRC.
Comment 40•2 years ago
|
||
TB 68 beta 5 / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/be43f86e0509f04130b512f61ddbbe2777d2bcb2
Reporter | ||
Comment 41•2 years ago
|
||
Bug 1565999 and bug 1565992 filed for the problems discovered while reviewing this.
Updated•2 years ago
|
Description
•