377041 - Sorting should put tasks without date alway at the bottom

Status: RESOLVED FIXED

(Calendar :: Tasks, enhancement)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The most effective sort order for tasks by date is from past to future. Tasks without a specified date should be at the end of the list, even if the list is sorted from future to the past, because the user wants to see the tasks which need to be finished soon.

Comment 1

[Stefan Sitter [:ssitter]]

Dupe of Bug 372879? See Bug 372879 Comment #8 for information on current sort order, see Bug 307137 Comment #7 for reasons that lead to decision.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

See "each successive calendar date" in comment 7 and Bug 307137 Comment #8 , it is not specified that now() should be used. Furthermore, tasks without date are expected to be done when the user has some time left, so they are voluntary tasks (compared to the "forced" tasks with dates which can't be seen if you have many voluntary tasks because the lists gets scrollable).

Comment 3

[cmtalbert]

Normally, we would not revisit decisions made in other bugs, but we are making an exception here since we are discussing re-designing the entire task UI.  Therefore, we are taking this discussion to the newsgroup mozilla.dev.apps.calendar please join the discussion there.  This bug can be used to implement part of whatever the outcome of that discussion is.

Comment 5

[Brennan Vincent]

Could this possibly be made a preference?

Comment 7

[Julius Stroffek]

Attachment: Work in Progress Patch

Hi All,

I am attaching the work in progress patch. It changes the order based on due date only (and due in since it's the same). The patch seems to work pretty well for me.

I have also added the time how much the task is overdue if it was not already completed. Please, have a look and provide some feedback.

Should the completed tasks be treated as without a due date? Since it does not make much sense anymore.

What other date do you think should be sorted in a same way? The ones without a date are always put at the end.

Appreciate any feedback.

Cheers

Julo

Comment 8

[Martin Schröder [:mschroeder]]

(In reply to comment #7)
> Created an attachment (id=388103) [details]
> Work in Progress Patch

Thanks for looking into this, and writing a first patch. Maybe you want to have a look at https://wiki.mozilla.org/Calendar:Module_Ownership for more details on the review process of patches for Calendar code.

Comment 9

[Julius Stroffek]

Attachment: [medium,l10n] Final Patch for Review

I have done minor cleanups in the patch and have changed the order of all dates of the task to the new way (putting entries without the date specified always at the end) as requested by the bug.

I have also added the code that shows how long the task is overdue for overdue tasks. Please, provide feedback.

Comment 10

[Julius Stroffek]

Comment on attachment 388160 [details] [diff] [review]
[medium,l10n] Final Patch for Review

Adding a "Requestee" does not work for me.

Normally, I do not see any text field below "Requestee:" caption.

The text boxes come up only if I disable javascript which then does not add the request for review (I do not see it listed in "My Requests").

Philipp, could you please review this patch?

Thanks!

Comment 11

[Stefan Sitter [:ssitter]]

Comment on attachment 388160 [details] [diff] [review]
[medium,l10n] Final Patch for Review

To request review select "?" in the review flag dropdown list and enter the requestee name.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 388160 [details] [diff] [review]
[medium,l10n] Final Patch for Review

Due to the fact we are in string freeze I will postpone the review until after the beta.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 388160 [details] [diff] [review]
[medium,l10n] Final Patch for Review

>diff -r 53f9c5558d5a calendar/base/content/calendar-task-tree.xml
>--- a/calendar/base/content/calendar-task-tree.xml	Sun Jul 12 17:36:54 2009 +0200
>+++ b/calendar/base/content/calendar-task-tree.xml	Sun Jul 12 23:32:11 2009 +0200
>@@ -319,8 +319,23 @@
>                       // Less than one hour
>                       return calGetString("calendar", "dueInLessThanOneHour");
>                   }
>+              } else if (!aTask.completedDate || !aTask.completedDate.isValid) {
>+                  // Overdue task
>+                  var minutes = Math.ceil( -dur.inSeconds / 60);
let vs var, here and elsewhere in this patch

>+    compareNativeTimeFilledAsc: function cal_compareNativeTimeFilledAsc(a, b) {
>+      if (a == b)
>+        return 0;
>+      if (a == -62168601600000000) // value for (0000/00/00 00:00:00)
>+        return 1;
>+      if (b == -62168601600000000) // value for (0000/00/00 00:00:00)
>+        return -1;
>+
>+      return (a < b ? -1 : 1);
>+    },
>+
>+    compareNativeTimeFilledDesc: function cal_compareNativeTimeFilledDesc(a, b) {
>+      if (a == b)
>+        return 0;
>+      if (a == -62168601600000000) // value for (0000/00/00 00:00:00)
>+        return 1;
>+      if (b == -62168601600000000) // value for (0000/00/00 00:00:00)
>+        return -1;
>+
>+      return (a < b ? 1 : -1);
>+    },
I'd appreciate some comments on these functions. I initially thought that in compareNativeTimeFilledDesc, you could just return -compareNativeTimeFilledAsc(a,b), but if the time is 0000/00/00 00:00:00, then the same result is offered. Is this supposed to be that way?

>+            if (modifier == 1)
>+              return cal.compareNativeTimeFilledAsc(nsA, nsB);
>+            else
>+              return cal.compareNativeTimeFilledDesc(nsA, nsB);
use brackets {} with this if-else block.


r=philipp with that fixed, feel free to check in anytime.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

I've decided to leave away the string change here, I don't think we should show more granularity here, otherwise we'd need a timer to keep it up to date.

I've also taken care of the review comments.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/c40723287a1c>
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/2540ffeaa7a8>

-> FIXED