337848 - Only use times for Today and Tomorrow in the agenda view

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Joey Minta]

Based on a suggestion from m.d.a.calendar.

Within the agenda view, the dates in the Today and Tomorrow categories are redundant with the headers.  We should simply format the times and display them.

Comment 1

[Richard Duivenvoorde]

ok will give this a try..

Comment 2

[Majken Connor [:Kensie]]

We might want to still have the dates somewhere in the tree for quick glance date checking.  I was thinking somewhere aligned with the word today, either with it in the same column "Today - May 1" or in the time column...

Today....................Thursday, May 1
-first event......................9:30

Comment 3

[Joey Minta]

(In reply to comment #2)
> We might want to still have the dates somewhere in the tree for quick glance
> date checking.  I was thinking somewhere aligned with the word today, either
> with it in the same column "Today - May 1" or in the time column...
> 
> Today....................Thursday, May 1
> -first event......................9:30
> 
I think this is probably a good idea in some form, but at the risk of starting another endless UI discussion, let's go ahead and focus on trimming the dates right now.  Perhaps simply adding tooltips to the events (and maybe 'Today' could have a tooltip with the expanded date) would be sufficient.

Comment 4

[Richard Duivenvoorde]

Attachment: small fix to see only time (without date) in agenda view (today and tomorrow)

for today and tomorrow's events only the time will show in second column of agenda-view (instead of date+time).
also added some lines of code to be able to show the date for today and tomorrow
(will post attachment with screenshot)

Comment 5

[Richard Duivenvoorde]

Attachment: new agendaview as image

Image of agendaview after fix. Date after 'Today' and 'Tomorrow' is showing now, but can easily be removed from code if giving to much discussion

Comment 6

[Stefan Sitter [:ssitter]]

Comment on attachment 223706 [details] [diff] [review]
small fix to see only time (without date) in agenda view (today and tomorrow)

>+	    if (event.title == "Today")
>+		    return dateFormatter.formatDate(this.today.start); 
>+	    else if 
>+		    (event.title == "Tomorrow")
>+			    return dateFormatter.formatDate(this.tomorrow.start);

Does this work with localized versions of Lightning? The displayed strings are read from http://lxr.mozilla.org/mozilla/source/calendar/lightning/locale/lightning.properties#39 and set in http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/agenda-tree.js#50

Comment 7

[Joey Minta]

Comment on attachment 223706 [details] [diff] [review]
small fix to see only time (without date) in agenda view (today and tomorrow)

OK, I like the bits about not showing the date, but some of the Today and Tomorrow changes need a bit of work.  We may want to split those two tasks into two patches.  Your choice.

function getCellText(row, column)
 {
     var event = this.events[row];
     if (column.id == "col-agenda-item") {
         if (event instanceof Synthetic)
             return event.title;
-        return event.title;
+        return event.title;	
Don't make random whitespace changes, please.
     }
 
+    var dateFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
+	    .getService(Components.interfaces.calIDateTimeFormatter);
When you break up a long line like this, you should line up the '.' so it looks like
    var dateFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
                                  .getService(Components.interfaces.calIDateTimeFormatter);
     if (event instanceof Synthetic)
-        return "";
+    {
+	    // OPTIONAL display today's or tomorrow's date in the second column
+	    if (event.title == "Today")
+		    return dateFormatter.formatDate(this.today.start); 
+	    else if 
+		    (event.title == "Tomorrow")
+			    return dateFormatter.formatDate(this.tomorrow.start);
+	    else
+		    return "";
+    }
ssitter is right, this won't work for localized versions of Lightning.  Also, always use braces, even for 1-line if/else.  In general, you should try to match the style of the code that's already in the file.  In this case, all the other code does
    if (foo) {
    }
with the opening brace on the same line.

+    // ok we have an 'event' here:
     var start = event.startDate || event.dueDate;
-    var dateFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
-                                  .getService(Components.interfaces.calIDateTimeFormatter);
-    start = start.getInTimezone(calendarDefaultTimezone());
-    return dateFormatter.formatDateTime(start);
+    if (start.compare(this.tomorrow.end) == -1) // show time only for today and tomorrow's events
+	    return  dateFormatter.formatTime(start.getInTimezone(calendarDefaultTimezone()));
watch out for the 2 spaces here.  Also, there shouldn't be any tabs in your patches.  Always use spaces.
+    else
+	    return dateFormatter.formatDateTime(start.getInTimezone(calendarDefaultTimezone()));  
 };

As I said, the time bit looks pretty good, and most of this stuff is just getting used to mozilla styling conventions, which takes a bit of time.  The main rule is: match the existing style of the file you're touching.  Looking forward to another version of this patch!

Comment 8

[Richard Duivenvoorde]

Attachment: second try to view time only on today and tomorrow events

next try: I was working with sources from the windows lightning release, apparently not up to date (no localisation in that one yet...)

Comment 9

[Joey Minta]

Comment on attachment 223917 [details] [diff] [review]
second try to view time only on today and tomorrow events

This looks a lot better!  I just have a couple of minor things at this point:

+    // first column
In theory, the columns could be re-ordered, so I think it'd be better if you said "title column" and "date column" rather than first/second.

+    if (event instanceof Synthetic){
+        if (event == this.today){
Style nit: put a space between the ) and the {. (same in other places)

     start = start.getInTimezone(calendarDefaultTimezone());
-    return dateFormatter.formatDateTime(start);
+    if (start.compare(this.tomorrow.end) == -1){
+        // only time for events on today and tomorrow
+        return  dateFormatter.formatTime(start.getInTimezone(calendarDefaultTimezone()));
+    }
Oops, you've already converted start to the right timezone before you get into the if/else, so you don't need the getInTimezone call here.

Looks really good though.  I think the next iteration should do it.

Comment 10

[Richard Duivenvoorde]

Attachment: show time only for today/tomorrow in agenda view

thanks for reviewing! I changed everything which was mentioned in the last review.

about code style: file is a little messy (different styles used). I didn't want to change that and mess up the diff file, but isn't there some automatic code style checker for javascript (like you have for java in Eclipse and IntelliJ (or maybe those can be used??).

Comment 11

[Joey Minta]

Comment on attachment 224310 [details] [diff] [review]
show time only for today/tomorrow in agenda view

I like it! After talking this over with dan, we decided it still is a bit difficult to distinguish the Today/Tomorrow/Soon rows, and with a date there, they look even more like events.  Can you file a follow-up bug to add some special styling to those rows (maybe bold, like TB)?
r=jminta

Comment 12

[Joey Minta]

Patch checked in.

Comment 13

[Richard Duivenvoorde]

> Can you file a follow-up bug to add some
> special styling to those rows (maybe bold, like TB)?

Ok done: 340477