Last Comment Bug 787357 - Print Preview shows "All day - All day" for allday events
: Print Preview shows "All day - All day" for allday events
Status: RESOLVED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: Printing (show other bugs)
: Lightning 1.7
: All All
: -- normal (vote)
: 1.8
Assigned To: Stefan Sitter
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 757902
  Show dependency treegraph
 
Reported: 2012-08-31 04:00 PDT by Philipp Kewisch [:Fallen]
Modified: 2012-09-23 03:37 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
regression fix (omit time for all-day items like in Lightning 1.6) (2.46 KB, patch)
2012-09-06 11:28 PDT, Stefan Sitter
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2012-08-31 04:00:22 PDT
Noticed this by two reporters from the blog. For all day events the print preview incorrectly shows the interval for all day events.
Comment 1 Stefan Sitter 2012-08-31 12:13:04 PDT
Caused here:
> http://mxr.mozilla.org/comm-central/source/calendar/base/modules/calPrintUtils.jsm#90
> let itemInterval = cal.getDateFormatter().formatItemTimeInterval(item);

This will always create a string like "starttime-endtime", i.e. "All Day-All Day" for an all day event. Previously there was more logic used to determine the correct format.
Comment 2 Rene Matteau 2012-09-02 15:20:11 PDT
In our case, it is the printing (monthly view) that show this behavior and it is a regression from the previous version we were using. A 3 days recurring all day event will print only on the first day as: "All Day-All Day Event name".

A fix would be appreciated. Thanks...
Comment 3 Stefan Sitter 2012-09-03 11:12:40 PDT
Rene, the second part of your problem is filed as bug 787537.
Comment 4 Rene Matteau 2012-09-03 12:05:13 PDT
Stefan,

Thanks for pointing me to the existing problem. Sorry I did not find it when I looked yesterday...
Comment 5 Stefan Sitter 2012-09-06 11:28:51 PDT
Created attachment 658938 [details] [diff] [review]
regression fix (omit time for all-day items like in Lightning 1.6)
Comment 6 Philipp Kewisch [:Fallen] 2012-09-21 10:42:36 PDT
Comment on attachment 658938 [details] [diff] [review]
regression fix (omit time for all-day items like in Lightning 1.6)

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

::: calendar/base/modules/calPrintUtils.jsm
@@ +164,5 @@
> +            return "";
> +        }
> +
> +        // Bug 787537: will result in wrong time label for events that span two or more days
> +        return cal.getDateFormatter().formatItemTimeInterval(aItem);

Hmm tough call. My original thought was that it makes more sense to fix this in the date time formatter, but if we always return a blank string there this might not be wanted in other places.

I see two options:

1. Keep the patch like it is (r=philipp for that)
2. Check other callers of formatItemTimeInterval() to see if an empty string makes sense there too and move this code into the formatter.

Let me know what you think
Comment 7 Stefan Sitter 2012-09-23 03:30:56 PDT
I think I'll go with pushing the patch as is for now to fix the current problem.

calDateTimeFormatter::formatItemTimeInterval() is only called once in calPrintUtils.jsm, therefore it might be possible to move the all day logic into it. We still need to work on this to fix the wrong times printed for multi day events. We can use Bug 359007 for this.
Comment 8 Stefan Sitter 2012-09-23 03:34:51 PDT
Pushed to https://hg.mozilla.org/comm-central/rev/601fc607aaf9
Comment 9 Stefan Sitter 2012-09-23 03:37:11 PDT
Pushed to https://hg.mozilla.org/releases/comm-aurora/rev/9a8111084ff8
Comment 10 Stefan Sitter 2012-09-23 03:37:55 PDT
Pushed to https://hg.mozilla.org/releases/comm-beta/rev/39fbe678702a

Note You need to log in before you can comment on or make changes to this bug.