325068 - Improve printing UI

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

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

the printing UI should be improved. It should allow to select a layout option, a daterange and maybe others.
(note that a layout option like 'week view' doesn't mean the daterange will be one  week. You should be able to print a few weeks)

Comment 1

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

Attachment: patch v1

Comment 2

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

Comment on attachment 210164 [details] [diff] [review]
patch v1

This patch does a few things:
- It changes the look of the print dialog
- It adds a way to pass in a daterange to the exporter.
- It connects the print dialog to this new export ability
- It renames the html exporter to htmllist, because there will be more html exporters later (week, month, etc)

Comment 3

[Joey Minta]

Comment on attachment 210164 [details] [diff] [review]
patch v1

How about a screen-shot?

Comment 4

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

Attachment: screenshot

Comment 5

[Stefan Sitter [:ssitter]]

Comment on attachment 210164 [details] [diff] [review]
patch v1

Could you remove the hard coded strings and make the dialog l10n friendly?

Comment 6

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

I rather have more comments on the rest of the patch before creating a new version.

Comment 7

[Joey Minta]

Comment on attachment 210164 [details] [diff] [review]
patch v1

+  /**
+   * Export the items into the stream.
+   * May assume that all the items are inside the given daterange.
+   */
+  void exportDateRangeToStream(in nsIOutputStream aStream,
+                               in calIDateTime aStart,
+                               in calIDateTime aEnd,
+                               in unsigned long aCount,
+                               [array, size_is(aCount)] in calIItemBase aItems);
 };

I'm really not clear on what purposes these arguments are supposed to perform and how the exporter is supposed to interpret them.  Are any of them optional?

Some scenarios:
1.) I choose case 'selected' (Print Selected Events).  This will send null for both start and end date.
2.) Is the exporter expected to retrieve events if it is passed a start and end date but no events in the list? Or is it supposed to export a blank grid?

More generally, are there other use-cases other than printing where exporting a specific date-range might be desired?  If not, then it might make sense to pursue a different strategy here, rather than trying to make exporters due something they weren't really designed to do.  However, there might be cases along the lines of browser integration.

I'm not really sure I like the requirement that this be included in the interface definition for normal exporters.  Some exporters, like csv, would find a date-range completely useless.  This seems to suggest that we need a third class, in addition to export/import.  The print-dialog could grab all registered implementations in this category, and offer their various layouts to the user.  Otherwise, I'm not clear how the print-dialog could distinguish between exporters than can print and exporters that can't.  This current patch requires that all possible printing options be html, which seems a bit restrictive.

Comment 8

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

The date ragne is needed for all exporters that want to export something intended to look at, instead of just containing the data. (svg, pdf, whatever)

The need of it: Imagine printing three months, jab, feb, mar. But you have no events in march yet. Then you expect an empty month to be printed. The exporter had no way of knowing which months to print just from looking at the events. So it needs the daterange to print the right empty pages.

We could create a new interface and/or category for this, sure.

Comment 9

[Joey Minta]

(In reply to comment #8)
> The date ragne is needed for all exporters that want to export something
> intended to look at, instead of just containing the data. (svg, pdf, whatever)
> 
> The need of it: Imagine printing three months, jab, feb, mar. But you have no
> events in march yet. Then you expect an empty month to be printed. The exporter
> had no way of knowing which months to print just from looking at the events. So
> it needs the daterange to print the right empty pages.

I understand the case you're trying to hit.  My point is that from just reading the code, it's not at all clear that this is what should happen.  The code you have in this patch in printDialog.js sometimes passes in a start/end and sometimes passes in null.  What's more, the exporter that it is passing to just ignores anything that is passed in.

We need to be clear:
(1) Is it acceptable to pass in null for start/end?
(2) Is it acceptable for the printer/exporter to disregard this information, if it is passed in?
(3) The docs should clearly state that the start/end dates are intended for use in the way you just described.

> 
> We could create a new interface and/or category for this, sure.
> 
I think this is the way to go.  It allows for extensions to add new printing layouts much more easily.

Comment 10

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

Attachment: patch v2

This patch is l10n friendly.
It also creates a new interface, just for printing. It (hopefully) has better documentation.

Comment 11

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

Attachment: patch v2, the missing files

patch v2 missed the new files. Here they are.

Comment 12

[Joey Minta]

Comment on attachment 214349 [details] [diff] [review]
patch v2

-            if (!comp.cid)
-                continue;
-            dump ("calItemModule: registering " + comp.contractid + "\n");
-            compMgr.registerFactoryLocation(comp.cid,
-                                            "",
-                                            comp.contractid,
-                                            fileSpec,
-                                            location,
-                                            type);
Why is this being moved?  It's going to throw off the order of the dump()s during registration (you'll see the 'registering for category stuff' message before you see the component name).  Also, it seems like the if (!comp.cid) check belongs where it is?

