Closed Bug 332063 Opened 14 years ago Closed 13 years ago

Need a weekly print option

Categories

(Calendar :: Internal Components, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jminta, Assigned: jminta)

References

Details

Attachments

(3 files, 5 obsolete files)

 
Attached patch week-planner print option (obsolete) — Splinter Review
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)
Attached file sample output (obsolete) —
Sample output
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.
Attached patch week-planner print option v2 (obsolete) — Splinter Review
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)
*** Bug 322800 has been marked as a duplicate of this bug. ***
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-
Attached patch revised per mvl's comments (obsolete) — Splinter Review
> > 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 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+
*** Bug 358137 has been marked as a duplicate of this bug. ***
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.
> 

*** 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)
Screenshot of week view print dialog
Attached image 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.
Attached patch revised againSplinter Review
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)
(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
Closed: 13 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
Whiteboard: [litmus testcase wanted]
Status: RESOLVED → VERIFIED
*** Bug 360097 has been marked as a duplicate of this bug. ***
Litmus testcase 3004 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.