1583595 - A dialog to handle opening .ics files

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement, P3)

Description

[Paul Morris [:pmorris]]

Implement a dialog window that appears when opening .ics files, e.g. when passing .ics files to Thunderbird via the command line, like so: thunderbird -file example.ics

The dialog should present some options:

• import the file's events into an existing calendar (or possibly a newly created calendar?)
• subscribe to the file (as a separate new calendar)

Ideally detect when the calendar add-on is not installed and at least point the user in the right direction to install it.

See comment 55 on bug 357480:

"First of all it would be helpful to even allow importing or subscribing to calendar files using the command line."

"There are two paths to consider when passing a file to Thunderbird/Lightning. It can either be imported (e.g. copied to an existing calendar or a new storage calendar), or it can be subscribed to (e.g using the ics provider, just subscribe to the file so that changes are being made directly in that file)."

"Maybe it would make sense to create an import dialog that explains and provides these two options. We could use it for bug 167255 too, although it doesn't have to be a wizard as mentioned there. When Thunderbird is started with the command line flag, then this dialog would show up and allow to select how to handle the file."

Comment 1

[Magnus Melin [:mkmelin] (away, back Aug 4)]

I think it would be useful to see upfront what event is in the ics file. And if there's many, you'd also want to know.

I'm not sure there's much of a case for subscribing to a file:// local ics file. Then the question is, do we ever get an http url?

Comment 2

[Paul Morris [:pmorris]]

Attachment: part1-refactor-import-export-0.patch

Refactor the import handling code to allow passing the file as an argument and skipping the file picker dialog. Then we can use it to open a given file from the command line (with the next patch).

Comment 3

[Paul Morris [:pmorris]]

Attachment: part2-wip-import-ics-files-command-line-0.patch

WIP - allows importing an ics file from the command line, for example, in the comm/ dir:

$ ../obj-artifact/dist/bin/thunderbird -file calendar/test/browser/data/import.ics

There's more to do (like the subscribe button does not do anything yet, and needing to handle error cases in the dialog, etc.) but I wanted to put this up for feedback on the general approach.

Comment 4

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #1)

I think it would be useful to see upfront what event is in the ics file. And if there's many, you'd also want to know.

That would be nice to have. I'd suggest adding that in a follow-up once the basic functionality is in place.

I'm not sure there's much of a case for subscribing to a file:// local ics file. Then the question is, do we ever get an http url?

Good thought. I imagine there are cases where you would want to either import or subscribe to a http url file, so we'd want to offer both options there. But yeah, if we can make it smarter without limiting functionality then let's do it, but I'd say let's get the basics working first.

Comment 5

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9115860 [details] [diff] [review]
part1-refactor-import-export-0.patch

