332063 - Need a weekly print option

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Comment 1

[Joey Minta]

Attachment: week-planner print option

This one prints a week view, much like what appears in many paper planners.

Comment 2

[Joey Minta]

Attachment: sample output

Sample output

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

This looks really cool.
A few comments though:

the sample output looks weird in week 12. The left column is too small, maybe because it is empty. I also saw that problem in my tests.

When i have a few items on a day, they end up centered. It doesn't show in your sample layout, because the two items have the same textual length. But try it with  one with a short and one with a long description

I get a warning: [JavaScript Warning: "Expected color but found 'undefined'.  Expected color but found 'undefined'.  Expected end of value for property but found 'undefined'.  Error in parsing value for property 'border'.  Declaration dropped." {file: "file:///tmp/calendarPrint.html" line: 0}] Maybe because i have a calendar without a color set.

Comment 4

[Joey Minta]

Attachment: week-planner print option v2

Now includes:
-checks for null category color preference
-explicit 50% column width
-right aligned events

Comment 5

[Joey Minta]

*** Bug 322800 has been marked as a duplicate of this bug. ***

Comment 6

[Matthew (lilmatt) Willis]

Attachment: unbitrotted and slightly updated version of v2

This was supposed to make 0.3.

Now uses calGetString where it can, and has locales in the correct place.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 242904 [details] [diff] [review]
unbitrotted and slightly updated version of v2

