Closed Bug 1631902 Opened 4 months ago Closed 2 months ago

Add event preview to ICS import dialog

Categories

(Calendar :: Dialogs, enhancement)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

(Whiteboard: [for78beta])

Attachments

(9 files, 3 obsolete files)

55.73 KB, image/png
Details
62.17 KB, image/png
Details
53.76 KB, image/png
Details
2.24 KB, text/calendar
Details
11.59 KB, patch
Details | Diff | Splinter Review
67.86 KB, image/png
Details
58.38 KB, image/png
Details
27.25 KB, patch
pmorris
: review+
Details | Diff | Splinter Review
14.39 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review

As discussed in bug 1583595, we want to show the user a preview of the events in an ICS file before they import it, as part of the ICS file import dialog.

Since an ICS file might have a lot of events, we'll have to handle that case. Indicate the total number of events. Maybe just show the first n events? (5? 10?) And maybe then allow to show the next n events, or something similar.

I think in reality it would mostly be just one event. If there are many, I think the events should just be in a scrollable area.

Below are the kinds of data for a given event or task that we could show in this preview. I was chatting with Alex about how it might make sense to have a shared/standard way to create such a preview. For example, it might be used for this import preview and also for the new "click to preview" feature in bug 1575195 (and maybe also eventually for calendar summary dialog?).

The first step is decide what data we are showing. The tricky case for import preview is how to show repeating events. Probably simplest would be to just include the repeat info, like in the summary dialog: "Repeats weekly until March 15, 2021".

BASIC/MINIMAL DATA TO INCLUDE

  • title
  • event/task start date/time
  • event/task end/due date/time
  • location
  • repeat details (daily, weekly, monthly, custom, etc.)

MAYBE ALSO INCLUDE

  • description
  • organizer
  • attendees
  • attachments
  • url link

PROBABLY NOT NEEDED FOR IMPORT PREVIEW

  • status (tentative, confirmed, cancelled, needs-action, in-process, completed)

NOT NEEDED FOR IMPORT PREVIEW

  • calendar
  • category
  • reminder details

The calendar summary dialog is a good resource where we're already doing this kind of thing.

Thanks for the detailed list of data we need to handle.
Some follow up questions.

Do we think it makes sense trying to have a shared and modular template we can include whenever we want to show this data? Removing the necessity of reinventing the wheel whenever we interact with calendar events?

Do we want to offer the ability to edit this data before it gets saved? Is editing data coming from an ICS "a thing" the user expects?

NOT NEEDED FOR IMPORT PREVIEW
calendar
category
reminder details

Shouldn't the user be able to decide where the events imported from the ICS should go?

I would think showing what we have in calendar-summary-dialog.xhtml is a good first step. Doesn't seem like anything should be left out. At least that would be consistent. We can improve how that's shown in a further step.

