Need a weekly print option

VERIFIED FIXED

Status

Calendar
Internal Components
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Joey Minta, Assigned: Joey Minta)

Tracking

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
 
(Assignee)

Comment 1

12 years ago
Created attachment 216606 [details] [diff] [review]
week-planner print option

This one prints a week view, much like what appears in many paper planners.
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #216606 - Flags: first-review?(mvl)
(Assignee)

Comment 2

12 years ago
Created attachment 216607 [details]
sample output

Sample output
(Assignee)

Updated

12 years ago
Attachment #216607 - Attachment mime type: application/octet-stream → text/html
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.
(Assignee)

Comment 4

12 years ago
Created attachment 216798 [details] [diff] [review]
week-planner print option v2

Now includes:
-checks for null category color preference
-explicit 50% column width
-right aligned events
Attachment #216606 - Attachment is obsolete: true
Attachment #216798 - Flags: first-review?(mvl)
Attachment #216606 - Flags: first-review?(mvl)
(Assignee)

Comment 5

12 years ago
*** Bug 322800 has been marked as a duplicate of this bug. ***
Created attachment 242904 [details] [diff] [review]
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.
Attachment #216798 - Attachment is obsolete: true
Attachment #242904 - Flags: second-review?(mvl)
Attachment #242904 - Flags: first-review?(cmtalbert)
Attachment #216798 - Flags: first-review?(mvl)
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");
>+        }
Attachment #242904 - Flags: second-review?(mvl) → second-review-
Created attachment 243422 [details] [diff] [review]
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.
Attachment #242904 - Attachment is obsolete: true
Attachment #243422 - Flags: second-review?(mvl)
Attachment #243422 - Flags: first-review?(cmtalbert)
Attachment #242904 - Flags: first-review?(cmtalbert)

Comment 9

12 years ago
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.
Attachment #243422 - Flags: first-review?(cmtalbert) → first-review+

Comment 10

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

Comment 11

12 years ago
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.
> 

(Assignee)

Comment 12

12 years ago
*** Bug 358137 has been marked as a duplicate of this bug. ***
(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)
Created attachment 243920 [details]
Print dialog with week view

Screenshot of week view print dialog
Created attachment 243921 [details]
Sample output

New and improved sample output
Attachment #216607 - Attachment is obsolete: true
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.
Created attachment 243993 [details] [diff] [review]
revised again

Refactored the html generating loop per mvl's suggestion, and removed the flat-out wrong comment
Attachment #243422 - Attachment is obsolete: true
Attachment #243993 - Flags: second-review?(mvl)
Attachment #243993 - Flags: first-review+
Attachment #243422 - Flags: second-review?(mvl)

Comment 18

12 years ago
(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 on attachment 243993 [details] [diff] [review]
revised again

r2=mvl
Attachment #243993 - Flags: second-review?(mvl) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(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. :)
Depends on: 358775

Updated

12 years ago
Whiteboard: [litmus testcase wanted]

Updated

12 years ago
Status: RESOLVED → VERIFIED
*** Bug 360097 has been marked as a duplicate of this bug. ***

Comment 23

11 years ago
Litmus testcase 3004 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.