>Index: calendar/import-export/calImportExportModule.js
>      service: false},
>+
>     {cid: Components.ID("{a6a524ce-adff-4a0f-bb7d-d1aaad4adc60}"),

Don't do uneeded white-space changes. (those two blocks were together for a reason: they are the importer and exporter for one type)


>Index: calendar/import-export/calWeekPrinter.js

>+calWeekPrinter.prototype.formatToHtml =
>+function weekPrint_format(aStream, aStart, aEnd, aCount, aItems, aTitle) {

General comment: This function needs a lot of comments. It's really not all that readable.



>+calWeekPrinter.prototype.getDayTd =
>+function weekPrint_getDayTable(date, aItems) {

>+        // end dates are exclusive
>+        if (sDate && sDate.isDate && eDate) {
>+            eDate = eDate.clone();
>+            eDate.day -= 1;
>+            eDate.normalize();
>+        }

I think you want this conversion after you do comparisation you do below. You want to compare the original dates.

>+        if (!eDate || eDate.compare(date) == -1) {
>+            continue;
>+        }

Just compare for < 0. Actually, compare for <= 0, because end-date is exclusive.

>+        if (!sDate || sDate.compare(date) == 1) {
>+            break;
>+        }

Compare for >0.

>+        function getStringForDate(date) {
>+            if (!date.isDate) {
>+                return dateFormatter.formatTime(sDate);
>+            }
>+            var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                                .getService(Components.interfaces.nsIStringBundleService);
>+            var props = sbs.createBundle("chrome://calendar/locale/dateFormat.properties");
You really want to cache those two things.

>+            return props.GetStringFromName("AllDay");
>+        }

Comment 8

[Matthew (lilmatt) Willis]

Attachment: revised per mvl's comments

> > I think you want this conversion after the comparision you do below. You?>
> > want to compare the original dates.

> >+        if (!eDate || eDate.compare(date) == -1) {
> >+            continue;
> >+        }
> Just compare for < 0. Actually, compare for <= 0, because end-date is
> exclusive.

Leaving the order alone and simply comparing for < 0 works correctly. Moving the comparisons before modifying the eDate (and comparing for <= 0) doesn't work correctly.

I've updated the patch in the spirit of your comment, but in a way that still works.

Comment 9

[cmtalbert]

Comment on attachment 243422 [details] [diff] [review]
revised per mvl's comments

> Shouldn't this compareItems function live somewhere where it will be generally available? It seems like a good piece of code that we could use in many different situations. Perhaps that should live in our calUtils.js?
>+
>+    var calIEvent = Components.interfaces.calIEvent;
>+    var calITodo = Components.interfaces.calITodo
>+    function compareItems(a, b) {
>+        // Sort tasks before events
>+        if (a instanceof calIEvent && b instanceof calITodo) {
>+            return 1;
>+        }
>+        if (a instanceof calITodo && b instanceof calIEvent) {
>+            return -1;
>+        }
>+        if (a instanceof calIEvent) {
>+            var startCompare = a.startDate.compare(b.startDate);
>+            if (startCompare != 0) {
>+                return startCompare;
>+            }
>+            return a.endDate.compare(b.endDate);
>+        }
>+        var dateA = a.entryDate || a.dueDate;
>+        var dateB = b.entryDate || b.dueDate;
>+        return dateA.compare(dateB);
>+    }

...snip...
The comment below needs to be above the prototype statement. Breaking the prototype away from the function declaration is rather confusing when reading the code (or at least it was to me).
>+
>+calWeekPrinter.prototype.getDayTd =
>+/**
>+ * Given a calIDateTime and an array of items, this function creates an HTML
>+ * table containing the items, using the appropriate formatting and colours.
>+ */
>+function weekPrint_getDayTable(aDate, aItems) {
>+    // mainTd is the <td> element from the parent HTML table that will hold
>+    // the child HTML tables containing the date string and this day's items.

R+ with those two nits. Otherwise, it looks really good. I built it and managed to print a week planner. Nice work.

Comment 10

[Sebastian Schwieger]

*** Bug 358137 has been marked as a duplicate of this bug. ***

Comment 11

[spnorth]

How can I modify Calendar 0.4a1 with this code?

(In reply to comment #9)
> (From update of attachment 243422 [details] [diff] [review] [edit])
> > Shouldn't this compareItems function live somewhere where it will be generally available? It seems like a good piece of code that we could use in many different situations. Perhaps that should live in our calUtils.js?
> >+
> >+    var calIEvent = Components.interfaces.calIEvent;
> >+    var calITodo = Components.interfaces.calITodo
> >+    function compareItems(a, b) {
> >+        // Sort tasks before events
> >+        if (a instanceof calIEvent && b instanceof calITodo) {
> >+            return 1;
> >+        }
> >+        if (a instanceof calITodo && b instanceof calIEvent) {
> >+            return -1;
> >+        }
> >+        if (a instanceof calIEvent) {
> >+            var startCompare = a.startDate.compare(b.startDate);
> >+            if (startCompare != 0) {
> >+                return startCompare;
> >+            }
> >+            return a.endDate.compare(b.endDate);
> >+        }
> >+        var dateA = a.entryDate || a.dueDate;
> >+        var dateB = b.entryDate || b.dueDate;
> >+        return dateA.compare(dateB);
> >+    }
> 
> ...snip...
> The comment below needs to be above the prototype statement. Breaking the
> prototype away from the function declaration is rather confusing when reading
> the code (or at least it was to me).
> >+
> >+calWeekPrinter.prototype.getDayTd =
> >+/**
> >+ * Given a calIDateTime and an array of items, this function creates an HTML
> >+ * table containing the items, using the appropriate formatting and colours.
> >+ */
> >+function weekPrint_getDayTable(aDate, aItems) {
> >+    // mainTd is the <td> element from the parent HTML table that will hold
> >+    // the child HTML tables containing the date string and this day's items.
> 
> R+ with those two nits. Otherwise, it looks really good. I built it and managed
> to print a week planner. Nice work.
> 

Comment 12

[Joey Minta]

*** Bug 358137 has been marked as a duplicate of this bug. ***

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #11)
> How can I modify Calendar 0.4a1 with this code?

You don't. You just wait patiently until the patch is checked in, and nightly builds are available.
(also, don't quote entire comments. that just adds noise, and reduces the odds of the bug ever getting fixed)

Comment 14

[Matthew (lilmatt) Willis]

Attachment: Print dialog with week view

Screenshot of week view print dialog

Comment 15

[Matthew (lilmatt) Willis]

Attachment: Sample output

New and improved sample output

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 243422 [details] [diff] [review]
revised per mvl's comments

>+        var mainWeek = <table width='100%' height="90%" border='solid 1px;'/>
>+        date.day += 1; // Move to Monday
>+        date.normalize();
>+        var MHrow = <tr height="33%"/>;
>+        MHrow.appendChild(this.getDayTd(date, sortedList));
>+        date.day += 3; // Move to Thursday
>+        date.normalize();
>+        MHrow.appendChild(this.getDayTd(date, sortedList));
>+        mainWeek.appendChild(MHrow);

TO make this section more readable, you can write it somewhat different. First, in a loop, get a html for each day.
Then put each of those html sections into one big table. 
>+        function getStringForDate(aDate) {
>+            if (!aDate.isDate) {
>+                return dateFormatter.formatTime(aDate);
>+            }
>+
>+            // aDate isn't a date. Therefore it must be "all day".
That comment seems wrong. aDate is a date.

Comment 17

[Matthew (lilmatt) Willis]

Attachment: revised again

Refactored the html generating loop per mvl's suggestion, and removed the flat-out wrong comment

Comment 18

[spnorth]

(In reply to comment #13)
> (In reply to comment #11)
> > How can I modify Calendar 0.4a1 with this code?
> 
Sorry about quoting all of the code.  The reason I asked is because I'm a beginning programmer, and I really didn't know what file you are modifying to make more print options available. 

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 243993 [details] [diff] [review]
revised again

r2=mvl

Comment 20

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 21

[Matthew (lilmatt) Willis]

(In reply to comment #18)
> Sorry about quoting all of the code.  The reason I asked is because I'm a
> beginning programmer, and I really didn't know what file you are modifying to
> make more print options available. 

Each print option implements the calIPrintFormatter interface, and is a subscript, loaded by calImportExportModule.js.

If you have more questions, I suggest posting them to the newsgroup (mozilla.dev.apps.calendar) rather than putting them in this (now) closed bug. :)

Comment 22

[Simon Paquet [:sipaq]]

*** Bug 360097 has been marked as a duplicate of this bug. ***

Comment 23

[Damian Szczepanik]

Litmus testcase 3004 created