409968 - Modifying all items of a recurring event runs into exceptions

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Daniel Boelzle [:dbo]]

Error: rangeStart has no properties
Source File: file:///d:/moz18/sbird-debug/dist/xpi-stage/lightning/js/calRecurrenceInfo.js
Line: 351

Caused by:

 // get all sub items if it is a recurring item
 -  647     var occs = item.getOccurrencesBetween(this.agendaListbox.getStart(),
 -  648                                           this.agendaListbox.getEnd(), {});
 -  649     occs.forEach(isSelected = this.agendaListbox.deleteItem, this.agendaListbox);
 -  650     return isSelected;

=> getOccurrencesBetween(null, null, ...) is called.

Comment 1

[Daniel Boelzle [:dbo]]

I need to switch collapse/uncollapse the today pane ranges, e.g. "Today" once to get those errors in the js console.

Comment 2

[Berend Cornelius]

Attachment: patch v.1

When the items are modified within the calendar view with agenda period being open  the code runs into a null pointer exception.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 295753 [details] [diff] [review]
patch v.1

Fix year. r=philipp

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 295753 [details] [diff] [review]
patch v.1

Philipp granted review, but I have some obvious style nits, especially camelCase capitalization of function and variable names all over agenda-listbox.js.

>Index: lightning/content/agenda-listbox.js

>- * The Original Code is Lightning code.
>+    - The Original Code is Sun Microsystems code.

This is mixed comment style.


>  *
>- * The Initial Developer of the Original Code is Oracle Corporation
>+ * The Initial Developer of the Original Code is Sun Microsystems.

According to http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c this should be two lines, and if you remove the contributors update the copyright year to 2007 (when the code was rewritten).


>@@ -334,18 +325,19 @@ function getlistItems(aItem, aPeriod) {

This function should be named "getListItems" (camelCase capitalization!).


> agendaListbox.deleteItem =
> function deleteItem(aItem) {
>     var isSelected = false;
>     var listItems = this.getlistItems(aItem);
>     if (listItems.length > 0) {
>         for (var i = listItems.length - 1; i >= 0; i--) {
>             var listItem = listItems[i];
>-            isSelected = listItem.selected;
>-            if (isSelected) {
>+            var lisSelected = listItem.selected;
>+            if (lisSelected) {
>+                isSelected = true;
>                 this.moveSelection();
>             }

Why do we need this new variable lisSelected (which name I don't understand)? What is the difference between isSelected and lisSelected? I don't like the naming conventions you follow. I prefer more meaningful names, so you understand their purpose without knowing the complete flow of the function.

>@@ -557,16 +549,32 @@ function removelistItems() {

This function should be named "removeListItems" (camelCase capitalization!).


>+agendaListbox.getlistItemByhashId =

This function should be named "getListItemByHashId" (camelCase capitalization!).


> agendaListbox.calendarObserver.onLocalDeleteItem =
[...]
>+    var SelectedItemhashId = -1;

Variable names should start with a lowercase letter.


> agendaListbox.calendarObserver.onModifyItem =
[...]
>+    var SelectedItemhashId = this.onLocalDeleteItem(oldItem);

Variable names should start with a lowercase letter.


>Index: lightning/content/agenda-listbox.xml

>-   - The Initial Developer of the Original Code is
>-   -   Mozilla Corp
>+   - The Initial Developer of the Original Code is Sun Microsystems.

According to http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-html this should be two lines.

Comment 5

[Simon Paquet [:sipaq]]

Good stuff Martin!

Berend, please only commit once you have adressed Martin's comments.

Comment 6

[Berend Cornelius]

>This function should be named "removeListItems" (camelCase capitalization!).
...
>This function should be named "getListItems" (camelCase capitalization!).
Basically I did not forget about Camel capitalization in my file. I just found it more appropriate to denote the name of the functions to the name of the common variable names like "listItem" or "hashId", that's why I used names like "getlistItem()" and "getlistItemByhashId". I will change that throughout the file.

>This is mixed comment style.
One of the files is an xml file where comments are wrapped in "<!-- ... -->".

>Why do we need this new variable lisSelected ...
The variable is necessary as within recurring events I just want to get to know if any item within the series is selected or not. Without this the variable would possibly be overwritten by the next unselected item state in the series. I will change the name to isSelected2. "l" was supposed to denote the more inner scope of the variable within the loop compared to "isSelected"

Comment 8

[Berend Cornelius]

Attachment: patch v. 2

final patch that I checked in

Comment 9

[Berend Cornelius]

patch check in on trunk and MOZILLA_1_8_BRANCH

Comment 10

[Andreas Treumann]

checked in lightning build 2008011003 -> task is fixed and verified.