611973 - Sorting Order of Reminders / Alarms

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[supporter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Lightning 1.0b2

The oder in which appointments are displayed in the notification dialog is descending by default.

The appointment that is closest to today is shown as the last appointment in the list.

I would suggest to either create a property in Lightning's property dialog to determine the sorting order (ascending or descending) or to have the appointments sorted in ascending order by default (appointment that comes first should be displayed as first appointment)

Reproducible: Always

Comment 1

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

I think we should sort ascending by default. I believe at the moment we don't do any sorting at all, the notifications just pour in.

Comment 2

[supporter]

You are probably right, the sorting order can change from time to time (or better log-in to log-in).

Sorting it in ascending order is a good idea, so the appointments pop up in the order they occurr (first appointment to last / appointment that is next then appointments several days ahead).

Comment 3

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

supporter will be taking care of this bug, thanks for your offer!

Comment 4

[supporter]

Attachment: This ZIP file contains the changed files "calendar-alarm-dialog.js" and "calUtil.js"

I introduced new functions binaryInsertNode(..) and binarySearchNode(..) in the file "calUtils.js".

The function addWidgetFor(..) in the file "calendar-alarm-dialog.js" was changed, so that new widgets in the alarm popup-window appear in a sorted manner. To compare the alarm dates of the widgets, a comptor-function "widgetAlarmComptor(..)" was introduced.

Please have a look at this patch and review it!

Thanks in advance

Comment 5

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

Attachment: Patch for review - v1

Hey Roman, sorry for the delay. Here's your zip files as a patch I can better review.

Comment 6

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

Comment on attachment 523773 [details] [diff] [review]
Patch for review - v1

>+ *   Roman Käppeler <[e-mail]>
I probably fscked this up myself when converting to a patch. I assume you've taken care that the file is saved in utf8.

Another thing I changed is converting tabs to spaces. Please either apply this patch and use it as a base, or convert your changes locally.


>+ * Comparison function for the start date of a calendar item and
>+ * the start date of a calendar-alarm-widget. 
>+ * 
>+ * @param aItem                A calendar item for the comparison of the start date property
>+ * @param calAlarmWidget       A calendar-alarm-widget for the start date comparison with the given calendar item
>+ * @return                     1 - if the calendar item starts before the calendar-alarm-widget
>+ *                             -1 - if the calendar-alarm-widget starts before the calendar item
>+ *                             0 - otherwise
>+ */
>+function widgetAlarmComptor(aItem, calAlarmWidget) {
>+    
>+    if (aItem == null || calAlarmWidget == null || calAlarmWidget.item == null) return -1;
>+    
>+    // Convert to String to compare the two start dates
Why use a string compare? We have a compare function for dates, see calIDateTime.


>-        // Set the selected element if there is none yet. Since the onselect
...
>+    // Always select first widget of the list.

The addWidgetFor function may be called when the dialog is already open, i.e the user is looking at his alarms and another alarm fires. If you always select the first widget, then the selection will jump around if the user has selected something different.



>+function binarySearchNode(parentNode, newNode, aItem, comptor) {

binarySearchNode is very similar to binarySearch. Maybe its possible to just use binarySearch with parentNode.childNodes as an argument?

>+    if (newIndex < 0) {
>+    parentNode.appendChild(insertNode);
>+    newIndex = 0;
>+    } else if (!discardDuplicates ||
When converting tabs to spaces, make sure indentation is still correct. I did it wrong in this function :-)

In general the code you've written looks great, just a few minor nits. From testing there seems to still be an issue though. I created three events (N Event, K Event, P Event, in that order), but they were not sorted when occurring.

I'm setting r- just to get a new patch with the nits fixed, I'd appreciate if you could use hg diff to create the patch in this kind of format next time.

Thanks for looking in to this!

Comment 7

[supporter]

Thanks for your help and sorry for the zip file!
I will provide a diff-file next time, so that it's easier to compare
the differences!

The events should be displayed in ascending order, which means, that the event which takes place first, is displayed as your first element in the Alarm Window. I will have a look at the order again, to verify if there are still issues with it.

Comment 8

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

Sorry, that was totally my fault. I mistakingly looked for ascending by title, which is of course not what we are looking for :)

Comment 9

[supporter]

Attachment: Patch for review - v2

- Correction of Tabs to spaces
- Used calIDateTime for startDate comparison in widgetAlarmComptor
- Replaced binarySearchNode by binarySearch with parentNode.childNodes as argument

Selection of items in function addWidgetFor:
I was confused by the selection many times when I opened TB and Lightning and the alarm window popped up. The problem is, that the alarmRichlist.selectedIndex is set to 0 within the if-block even when the user did not click an item. It may happen that events pop up that have to be sorted before this selected element so that when lightning is opened the selection is not intuitive. I think when lightning is started the first element should be always selected.

Is there a way to detect that a user explicitly selected an item? If so, this would be a better solution. Just check if the user has clicked an item and therefore changed the selection or if it's the default selection of the item 0. I would keep it the way I changed it - think about it!

Comment 10

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

Comment on attachment 523854 [details] [diff] [review]
Patch for review - v2

(In reply to comment #9)
> Created attachment 523854 [details] [diff] [review]
> Patch for review - v2
> 
> - Correction of Tabs to spaces
> - Used calIDateTime for startDate comparison in widgetAlarmComptor
> - Replaced binarySearchNode by binarySearch with parentNode.childNodes as
> argument
> 
> Selection of items in function addWidgetFor:
> I was confused by the selection many times when I opened TB and Lightning and
> the alarm window popped up. The problem is, that the
I see what you mean. There is no direct way to detect this, but I guess you could save a variable that is set when the user selects something manually and check for it when deciding if the widget should be selected.

> I would keep it the way I changed it - think about it!
I still think this might create a wierd user experience. If you have the last item of a long set of reminders selected and then a new alarm pops up, then you have to scroll down to the bottom again.

Aside from this issue, your patch looks great! r=philipp, but I would appreciate if you could upload a new patch with the selection issue fixed. Feel free to mark that patch r+ too, as you are carrying forward the review.

Comment 11

[supporter]

Attachment: [for beta]Patch for review - v3

Function "onSelectAlarm(..)" adds a new variable "userSelectedWidget" to the alarm-rich-list and sets its value to "true". The variable indicates that an item in the Calendar Alarm Dialog has explicitly been selected.

Function "addWidgetFor(..)" evaluates the value of "alarmRichlist.userSelectedWidget":

=> if no widget was selected by a user the first item of the list will be highlighted/selected

=> if an item of the list was selected and new events pop up in the Calendar Alarm Window they will remain highlighted/selected as long as the Calendar Alarm Dialog stays open.

Comment 12

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

Comment on attachment 524845 [details] [diff] [review]
[for beta]Patch for review - v3

Looks great, sorry for the delay! r=philipp

Comment 13

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/ad5f8b0ed1c3>
-> FIXED

Comment 14

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

Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/d4236b5c9ef6>. I've filed a bug to get a new target milestone :)