373562 - All day events should be printed at the top

Status: RESOLVED FIXED

(Calendar :: Printing, defect)

Description

[Damian Szczepanik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070308 Calendar/0.6a1

Bug 349960 has been verified for displaying but the problem still exists for printing

Reproducible: Always

Steps to Reproduce:
1. Create event, set from and to time
2. Create second event: the same date, mark as "All day"
3. Print it, use monthly or weekly view
Actual Results:  
All day event is not printed first

Expected Results:  
All day events should be printed at the top as in bug 349960

Comment 1

[Ferdinand Grassmann]

Damian,

I know this bug, and it is one of the reasons I developed my printer extension. As long as it is not fixed in Calendar, I would encourage you to use the extention. You can download it from: http://www.grassmann.info/fgPrinters.xpi

As for the actual fix in the source:

I found that adding the following lines to compareItems in calMonthGridPrinters.js and calWeekPrinter.js will do the trick:


        if (a instanceof calIEvent) {
			if (a.startDate.isDate && b.startDate.isDate)
			{
				return a.startDate.compare(b.startDate);
			}
			if (a.startDate.isDate)
			{
				var tmpd = b.startDate.clone();
				tmpd.isDate = true;
				tmpd.normalize();
				if (a.startDate.compare(tmpd) == 0) return -1;
			}
			if (b.startDate.isDate)
			{
				var tmpd = a.startDate.clone();
				tmpd.isDate = true;
				tmpd.normalize();
				if (b.startDate.compare(tmpd) == 0) return 1;
			}
        }

This should be placed right before the "if (a instanceof calIEvent) {"-line.

Comment 2

[Ed Davis]

I have only done a little testing but at first glance the patch for bug 551814 seems to fix this problem too.

Comment 3

[Aleksandar]

Yes it does fix it. I have noticed this problem while printing "All day" events and regular events. This fix is included in bug fix https://bugzilla.mozilla.org/show_bug.cgi?id=551814

Comment 4

[Ritesh]

Attachment: calender

Comment 5

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

Hi Ritesh, have you tried this code in Lightning? I'd appreciate if you could put this together as a patch, feel free to email me for details.

Comment 6

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

Ritesh, are you interested in bringing this into core Lightning? I would very much appreciate, please contact me for details.

Comment 7

[Merouane Atig]

I'm interested in fixing this behavior. I already have a working development environment for Thunderbird+Lightning.

I'm new to contributing to Mozilla projects and I still struggle with the architecture of Thunderbird. It seems to me that the fix suggested in Comment 1 could be implemented in calPrintUtils.jsm comparePrintItems method but I'm not sure.

Could someone give me some directions?

Comment 8

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

Hey Merouane, I'm happy to hear you'd like to fix this issue! comparePrintItems looks like the right method to fix this in. I'd suggest testing it there and if it works and you are still motivated, maybe you could do some refactoring so that both the calendar views and the print view shares the same method to compare items.

There is a comparator here for example, and there are surely also other comparators for the other views.

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

For simplicity you could put the common method in calUtils.jsm, but eventually we probably want to move this to calViewUtils.jsm (bug 409950).

If you'd prefer to only do it in the print view for now that is fine with me too.

If you'd like ad-hoc help, please find me on irc.mozilla.org #calendar, my nickname is Fallen. You can also email me if you like.

Comment 9

[Merouane Atig]

Attachment: bug_373562.patch

I am not sure I factorized all items comparators into one. I searched for others but I did not find any.

Comment 10

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

Comment on attachment 8505021 [details] [diff] [review]
bug_373562.patch

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

This looks great! Could you upload a new patch that has the following comments fixed, and preferably with a commit message like:

"Fix bug 373562 - All day events should be printed at the top. r=philipp"

::: calendar/base/modules/calViewUtils.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource://calendar/modules/calUtils.jsm");
> +
> +EXPORTED_SYMBOLS = ["calView"];

I'd suggest calling it cal.view and then just exporting cal, just like in the other files.

@@ +14,5 @@
> +    - * @param b     The second item
> +    - * @return      The usual -1, 0, 1
> +    - */
> +  compareItems: function(a, b) {
> +    if (!a) return -1;

We use 4 space indent in all calendar js files.

::: calendar/test/unit/test_view_utils.js
@@ +10,5 @@
> +  test_compare_startdate();
> +  test_compare_enddate();
> +  test_compare_alldayevent();
> +  test_compare_title();
> +  test_compare_todo();

Thank you so much for writing a test for this! We have far too few of them :-)

Comment 11

[Merouane Atig]

Attachment: bug_373562_v2.patch

Comment 12

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

Comment on attachment 8510606 [details] [diff] [review]
bug_373562_v2.patch

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

Sorry for the delay, I could have done this much earlier since its just a followup from the previous review and I don't really need to take a deep look. I'm going to set checkin-needed so this can be pushed as soon as the tree is open again.

Comment 13

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/01cf9234f2fb