-  // set the date to the currently selected date
-  document.getElementById( "start-date-picker" ).value = gCalendarWindow.currentView.selectedDate;
-  document.getElementById( "end-date-picker" ).value = gCalendarWindow.currentView.selectedDate;
+    // set the date to the currently selected date
+    document.getElementById("start-date-picker").value = gCalendarWindow.currentView.selectedDate;
+    document.getElementById("end-date-picker").value = gCalendarWindow.currentView.selectedDate;
I'm all for normalizing style, but it seems like this should either be done file wide, or not at all.  Having mixed indentations between functions isn't good in my mind.

Also, can you update these to be window.opener.document...?

+    // Walk the list, adding item to the layout listbox
+    var layoutList = document.getElementById("layout-field");
Why are you adding to the layout-field and not to the menupopup?  Also, the comment should probably say 'menulist', not 'listbox'.

+          <hbox class="field-label-box-class" pack="end">
We had bugs filed on us last time we used pack="end" for violating HIG.  Leaving things aligned to the left is prefered, I believe.  (same below in Layout)

-<!ENTITY newevent.recurrence.day.label      "daily" >
-<!ENTITY newevent.recurrence.week.label     "weekly" >
-<!ENTITY newevent.recurrence.month.label    "monthly" >
-<!ENTITY newevent.recurrence.year.label     "annually" >
+<!ENTITY newevent.recurrence.day.label      "Daily" >
+<!ENTITY newevent.recurrence.week.label     "Weekly" >
+<!ENTITY newevent.recurrence.month.label    "Monthly" >
+<!ENTITY newevent.recurrence.year.label     "Annually" >
This looks like leftovers from another patch?

This part looks good.  r=jminta

Comment 13

[Joey Minta]

Comment on attachment 214350 [details] [diff] [review]
patch v2, the missing files

Index: base/public/calIPrintFormatter.idl
+ * Portions created by the Initial Developer are Copyright (C) 2005
2006.

+interface nsIInputStream;
I don't see any uses of nsIInputStream

Index: import-export/calListFormatter.js
+ * The Initial Developer of the Original Code is
+ * ArentJan Banck <[e-mail]>.
+ * Portions created by the Initial Developer are Copyright (C) 2002
+ * the Initial Developer. All Rights Reserved.
+ *
That doesn't look right.

+
+// Export
+
This comment doesn't belong here.

calListFormatter.prototype.getName =
+function list_getName() {
+    var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
+                        .getService(Components.interfaces.nsIStringBundleService);
+    var props = sbs.createBundle("chrome://calendar/locale/calendar.properties");
+    return props.GetStringFromName("formatListName");
+}
Should have a ; at the end, after the }.

+calListFormatter.prototype.formatToHtml =
+function list_formatToHtml(aStream, aStart, aEnd, aCount, aItems) {
+dump(aStart+" - "+aEnd+"\n");
+    htmlexporter = Components.classes["@mozilla.org/calendar/export;1?type=htmllist"]
+                             .createInstance(Components.interfaces.calIExporter);
+    htmlexporter.exportToStream(aStream, aCount, aItems);
Remove the dump, please.  Also, there's a strict warning here for undeclared htmlexporter.

This looks solid. r=jminta

Comment 14

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

patch checked in. (with review comments fixed)
Printing is now better pluggable :)