Closed Bug 287550 Opened 15 years ago Closed 14 years ago

Reimplement the 'attendee' tab of the 'new/edit event' dialog

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stelian, Assigned: jminta)

References

Details

Attachments

(4 files, 9 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050302 Firefox/1.0.1 Fedora/1.0.1-1.3.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050302 Firefox/1.0.1 Fedora/1.0.1-1.3.2

The code corresponding to the attendee tab contains a lot of dead and not
working code. It also doesn't let the user see or choose the role and status of
each attendee.

The attached patch rewrites most of the javascript code and redesigns the UI
too. There is room for improvement (like sorting the tree items by columns etc.)
but it's already functional and much better than before.

Reproducible: Always

Steps to Reproduce:
1. Look at the attendee tab
2.
3.
Attached patch Patch fixing the attendee tab (obsolete) — Splinter Review
Attachment #178470 - Flags: first-review?(pavlov)
Comment on attachment 178470 [details] [diff] [review]
Patch fixing the attendee tab

I'm OK with the IDL changes.
Ok, after some discussion on IRC, here is a new version which separates the name
textfield from the email one, puts better labels on buttons, puts the buttons
right after the list, and several other minor changes.

I am also attaching a screen shot of the dialog.
Attached patch New patch (obsolete) — Splinter Review
Attachment #178560 - Flags: first-review?(pavlov)
Attached image a screenshot of the dialog pane (obsolete) —
Attachment #178470 - Attachment is obsolete: true
Attachment #178470 - Flags: first-review?(pavlov)
This has been done already on trunk.  Any objection to marking this bug resolved?
No, this patch isn't on trunk. Keep this one open.
I'd prefer to see this bug closed and see sunbird use the new event dialog.... mvl?
that said, there are some interesting parts to the new UI and it is far more
complex and complete than the new UI (a textbox of comma seperated email
addresses).  I'm not sure how this fits in with the new dialog.. its kind of
large which i'm not a huge fan of, but it seems to handle a lot.
QA Contact: gurganbl → sunbird
Comment on attachment 178560 [details] [diff] [review]
New patch

I think the plan is to use the new dialog, so this patch is no longer relivant.
Attachment #178560 - Flags: first-review?(pavlov) → first-review-
Assignee: mostafah → michael.buettner
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
*** Bug 330112 has been marked as a duplicate of this bug. ***
Attached patch patch v1 (obsolete) — Splinter Review
This patch implements the 'attendee editing'-feature, please see http://wiki.mozilla.org/Calendar:Event_Dialog#Attendees_Tab_Page for how this looks like. Please note that this patch only addresses the listbox on the left side of the screenshot (without the role and status icons). Address book integration and LDAP support are included.
Attachment #178560 - Attachment is obsolete: true
Attachment #178561 - Attachment is obsolete: true
Attachment #222040 - Flags: first-review?(jminta)
Please don't break Sunbird by purpose. I tried this patch and all I get are errors like:

Warning: reference to undefined property Components.classes['@mozilla.org/messenger/headerparser;1'] Source File: chrome://calendar/content/calendar-attendees.xml Line: 219

Error: Components.classes['@mozilla.org/messenger/headerparser;1'] has no properties Source File: chrome://calendar/content/calendar-attendees.xml Line: 219

Error: this.mPrefBranchInternal has no properties  Source File: chrome://calendar/content/calendar-attendees.xml Line: 274

Error: this.mPrefs has no properties  Source File: chrome://calendar/content/calendar-attendees.xml Line: 656
(In reply to comment #13)
> Please don't break Sunbird by purpose. 
ssitter is right.  All these patches should be tested on Sunbird too.  Also, for all of these, it would make things much easier if you could attach a screenshot of how the dialog looks with the changes.
Attached patch patch v2Splinter Review
of course i should have tested it with sunbird, sorry for that. this one will do the trick.
Attachment #222040 - Attachment is obsolete: true
Attachment #222203 - Flags: first-review?(jminta)
Attachment #222040 - Flags: first-review?(jminta)
Attached image screenshot
here's the screenshot showing the attendee-list.
Comment on attachment 222203 [details] [diff] [review]
patch v2

Some things I noticed on a first pass:

1.) There's a bunch of gross stuff in here computing box-heights, etc.  Really, it all seems to be pointing to a  <richlistbox>, which I think would save a lot of trouble/computation.

2.) document.getElementById() is always a red flag in xbl bindings.  Since xbl is meant to be re-used, if you have stuff that relies on ids, you have a good chance of ending up with 2 elements in the same document with the same id.

