325786 - Cannot add exceptions to recurring events in new item dialog

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

There is a 'Remove Exception' button, but no 'Add Exception' option.  This is pretty basic functionality.  Nominating for an 0.1 blocker.

Sunbird regression

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

If the work required to do this is "add a button that pops up an already designed-and-implemented dialog", blocking 0.1 sounds reasonable.  If the work is "design and implement a new dialog", I think the right tradeoff is shipping 0.1 sooner and living without this until the next version.

Comment 2

[I. Conver]

When adding exceptions, the only way to add multiple exceptions is to use the GUI date tool. Manually typing in an second,third,etc. additional date doesn't get noticed by the "Add Exception" button.

Comment 3

[Joey Minta]

Attachment: implement 'add exception'

Basic implementation of something I think a lot of users are going to want.  It doesn't have all the idiot-checking that the old dialog had (to prevent adding duplicate instances of the same exception, etc), but it ought to be dogfood at least.

dmose, do you think you'll have time to get to this prior to 0.1?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 211501 [details] [diff] [review]
implement 'add exception'

mvl offered to take some reviews off my hands, so here's one.  Thanks!

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 211501 [details] [diff] [review]
implement 'add exception'

>Index: calendar/base/content/calendar-recurrence-dialog.js
>+    var date = item.startDate || item.entryDate || item.dueDate;
>+    document.getElementById("exdate-picker").value = item.jsDate;

I bet you want date.jsDate, not item.jsDate

>+function addException() {
>+    this.sizeToContent();

Why do you need to size the window after adding an item to the listbox? The listboc should just stay the original size, and show a scrollbar when needed. With this sizeToContent, the size of the listbox changes for me. Looks weird.


When adding a date and removing it, without closing the dialig inbetween, an exception gets thrown:
[JavaScript Error: "[Exception... "'Illegal value' when calling method: [calIRecurrenceInfo::restoreOccurrenceAt]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://calendar/content/calendar-recurrence-dialog.js :: saveDialog :: line 240"  data: no]" {file: "chrome://calendar/content/calendar-recurrence-dialog.js" line: 240}]

So I guess this patch needs some minor tweaks..

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 211501 [details] [diff] [review]
implement 'add exception'

>+function addException() {
>+    var jsDate = document.getElementById("exdate-picker").value;
>+    var cdt = jsDateToDateTime(jsDate);

imo, variable names should indentify their meaning, not their type. Please pick more useful names here.

>+    exBox.appendItem(cdt.toString()).date = cdt;

That line is way to magical. Split it up, please. First append, then set attributes.

Comment 7

[Joey Minta]

Attachment: implement 'add exception' v2

Patch updated to review comments.  We now keep a list of the exceptions that were added during this dialog-opening.  That way, we won't fall victim to the add+remove bug.

The listbox is going to expand anyway, because of some weird xul-behavior.  using this.sizeToContent() is a workaround to keep the Ok/Cancel buttons from being cut off.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 213594 [details] [diff] [review]
implement 'add exception' v2

>+function addException() {
...
>+    this.sizeToContent();

Is sizeToContent really needed? That listbox should already have the correct size. In general, try to limit the call to sizeToContent to an absolute minimum, because it makes the window randomly resize.

> function removeSelectedException()
> {
>+    for (var ii in window.addedExceptions) {

Rename that ii into something more meaningful, please (just i is already a lot better, but ex or addedException would be best)

Comment 9

[Joey Minta]

(In reply to comment #8)
> (From update of attachment 213594 [details] [diff] [review] [edit])
> >+function addException() {
> ...
> >+    this.sizeToContent();
> 
> Is sizeToContent really needed? That listbox should already have the correct
> size. In general, try to limit the call to sizeToContent to an absolute
> minimum, because it makes the window randomly resize.
As I said, the listbox is behaving erratically, causing it to expand when an exception is added.  sizeToContent is the only way to keep the Ok/Cancel buttons from being pushed off the screen.

> 
> > function removeSelectedException()
> > {
> >+    for (var ii in window.addedExceptions) {
> 
> Rename that ii into something more meaningful, please (just i is already a lot
> better, but ex or addedException would be best)
It's just an index, not an object, so ex isn't a good name.  I can change it to simply i if you prefer.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 213594 [details] [diff] [review]
implement 'add exception' v2

ok, the listbox item seems to be a bug. Can you file one on it?
r=mvl

Comment 11

[Joey Minta]

Patch checked in.