Closed Bug 1272243 Opened 4 years ago Closed 3 years ago

Tasks are not ordered by start/due time in Multiweek/Month view (Lightning)

Categories

(Calendar :: Calendar Views, defect)

Lightning 4.0.5.2
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lopezibanez, Assigned: bv1578)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160425115337

Steps to reproduce:

Create two tasks (task11 and task10) on the same day with different start dates (11.00 and 10.00).


Actual results:

 In Multiweek and Month view, task11 shows before than task10.


Expected results:

Task11 should appear after task10. Note that in Week and Day views tasks are correctly ordered.
Attached patch patch-v1 (obsolete) β€” β€” Splinter Review
Confirmed.
In multiweek/month views the tasks are added without sorting by date.
As far as I can see it's always been like that, but I can't find an older bug report so I validate this one.

The patch should fix.
Assignee: nobody → bv1578
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8752548 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Attachment #8752548 - Flags: review?(philipp) → review+
I was just adding the links when you posted.
I was pretty sure I had tried this on the try-server along with another patch but I was wrong.

Sorry for the extra work.
Attached patch patch - v2 (obsolete) β€” β€” Splinter Review
The patch fails with test_compare_todo():

http://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_view_utils.js#106

The test calls "view.compareItems" with two tasks without entry and due date but actually this condition never arises when sorting elements that have to be displayed in the view, because at least one date is required:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#84

I've added two checks about null dates to avoid the problem.
Maybe the second check is a useless caution because Lightning always adds the end date even for the few cases considered in rfc5545 of events without DTEND (event with DTSTART + DURATION or event all-day, one day, with only DTSTART date):

http://mxr.mozilla.org/comm-central/source/calendar/base/src/calEvent.js#176


Here the try-server build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f4c7e1f0924aa6636e68a57f6312dc7a756cbc1&selectedJob=19323
Attachment #8752548 - Attachment is obsolete: true
Attachment #8762674 - Flags: review?(philipp)
Comment on attachment 8762674 [details] [diff] [review]
patch - v2

Review of attachment 8762674 [details] [diff] [review]:
-----------------------------------------------------------------

r+, sorry for the delay. I think an extra null check is fine, better safe than sorry!

Note that we have landed patches to run eslint on the code. It is not (easily) runnable at the moment since I have to finish up some m-c patches, but there may be issues here due to the one-line ifs. Check the current code for details. Please email me if you have questions, I'm happy to help you set this up.

r=philipp with this minor issue:

::: calendar/base/modules/calViewUtils.jsm
@@ +42,3 @@
>          if (cmp != 0) return cmp;
>  
> +        if (!aEndDate || !bEndDate) return 0; 

Whitespace snuck in here
Attachment #8762674 - Flags: review?(philipp) → review+
I did some hack to make ESLint working with the rules in \calendar\.eslintrc and I get in the output just a warning about the definition of the rule 'mozilla/import-globals' which was not found, so I think this patch is OK.
Attachment #8762674 - Attachment is obsolete: true
Attachment #8792955 - Flags: review+
Keywords: checkin-needed
Thanks for going through the effort! I just landed an update to eslint on mozilla-inbound, so very soon you should be able to update mozilla-central and have it work without hacks. There is a command mach eslint, you can run mach eslint calendar for example. Right now there is still an issue with the project dir not being right so you may need to do ln -s mozilla/tools in the comm-central directory. I'll send around more instructions soon. The patch looks fine though, so nothing left to do (other than wait for someone to push) in this bug.
https://hg.mozilla.org/comm-central/rev/6d6f444d58eb6c7f893c74ea41b7ab9f96e94e46
Bug 1272243 - Tasks are not ordered by start/due time in Multiweek/Month view (Lightning). r=Philipp
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.4
You need to log in before you can comment on or make changes to this bug.