(In reply to Alessandro Castellani (:aleca) from comment #3)

Do we think it makes sense trying to have a shared and modular template we can include whenever we want to show this data? Removing the necessity of reinventing the wheel whenever we interact with calendar events?

The cases seem similar enough that it might make sense. At least there could be shared code for serializing calendar item data into strings to show in one or more templates. I've started working on that by refactoring the calendar summary dialog code.

Do we want to offer the ability to edit this data before it gets saved? Is editing data coming from an ICS "a thing" the user expects?

I don't think users expect that, and it's probably simpler not to offer it.

But it might make sense for things a user might add to an item like 'reminders' and 'categories'. Also maybe 'calendar' (see below). Or maybe offer a way to "import and edit" to import an event and then immediately open it for editing.

Shouldn't the user be able to decide where the events imported from the ICS should go?

Currently the user clicks the "import" button and then sees a dialog to select a calendar for the import. It would be nicer to not have a separate dialog window for this, and allow picking a calendar inline.

Per Magnus we want to add the ability to import events individually, like an "import" button for each event (and then have an "Import All" button). Or have a checkbox you can uncheck to omit an event from the import.

Currently we import all the events into a single calendar. Do we want to allow importing different events into different calendars?

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

I would think showing what we have in calendar-summary-dialog.xhtml is a good first step. Doesn't seem like anything should be left out. At least that would be consistent. We can improve how that's shown in a further step.

Makes sense. I'll start with showing everything we show in the summary dialog and we can iterate from there.

(In reply to Paul Morris [:pmorris] from comment #5)

Currently we import all the events into a single calendar. Do we want to allow importing different events into different calendars?

That's probably not common.

Re editing, I think that is something one could often want. Events are often authored in the perspective of the other user, say "Birthday party", but when you add it you might want to add who's party it is.

I agree with Magnus's direction.

Currently the user clicks the "import" button and then sees a dialog to select a calendar for the import. It would be nicer to not have a separate dialog window for this, and allow picking a calendar inline.

Indeed, we should avoid all these dialogs one after another. We should offer what's editable and selectable directly in the single import dialog.

Re editing, I think that is something one could often want. Events are often authored in the perspective of the other user, say "Birthday party", but when you add it you might want to add who's party it is.

I also agree with this point.
Maybe we should allow editing a few fields, for example just the title, if that doesn't add any major complexity to this dialog.
Furthermore, if we plan to use the same template for everything, disabling/enabling fields accordingly based on what we want to allow editing might be a possible solution.
A second round of UI improvements can deal with making those fields more or less obvious based on their status.

Okay, since what we want for the import dialog is basically the same as the main part of the summary dialog, I think it makes sense to pull that out into a custom element that displays a summary of an item, then we can use it in the summary dialog and to display multiple items in the import dialog.

The summary dialog uses inputs that are readonly by default, so it should be feasible to make some of them editable, as needed. For example, currently the reminders are editable on an otherwise read-only invitation (in the summary dialog).

So here's how this work might go, maybe split into more than one bug to keep it more incremental and manageable:

  1. Move part of the summary dialog into a custom element and use it for the summary dialog.
  2. Use the custom element to preview items in the import dialog. Probably read-only for this first pass.
  3. Make some parts of imported items editable in the import dialog.
  4. Move calendar selection into the import dialog.
  5. UI improvements and polish.
Depends on: 1637133
Summary: Add event preview to ICS import dialog → [meta] Add event preview to ICS import dialog

Building on top of the work in bug 1637133, I now have a scrollable list of items displayed in the ICS file import dialog. Each appears like it does in the item summary dialog (this will need improvement). The "Import this Item" buttons are not yet functional. I realized we need to select the calendar first before importing any items. So here's the current plan:

  1. DONE: Move part of the summary dialog into a custom element and use it for the summary dialog.
  2. DONE: Use the custom element to preview items in the import dialog.
  3. Add a calendar drop down menu for selecting the calendar (instead of a separate dialog).
  4. Allow importing individual items (one "import" button per item, plus an "import all" button).
  5. Make some properties of items editable in this dialog.
    (The summary dialog has UI for setting reminders, so that can be re-used.)
  6. UI improvements and polish.

Quick update on current status:

  1. DONE: Move part of the summary dialog into a custom element and use it for the summary dialog.
  2. DONE: Use the custom element to preview items in the import dialog.
  3. DONE: Add a calendar drop down menu for selecting the calendar (instead of a separate dialog).
  4. DONE: Allow importing individual items (one "import" button per item, plus an "import all" button).
  5. Prevent the "Import All" button from importing any previously imported items a second time.
  6. UI improvements and polish.

Per mkmelin we are now deferring "Make some properties of items editable in this dialog" for now to get this landed sooner. Will address that in a follow-up.

  • Show a summary of each event and task in the ICS file that's being imported in the ICS file import dialog.
  • Allow each event or task to be imported individually (in addition to import of the whole file).

Something I just realized when testing this is that our ICS import code doesn't import tasks only events. So we should either 1. make it import tasks, or 2. change the wording of this dialog so it doesn't claim that tasks can be imported. I'd say 1 is preferable. If we do 1, that could be a follow-up. If 2 then we should do it as part of this patch to avoid unnecessary string churn. Edit: the tasks are imported, I just hadn't looked for them hard enough.

Richard, flagging you for feedback on CSS changes. Namely, now that we are using the "calendar summary dialog" CSS for this as well, maybe some re-organization is in order? (I'm sure the aesthetics can be improved as well, but we could do that as a follow-up, when there's time, what with everything needing to be done before the 78 release.) I'll upload some screenshots.

Attachment #9150881 - Flags: review?(geoff)
Attachment #9150881 - Flags: feedback?(richard.marti)
Attached image ics-import-dialog-1.png

As you can see I've hidden the section headings ("General", "Description", "Attendees" "Link"), otherwise things should look the same as the "calendar summary dialog".

I noticed that reminders/alarms are not being shown. In the summary dialog these are editable via drop down menu. I'd say let's tackle showing them, and possibly editing them in this dialog, in a follow up.

Attached image ics-import-dialog-2.png

Just another screenshot.

Here's an ICS file that I've been using for some manual testing. Put it in the comm dir then launch TB something like so:
$ ../obj-artifact/dist/bin/thunderbird -file example-for-testing.ics

Comment on attachment 9150883 [details]
ics-import-dialog-1.png

Should the scrollable centre section go right to the edges of the window and/or be a lighter colour than the rest of the window? Or if not going to the edges, have a border on all four edges? That would make what is happening a bit more obvious, I think, especially if the OS theme has a hidden scrollbar effect.
Comment on attachment 9150881 [details] [diff] [review]
ics-file-import-dialog-improvements-0.patch

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

On the whole this is alright, but there's some work-ons. Also the styling could probably be better but I'll leave that for Richard to comment on.

Here's some things I discovered that you probably haven't seen:
* I accidentally passed in a file that wasn't ICS. I got an error message saying as much, and then the import dialog with no events. Not sure what that's about.
* Events in non-local time zones have very long labels because they display the time in event time and local time. These labels fall off the side.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.js
@@ +28,5 @@
>    // can await it and then resize the window to fit its content.
> +  let name = gModel.file.leafName;
> +  let maxLength = 50;
> +
> +  let fileName = name.length > maxLength ? name.slice(0, maxLength - 3) + "..." : name;

Don't do this. Have the label wrap if it's too long.

(FYI: If you were to do this I'd be asking you to use \u2026 instead of ...)

@@ +224,5 @@
> +  let errorMessageElement = document.getElementById("calendar-ics-file-dialog-error-message");
> +  errorMessageElement.value = errorMessages.join(" ");
> +
> +  let dialog = document.getElementsByTagName("dialog")[0];
> +

You love empty lines, don't you? :-P

::: calendar/locales/en-US/calendar/calendar-ics-file-dialog.ftl
@@ +14,5 @@
>  calendar-ics-file-accept-button-ok-label = OK
>  
>  # $fileName (string) - The name of the file.
> +calendar-ics-file-dialog-message-2 = Import calendar events and tasks from the file "{ $fileName }".
> +calendar-ics-file-dialog-calendar-menu-label = Select a calendar to import into.

As a label for a widget this should end with a colon instead of a dot.
Attachment #9150881 - Flags: review?(geoff)
Comment on attachment 9150881 [details] [diff] [review]
ics-file-import-dialog-improvements-0.patch

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

Unfortunately this dialog shows only on startup and I can't inspect it. I hope this dialog is later also shown when TB already runs.

The scrollbar issue (not at the edge of the dialog comes from the padding here: https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/toolkit/themes/windows/global/dialog.css#15. Mac has 14px.
You need to set `#calendar-ics-file-dialog: padding-inline: 0;` and then on the two <vbox> the needed padding-inline. And the dialog-button-box needs then the padding too, but this could be a bit tricky because its in the shadow-DOM.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.css
@@ +7,5 @@
>  }
> +
> +#calendar-ics-file-dialog-items-container {
> +  border-top: 1px solid #ccc;
> +  border-bottom: 1px solid #ccc;

Instead of border-top and border-bottom you could use `border-block: 1px solid #ccc;`

@@ +18,5 @@
> +.calendar-ics-file-dialog-item-frame {
> +  background-color: #fff;
> +  border: 1px solid #ccc;
> +  margin: 1em 0;
> +  padding: 1em 0;

Do you only need the top/bottom margin/padding and added 0 to make it simple? Then use `margin-block: 1em;` and `padding-block: 1em;`.
Attachment #9150881 - Flags: feedback?(richard.marti) → feedback+

1 of 3. Thanks for the reviews. This addresses most of the review comments, with exceptions covered in my comments on the next patches. I also put some additional effort into improving the styling in general, and I think it's looking better overall.

Attachment #9150881 - Attachment is obsolete: true
Attachment #9151384 - Flags: ui-review?(richard.marti)
Attachment #9151384 - Flags: review?(geoff)

2 of 3. Go ahead and use this new dialog when importing a file from within Thunderbird, like when the user selects "Import" from the "Events and Tasks" menu. To provide a more consistent UX.

Attachment #9151385 - Flags: ui-review?(richard.marti)
Attachment #9151385 - Flags: review?(geoff)

3 of 3. WIP on solving this:

Events in non-local time zones have very long labels because they display the time in event time and local time. These labels fall off the side.

This is an existing issue with our event/task summary dialog. We are using read only text inputs which don't wrap, and we don't set the dialog width to fit the contents, which is difficult to do because of the text inputs.

In this patch I switch to using divs so the text will wrap and it should be easier to set the width to fit the content when that makes sense (not too too wide). I haven't attempted the automatic width setting yet. I ran into various layout issues that need fixing to get the "label" text in the left hand column to align vertically with the text in the right hand column, when wrapping and not wrapping, etc. Also the left edge of the text in the right hand column doesn't always align with the text above and below, etc.

Before digging deeper I want to make sure this is what we want to do. Also, given that string freeze is soon, I think it makes sense to defer this to a follow-up since it doesn't involve strings.

Attachment #9151389 - Flags: feedback?(geoff)

Updated screenshot. (Without the WIP 3rd patch.) The dialog is not this tall by default. I resized it to show more in a single screenshot.

Besides the issue in my previous comment, the main thing I know that needs addressing is that reminders/alarms are not currently being shown. I haven't had a chance to really look into this yet. I'd prefer to tackle this in a follow-up.

Also:

  1. tests
  2. allow editing the item title in this dialog (for a follow-up).
Comment on attachment 9151384 [details] [diff] [review]
part1-ics-file-import-dialog-improvements-1.patch

Looks good now. Thanks!
Attachment #9151384 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9151385 [details] [diff] [review]
part2-use-ics-file-import-dialog-everywhere-0.patch

As a little stress test I imported a holiday ICS file. TB was for around 10 seconds not usable with freezing or very laggy UI. Then cancelling the dialog was also not immediately with a delay of 2-3 seconds.

When I import some or all events, they are imported but not shown in the events list. I have to change the filter menulist from "All Events" to another item to let update the list.

I still give the ui-r+ in the thinking this is because of the non-e10s nature of TB and that the list issue isn't an issue from this patch. But maybe you can improve it somehow.
Attachment #9151385 - Flags: ui-review?(richard.marti) → ui-review+

(In reply to Richard Marti (:Paenglab) from comment #23)

As a little stress test I imported a holiday ICS file. TB was for around 10
seconds not usable with freezing or very laggy UI. Then cancelling the
dialog was also not immediately with a delay of 2-3 seconds.

Good call to test this. I'd be curious to see how this compares with the previous import without the dialog. I would think it would be similar.

When I import some or all events, they are imported but not shown in the
events list. I have to change the filter menulist from "All Events" to
another item to let update the list.

I've noticed this as well, where the events list is not updated. I'd be curious whether this was a problem before changing to the dialog. It should be fixed in any case of course.

I still give the ui-r+ in the thinking this is because of the non-e10s
nature of TB and that the list issue isn't an issue from this patch. But
maybe you can improve it somehow.

Makes sense, thanks.

Magnus says that one reason we're using read-only html inputs in the summary dialog is to allow selecting the text and copying it with right-click, etc. There should be a way to do this with divs using contemporary CSS, which is how we're doing it for email messages (subject, etc.) in the 3-pane mail view.
Edit: see https://searchfox.org/comm-central/source/mail/themes/shared/mail/messageHeader.css#341-345

Status: NEW → ASSIGNED
Comment on attachment 9151384 [details] [diff] [review]
part1-ics-file-import-dialog-improvements-1.patch

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

::: calendar/base/content/dialogs/calendar-ics-file-dialog.css
@@ +12,5 @@
> +}
> +
> +#calendar-ics-file-dialog-header,
> +#calendar-ics-file-dialog-items-container {
> +  /* See note above. This padding needs to change elsewhere if it changes here. */

Say where.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.js
@@ +24,5 @@
> +async function onWindowLoad() {
> +  // Workaround to add padding to the dialog buttons area which is in shadow dom.
> +  // If the padding value changes here it should also change in the CSS.
> +  let dialog = document.getElementsByTagName("dialog")[0];
> +  dialog.getButton("accept").closest(".dialog-button-box").style = "padding-inline: 10px;";

I think `dialog.shadowRoot.querySelector(".dialog-button-box")` should do the trick.
Attachment #9151384 - Flags: review?(geoff) → review+
Attachment #9151385 - Flags: review?(geoff) → review+
Comment on attachment 9151389 [details] [diff] [review]
part3-wip-wrap-table-cell-contents-in-item-summaries-0.patch

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

I haven't tested this yet because I'm in the middle of a build, but the code looks good to me.

Unfortunately now the table is overflowing in the other direction, I assume because XUL has never been very good at handling wrapping text. The table height changes as you resize the window but the space allocated for it doesn't. That's rather annoying. Maybe you can do some recalculation of the height and assign it to the container at appropriate times. :-/

There's also several text alignment issues, in both directions, because of the varied contents of cells. Setting vertical-align: baseline on all of the cells should help. In the case of the multiple repeat strings, I'd concatenate them, and put them in a div. (Or straight into the cell, I don't think having inner divs gains us anything.)

Thanks for the review. I've updated this part1 patch to address the comments. Here's a just-started try run with part1 and part2. (part3 will need more work.)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5061f66495f60934cc8dda30bac12fdadd39d251

Attachment #9151384 - Attachment is obsolete: true
Attachment #9152280 - Flags: review+

Try runs are spotty given the bustage this week. Here's the latest which isn't helpful: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central

Locally, with part1 and part2 applied, the tests pass, except for browser_import.js which will need to be updated for the part2 patch.

Given the beta release and string freeze coming up, I'm going ahead and setting checkin-needed-tb for part1 at least. part2 is ready to land too except for the failing browser_import.js test. I'll see if I can get that test working this weekend before the beta release.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/37b8335f94dc
Add event and task preview to ICS file import dialog. r=darktrojan

New version of part2 that updates and expands the browser_import.js test to cover the new import dialog. Besides changes to that test file, there are only a few differences from the previous version of the patch, namely these:

--- a/calendar/base/content/import-export.js
+++ b/calendar/base/content/import-export.js
 
 /**
  * Loads events from a file into a calendar. If called without a file argument,
  * the user is asked to pick a file.
  *
  * @param {nsIFile} [fileArg] - Optional, a file to load events from.
- * @return {Promise<boolean>} True if the import succeeded, false if not (e.g.
- *                            user canceled the file picker dialog).
+ * @return {Promise<boolean>} True if the import dialog was opened, false if
+ *                              not (e.g. on cancel of file picker dialog).
  */
 async function loadEventsFromFile(fileArg) {
   let file = fileArg;
   if (!file) {
     file = await pickFileToImport();
     if (!file) {
       // Probably the user clicked "cancel" (no file and the promise was not
       // rejected in pickFileToImport).
-      return;
+      return false;
     }
   }
 
   Services.ww.openWindow(
     null,
     "chrome://calendar/content/calendar-ics-file-dialog.xhtml",
     "_blank",
     "chrome,titlebar,modal,centerscreen",
     file
   );
+  return true;
 }

Tests pass locally, have not tried a try run yet due to current bustage.

Attachment #9151385 - Attachment is obsolete: true
Attachment #9152974 - Flags: review?(geoff)
Comment on attachment 9152974 [details] [diff] [review]
part2-use-ics-file-import-dialog-everywhere-1.patch

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

Fine by me.
Attachment #9152974 - Flags: review?(geoff) → review+

Thanks Geoff. Here's a try run, just started, using a special GECKO_HEAD_REF to avoid bustage (thanks Magnus for that):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central

Setting checkin-needed-tb for the part2 patch, assuming the try run goes well (tests pass locally), in the interest of getting this into the next beta release. I'll work on part3 and anything else needed in follow-up bugs.

For windows there's a small error on try:

TEST-UNEXPECTED-FAIL | comm/calendar/test/browser/browser_import.js | the displayed ics file path is correct - Got browser\comm\calendar\test\browser\data\import.ics, expected browser/comm/calendar/test/browser/data/import.ics
[task 2020-05-31T04:33:04.812Z] 04:33:04 INFO - Stack trace:
[task 2020-05-31T04:33:04.812Z] 04:33:04 INFO - chrome://mochikit/content/browser-test.js:test_is:1327
[task 2020-05-31T04:33:04.812Z] 04:33:04 INFO - chrome://mochitests/content/browser/comm/calendar/test/browser/browser_import.js:dialogWindowPromise<:48
[task 2020-05-31T04:33:04.812Z] 04:33:04 INFO - resource://testing-common/BrowserTestUtils.jsm:promiseAlertDialogOpen:2173
[task 2020-05-31T04:33:04.812Z] 04:33:04 INFO - resource://testing-common/BrowserTestUtils.jsm:promiseAlertDialog:2201
[task 2020-05-31T04:33:04.813Z] 04:33:04 INFO - chrome://mochitests/content/browser/comm/calendar/test/browser/browser_import.js:null:38

Whiteboard: [for78beta]

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/713f46cfd0fa
Use the new file import dialog for all calendar file imports. r=darktrojan DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

I fixed the Windows path separators (at least I assume I did, running a bit blind there), and also stopped using hard-wired date strings which I didn't pick up in review. They should be avoided because they're locale dependent, and also 12/24 hour clock dependent.

Target Milestone: --- → 78

Thanks Geoff for fixing those things and landing this in time for the 78 beta!

Follow-ups to tackle in separate bugs:

  • let the text wrap in the right hand column of the table, particularly date fields, (the WIP part3 patch) (bug 1642635)
  • event and task lists should be updated immediately on import
  • show reminders/alarms for items that have them
  • look into improving responsiveness and performance when importing larger files
  • update thunderbird --help to mention calendar file support with -file option (bug 1642347)
Attachment #9151389 - Flags: feedback?(geoff)
Blocks: 1642635
Summary: [meta] Add event preview to ICS import dialog → Add event preview to ICS import dialog
Blocks: 1648521
You need to log in before you can comment on or make changes to this bug.