3.) dmose totally needs to review the ldap portions of this.

Smaller stuff:
-License header in calendar/base/content/calendar-attendees.css

+      <method name="createAttendee">
+        <body>
+          <![CDATA[
+            var attendee = createAttendee();
You can't rely on external functions within xbl.
-You assign this.mMaxAttendees = 0 twice in the attendees property.
-Why are there deleteHit and arrowHit methods?  That code only seems to get called from the event handlers, making me think it should probably just live there itself.
-The prefbranchinternal interface was renamed to prefbranch2 awhile back, so mPrefBranchInternal should probably be renamed.
-You have a couple of getService calls that need to be broken up into multiple lines.
(In reply to comment #17)
> 1.) There's a bunch of gross stuff in here computing box-heights, etc.
> Really, it all seems to be pointing to a  <richlistbox>, which I think
> would save a lot of trouble/computation.
the listbox just fills itself with either items containing an attendee or dummy items. this is done to always have as many items as the height of the listbox dictates, thus being able to display a grid regardless of how many real items the listbox contains. i never tried the <richlistbox>, but as far as i know it doesn't help here. besides that, since i already went through all this trouble it would be much more pain to revoke all of this stuff.

i'll address the other issues you mentioned, new iteration is in the pipeline :-)
Component: Sunbird and Calendar-Extension Front End → Base
OS: Linux → All
Hardware: PC → All
besides the above mentioned smaller issues, i was wondering whether we go for the implementation of the list as it currently stands or for the suggested richtlistbox? since i can't see any real objections against the current version, i assume we agree on the grid-style listbox, right? it doesn't make any sense for me to submit a new iteration with those smaller stuff fixed but the core of the patch being not agreed on.
This is a log of the discussion that happened a few days ago in IRC concerning the grid/list question.  The general impression I got was that the grid-style was discouraged, but a clear consensus wasn't really arrived at.
i generally didn't have the impression that either of the alternatives were prefered, so i can't see a reason for holding back this patch. i wouldn't have a problem to change the implementation of the attendee control if we later decide to use any other type of control, but i would suggest continuing with this patch, as it provides substantial value add besides those nonrelevant styling issues.
Whiteboard: [cal-ui-review needed]
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: sunbird → base
The UI owners met on this proposal.  I was recruited to post a summary of the decision here.

The overall a decision was to go with a "modified grid" style.  For those of you who haven't been following the discussion where 'grid' and 'list' acquired special meaning, this means essentially:
-A list of attendees similar to the compose window and the screenshot here.  However, rather than fill in all the space below the last typed attendee with blank lines (the 'grid'), the UI owners would like to always keep 1 extra line with grey text saying something like 'Click to add attendee'.
-The list's rows should support both names only and names+email.
-The list wants to complete first from previously entered attendees, and then from address book/ldap server.
-We'll want some icons (similar to the icons in http://wiki.mozilla.org/Calendar:Event_Dialog#Attendees_Tab_Page) to show confirmed/declined/etc.
-We don't want the rows to be as thin as in the current screenshot in this bug.

This proposal has the advantage of not needing extra widgets for adding/removing attendees.

I'll note on my own that not all of these points should be included in the original implementation patch, as that will get rather large.  dmose explicitly mentioned that the icons bit can easily wait until later.

dmose and I are going to have a conversation later this afternoon about (a) implementation plans (b) who should review/drive this bug (c) to make sure my report here is an accurate reflection of the decision/conversation he and mvl had.
Whiteboard: [cal-ui-review needed]
Attached patch basic list (obsolete) — Splinter Review
Patch is an implementation of the UI decision here.  Follow-up patches can be made to handle things like auto-complete and maybe confirmed/declined status.  Focus code sucks.
Attachment #232655 - Flags: second-review?(dmose)
Attachment #232655 - Flags: first-review?(mattwillis)
Comment on attachment 222203 [details] [diff] [review]
patch v2

Cancelling review request, given the UI-review comments.
Attachment #222203 - Flags: first-review?(jminta)
Comment on attachment 232655 [details] [diff] [review]
basic list

In calendar/base/content/calendar-attendee-list.xml:
+Components.utils.reportError("maybe finish");
Looks like you've left some debugging statements. I'm assuming you want to remove them.

+          for each (att in val) {
+            this.addAttendee(att);
+          }
How do we want to deal with duplicate attendees? Clearly this loop will only show them once, but should they be de-duped? Or am I over-worrying about this?

+                setTimeout(reFocus, 5);
Is that in milliseconds or seconds? Please tack on a comment noting which.


In calendar/lightning/jar.mn:
   content/calendar/calendar-event-dialog.css	(/calendar/base/content/calendar-event-dialog.css)
+    content/calendar/calendar-attendee-list.xml (/calendar/base/content/calendar-attendee-list.xml)
   content/calendar/calendar-recurrence-dialog.js	(/calendar/base/content/calendar-recurrence-dialog.js)
What's up with the different indenting style?

In calendar/base/content/calendar-event-dialog.js:
+    var attendeeListBox = document.getElementById("attendees-list");
+    for each (att in attendeeListBox.attendees) {
+        item.addAttendee(kid.attendee);
     }
[Exception... "'[JavaScript Error: "kid is not defined" {file: "chrome://calendar/content/calendar-event-dialog.js" line: 370}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

Changing kid to att gives this:
[Exception... "'[JavaScript Error: "this.mAttendees[i] has no properties" {file: "file:///Users/mwillis/moz/printing/mozilla/obj-i386-apple-darwin8.7.1/dist/CalendarDebug.app/Contents/MacOS/js/calItemBase.js" line: 645}]' when calling method: [calIEvent::icalString]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://calendar/content/calendar-event-dialog.js :: saveDialog :: line 398"  data: yes]


Something is still screwy in the focus code. After entering an attendee name in the Event Dialog, I have to click twice in any other (non-attendee-list) textfield to be able to actually type. The first click creates a new blank line in the attendee list, but doesn't change focus to the field I clicked on. If I click once and type without actually looking at what's going on, I'm typing in the new blank attendee box.

We also need to handle adding more attendee lines than the dialog has space for. Right now, if I add more than six attendees, the "Less", "Cancel" and "OK" buttons begin to be cut off by the bottom of the dialog. Each additional attendee just makes it worse. Clicking "Less" and then "More" redraws the dialog correctly, but that's a crappy workaround.

While I'm nitting, the attendees box draws itself a few pixels left from all the other boxes, at least on the Mac. We should see if that happens on other platforms, and fix it if we can.

Minusing on the exceptions and typing focus fun
Attachment #232655 - Flags: first-review?(mattwillis) → first-review-
Attached patch basic list v2 (obsolete) — Splinter Review
Patch updated to previous review comments.  I also made a behavioral change here, after playing with this some more.  Originally, pressing enter or blurring the listbox would give you a new row.  This struck me as fairly undiscoverable, not to mention that it conflicted with enter-closes-dialogs.  Therefore, we now give you a new row as soon as you type something non-null in the previously blank row.
Attachment #232655 - Attachment is obsolete: true
Attachment #232818 - Flags: second-review?(dmose)
Attachment #232818 - Flags: first-review?(mattwillis)
Attachment #232655 - Flags: second-review?(dmose)
Attached patch basic list v2.1 (obsolete) — Splinter Review
The previous patch didn't have one last edit in it.
Attachment #232818 - Attachment is obsolete: true
Attachment #232819 - Flags: second-review?(dmose)
Attachment #232819 - Flags: first-review?(mattwillis)
Attachment #232818 - Flags: second-review?(dmose)
Attachment #232818 - Flags: first-review?(mattwillis)
Comment on attachment 232819 [details] [diff] [review]
basic list v2.1

[20:26] 	*	jminta is trying to figure out what other bugs he really needs to get
[20:29] 	lilmatt	The focus issue when you delete something in the middle, the fact that which item you deleted persists, the ever expanding dialog vs. scrollbars
[20:29] 	lilmatt	The scrollbar thing I think is the most egregious

Minusing until we figure this scrollbar crap out.
Attachment #232819 - Flags: second-review?(dmose)
Attachment #232819 - Flags: first-review?(mattwillis)
Attachment #232819 - Flags: first-review-
Attached patch scrollbars and deleted items (obsolete) — Splinter Review
Patch should fix the scrollbar issue.  Also includes some changes so that on-load, we always have the blank attendee at the bottom.
Attachment #232819 - Attachment is obsolete: true
Attachment #233165 - Flags: second-review?(dmose)
Attachment #233165 - Flags: first-review?(mattwillis)
Comment on attachment 233165 [details] [diff] [review]
scrollbars and deleted items

Yay!

r=lilmatt
Attachment #233165 - Flags: first-review?(mattwillis) → first-review+
Attached patch re-insert 'enter' handling (obsolete) — Splinter Review
dmose said that we should definitely listen for the enter key and move to the next attendee when that happens.  This patch reintroduces that functionality.
Attachment #233165 - Attachment is obsolete: true
Attachment #233862 - Flags: second-review?(dmose)
Attachment #233862 - Flags: first-review+
Attachment #233165 - Flags: second-review?(dmose)
Comment on attachment 233862 [details] [diff] [review]
re-insert 'enter' handling

+      <method name="addAttendee">
+        <parameter name="aAttendee"/>
+        <parameter name="aDontFocus"/>
+        <body><![CDATA[
make it clear that this is used both to add calIAttendees and when someone hits enter, etc to add blank rows.

Actually, put comments before most methods

+          var att = document.createElement("calendar-attendee-item");
+          this.appendChild(att);
+          if (aAttendee) {
+            att.attendee = aAttendee;
Here and elsewhere, please don't use att for an element.  It makes it hard to follow.

+        // focus code sucks.  Work around some scoping problems here. (Listening
+        // for an event gives you the wrong |this| object, so things like
+        // mShowingInstructions will be undefined, which is bad.)
Re-write this comment. |this| isn't wrong, it's just not what you want.

+          if (val.id && val.id.toLowerCase().indexOf("mailto:") != -1) {
+              textbox.value = val.id.toLowerCase().split("mailto:")[1];
mailto will always be first, if it's there.  So just use substr.

+          if (aEvent.keyCode != 13) {
+            return;
+          }
Maybe use a prettier version of this constant?

+          aEvent.preventDefault();
+          document.commandDispatcher.advanceFocus();
Comment here.

+  -moz-binding: url(chrome://calendar/content/calendar-attendee-list.xml#calendar-attendee-list);
+  background: white;
+  -moz-user-focus: normal;
Make sure the tabbing behavior in and out of the listbox is correct.

+
+attendeeInstructions=Type new attendee
After discussion, let's just make the string "email@example.com" and file a separate bug for other use-cases of attendees.
Attachment #233862 - Flags: second-review?(dmose) → second-review-
Attached patch more commentsSplinter Review
Patch addresses previous review comments.  Bigger this time because there are a lot more comments.  The only code changes are renaming att/attItem to attElem, and using substr instead of indexOf.
Attachment #233862 - Attachment is obsolete: true
Attachment #234063 - Flags: second-review?(dmose)
Attachment #234063 - Flags: first-review+
Assignee: michael.buettner → jminta
Status: ASSIGNED → NEW
Comment on attachment 234063 [details] [diff] [review]
more comments

+      <field name="mListenerMap">null</field>
Comment explaining this.

+      <!-- Call this if you are in doubt about whether or not there is still
+         - a blank row in the listbox.  (There should always be one, so that
+         - a user can easily add a new attendee.)
+        -->
Tweak this comment so that it's clear who is responsible for adding the new blank row.†

+      <!-- Call this to when the user is no longer typing in the textbox.  If
+         - the textbox is blank, we'll restore the original grey instructions.
+        -->
+      <method name="showInstructions">
+        <body><![CDATA[
Consider renaming this, to include something about 'if blank'.

r=dmose with that.
Attachment #234063 - Flags: second-review?(dmose) → second-review+
Patch checked in.  Please file followups for additional issues.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.