Closed Bug 325068 Opened 18 years ago Closed 18 years ago

Improve printing UI

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Assigned: mvl)

Details

Attachments

(3 files, 1 obsolete file)

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)
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → mvl
Status: NEW → ASSIGNED
Attachment #210164 - Flags: first-review?(jminta)
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 on attachment 210164 [details] [diff] [review]
patch v1

How about a screen-shot?
Attached image screenshot
Comment on attachment 210164 [details] [diff] [review]
patch v1

Could you remove the hard coded strings and make the dialog l10n friendly?
I rather have more comments on the rest of the patch before creating a new version.
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.
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.
(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.
Attached patch patch v2Splinter Review
This patch is l10n friendly.
It also creates a new interface, just for printing. It (hopefully) has better documentation.
Attachment #210164 - Attachment is obsolete: true
Attachment #214349 - Flags: first-review?(jminta)
Attachment #210164 - Flags: first-review?(jminta)
patch v2 missed the new files. Here they are.
Attachment #214350 - Flags: first-review?(jminta)
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
Attachment #214349 - Flags: first-review?(jminta) → first-review+
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 <ajbanck@planet.nl>.
+ * 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
Attachment #214350 - Flags: first-review?(jminta) → first-review+
patch checked in. (with review comments fixed)
Printing is now better pluggable :)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.