Review of attachment 9115860 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/import-export.js
@@ +16,5 @@
>  var MODE_CREATE = 0x08;
>  var MODE_TRUNCATE = 0x20;
>  
>  /**
> + * Optionally shows a file picker dialog, reads and parses events from the

Loads events from a file. Optionally....

@@ +23,2 @@
>   *
> + * @param {calICalendar=} calendar - Optional, a calendar to import events into.

we've been using the JSDoc syntax for optional as
@param {calICalendar} [calendar] - A calendar to import events into.

@@ +28,2 @@
>   */
> +async function loadEventsFromFile(calendar, file, contractId) {

Not sure I see the point of passing around an import contractId. You'll be handling ics and csv. You'd just check the filename extension and use whatever needed at that point, no?

@@ +42,5 @@
> +
> +  let targetCalendar = calendar || (await getCalendarToImportInto());
> +
> +  if (targetCalendar) {
> +    // This await is needed for the mochitest to work.

probably not want the comment. the code is correct though

@@ +113,5 @@
> +  let importer = Cc[contractId].getService(Ci.calIImporter);
> +
> +  let inputStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(
> +    Ci.nsIFileInputStream
> +  );

Let's use something more js, like OS.File (see e.g. converterWorker.js)

Comment 6

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 9115864 [details] [diff] [review]
part2-wip-import-ics-files-command-line-0.patch

I think too early to give proper feedback here. It doesn't really work... at least for me. I couldn't get it to actually import or subscribe. The dialog comes up but the buttons do not work.

On a general note, we shouldn't add new dialogs using dtds, but move to Fluent for localization instead.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part1-refactor-import-export-1.patch

I've made the changes to the docstrings. We should document that so the conventions/expectations are clear and simple to follow.

Not sure I see the point of passing around an import contractId. You'll be
handling ics and csv. You'd just check the filename extension and use
whatever needed at that point, no?

In the existing code the output of the file picker dialog includes both the file and what the user selected for the file type. At the code level the latter is a contractId. In this patch I'm just refactoring the code so it will work for importing an ics file passed via the command line. Relying on the filename extension would change what the code does and also seems unnecessary since we already know what kind of file it is (and likely in a more robust/reliable way), from the file picker dialog. I'd rather pass another argument than recalculate the same thing again in a way that might diverge from the current behavior.

Let's use something more js, like OS.File (see e.g. converterWorker.js)

I looked into this, but it does not look that feasible because calIImporter has an importFromStream method so we need a nsIFileInputStream to pass to it. Like I said above, my goal with this refactor is limited to the scope of this bug. If we want to do more, let's do it in a different bug.

Comment 8

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Comment on attachment 9115864 [details] [diff] [review]
part2-wip-import-ics-files-command-line-0.patch

I think too early to give proper feedback here. It doesn't really work... at
least for me. I couldn't get it to actually import or subscribe. The dialog
comes up but the buttons do not work.

It works for me. Like I mentioned above, the "subscribe" button functionality is not yet implemented, but import should be functional and works for me.

I looked into the subscribe code a bit and it looks like it will be more involved to achieve than import, and import seems to be the primary use case. So I think the first step should be a dialog that just offers 'import' and then we can add subscribe in a follow-up.

Comment 9

[Geoff Lankow (:darktrojan)]

I haven't reviewed this yet because the test is disabled. Hoping to get some action on bug 1605674.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9117352 [details] [diff] [review]
part1-refactor-import-export-1.patch

Review of attachment 9117352 [details] [diff] [review]:
-----------------------------------------------------------------

This seems okay. It needs rebasing as it doesn't apply any more. Sorry about the delay, the test should be enabled again soon.

::: calendar/base/content/import-export.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* import-globals-from calendar-item-editing.js */
> +/* globals startBatchTransaction endBatchTransaction */

This is implied by the line above.

@@ +18,5 @@
>  
>  /**
> + * Loads events from a file. Optionally shows a file picker dialog, reads and
> + * parses events from the relevant file(s), optionally shows a file picker for
> + * a calendar, and adds the events to the relevant calendar.

This reads poorly. How about something like "If called without arguments, the user is asked to pick a file and a calendar to import into."

Consider swapping the arguments. It's a bit awkward with contractId but I think it reads better. (And I'm really hoping you're going to eliminate the need for contractId soon.)

@@ +117,5 @@
> +  let items = [];
> +  let exception;
> +
> +  try {
> +    inputStream.init(file, MODE_RDONLY, parseInt("0444", 8), {});

Octal numbers can be written as 0o444 in javascript.

@@ +155,5 @@
> +    .getCalendars()
> +    .filter(cal.acl.isCalendarWritable);
> +
> +  if (calendars.length == 1) {
> +    targetCalendar = calendars[0];

Just return early.

Comment 11

[Paul Morris [:pmorris]]

Attachment: part1-refactor-import-export-2.patch

1 of 2: Rebased, addressed review comments, improved a bit. No longer passing around a contractId, instead using file extension to detect file type.

Comment 12

[Paul Morris [:pmorris]]

Attachment: part2-import-ics-files-command-line-1.patch

2 of 2. Now using fluent for l10n and only offering import (not subscribe). We can add subscribe as an option in a follow-up if that makes sense (some UX discussion likely needed there). I wanted to start with import as that's the main use case. Simpler to offer this feature now that calendar is built in. To see it work, from the comm directory do:

$ ../obj-artifact/dist/bin/thunderbird -file calendar/test/browser/data/import.ics

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9139612 [details] [diff] [review]
part1-refactor-import-export-2.patch

Review of attachment 9139612 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/import-export.js
@@ +21,5 @@
>   *
> + * @param {nsIFile} [file] - Optional, a file to load events from.
> + * @param {calICalendar} [calendar] - Optional, a calendar to import events into.
> + * @return {boolean} True if the import succeeded, false if not (e.g. user
> + *                   canceled the file picker dialog).

If you're going to have this return value you should check it in the test.

@@ +34,5 @@
> +      return false;
> +    }
> +  }
> +
> +  let items = getItemsFromFile(file);

Only do this if we know we're going to need the items, as it's potentially expensive.

@@ +88,1 @@
>          reject();

Rejecting here raises an exception in the calling function, which you haven't handled. Also, how does this last case differ from the first, so that you reject here but resolve with no argument there?

(Side point: I don't think it's actually possible to be in this last case, other than if returnValue == Ci.nsIFilePicker.returnReplace which would be wrong anyway.)

I think this block should just be this:
if (returnValue == Ci.nsIFilePicker.returnCancel) {
  resolve();
}
resolve(picker.file);

@@ +96,5 @@
> + *
> + * @param {nsIFile} file - A file.
> + * @return {string} A contract ID.
> + */
> +function getContractIdForImportingFile(file) {

I have a dream that we can be smarter about this than just checking the extension, but it's good enough for now.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9139618 [details] [diff] [review]
part2-import-ics-files-command-line-1.patch

I need to run this code and see it before making a decision on it, and I'm not in a position to do so right now, but I have some general comments already:

Don't use the extra1 button for this. For a yes/no question like this you should use the accept and cancel buttons. If the user does click the accept button, immediately disable both buttons for the duration of the import, and for a minimum of, say, 1 second (a guess on my part) if the import is faster than that. Then change the label back to OK (or maybe Close) and enable it.

Comment 15

[Paul Morris [:pmorris]]

Attachment: part1-refactor-import-export-3.patch

Thanks for the review; I appreciate the helpful feedback. I've addressed the comments.

(In reply to Geoff Lankow (:darktrojan) from comment #13)

Rejecting here raises an exception in the calling function, which you
haven't handled. Also, how does this last case differ from the first, so
that you reject here but resolve with no argument there?

My thinking was to differentiate the "nothing went wrong, user just clicked cancel" case from the case where something went wrong and the file wasn't there when it really should have been. The previous code checked for that failure mode, so I assumed it was a potential problem. But I suppose we can trust the file picker to work as advertised.

(Side point: I don't think it's actually possible to be in this last case,
other than if returnValue == Ci.nsIFilePicker.returnReplace which would be
wrong anyway.)

I think this block should just be this:
if (returnValue == Ci.nsIFilePicker.returnCancel) {
resolve();
}
resolve(picker.file);

That works for me.

I have a dream that we can be smarter about this than just checking the
extension, but it's good enough for now.

Yeah, it seems like there should be a more robust way. (That's one reason I was originally holding on to the contractId from the dialog, thinking that was likely to be more reliable. But this does the job for now.)

Comment 16

[Paul Morris [:pmorris]]

Attachment: part2-import-ics-files-command-line-2.patch

(In reply to Geoff Lankow (:darktrojan) from comment #14)

Don't use the extra1 button for this. For a yes/no question like this you
should use the accept and cancel buttons. If the user does click the accept
button, immediately disable both buttons for the duration of the import, and
for a minimum of, say, 1 second (a guess on my part) if the import is faster
than that. Then change the label back to OK (or maybe Close) and enable it.

Makes sense, and done. As it happens, Promise.allSettled is really useful! (The extra1 button was partly left over from an earlier iteration that had both import and subscribe buttons.) I went with half a second since a full second seemed a little long when I was testing it out.

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9140232 [details] [diff] [review]
part2-import-ics-files-command-line-2.patch

Sorry to be a pain, but now I've seen this in action I don't like it.

Instead of creating a whole new dialog, let's just use the standard prompts. We don't have to put any effort into how they look or maintaining them. Unfortunately the downside is the prompt disappears while the import happens (only really a problem if it's an especially long process but that'd be rare).

I think the title of the prompt should be "Import Event" but the other strings look okay to me.

Here's the code for a confirm prompt, since I've already gone to the trouble of remembering how it works: (see nsIPromptService.idl for more explanation)
Services.prompt.confirmEx(null, title, text, Services.prompt.STD_YES_NO_BUTTONS, null, null, null, null, {});

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9140232 [details] [diff] [review]
part2-import-ics-files-command-line-2.patch

Review of attachment 9140232 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, you talked me into this given the dialog's future uses, but I still think it's ugly. :-)

The question should disappear when the success or failure message is shown. It's not relevant at that point.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.xhtml
@@ +11,5 @@
> +        xmlns:html="http://www.w3.org/1999/xhtml"
> +        onload="onLoad()"
> +        data-l10n-id="calendar-ics-file-window"
> +        minheight="200"
> +        minwidth="500">

I would lose the minimum height and make the minimum width smaller (commonDialog.xhtml is 29em for example, em units would be better because they change with the font size).

Comment 19

[Paul Morris [:pmorris]]

Thanks for the reviews and putting up with this ugly dialog. ;-) Just for context, on chat we discussed how:

"mkmelin has requested that in a future iteration it would show the user a preview of the events that will be imported before they click "import". Also, Fallen has discussed having it also handle 'subscribe' as well as 'import' options."

Another possibility would be to present the calendar selection part in this import dialog, so the user chooses a calendar and then clicks "import" rather than opening a second dialog for that as a separate step. Doing it that way probably makes sense in other places too, and then we wouldn't need the separate calendar selector dialog.

Comment 20

[Paul Morris [:pmorris]]

Attachment: part2-import-ics-files-command-line-3.patch

Okay, I've made the suggested changes (dropped minheight, used 29em for minwidth, the question disappears on success and failure).

The dialog is now sized to its content, even when the content changes/is added dynamically.

I also made the import button go away when there is an error. Seemed better to let the user start over in that case.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9140843 [details] [diff] [review]
part2-import-ics-files-command-line-3.patch

Review of attachment 9140843 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/dialogs/calendar-ics-file-dialog.xhtml
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +-->

Nit: the --> goes on the previous line.

Comment 22

[Paul Morris [:pmorris]]

Attachment: part2-import-ics-files-command-line-4.patch

Fixes the nit. Thanks for the review. Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=424e003589ce64598afacbdf383852e9abb22424

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26e623746758
Refactor import-export.js to support opening a given ics file. r=darktrojan
https://hg.mozilla.org/comm-central/rev/d751725dce3a
Allow import of ICS files via the command line. r=darktrojan DONTBUILD