1575195 - opening an event should open the event summary, not the edit event dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement)

Description

[Magnus Melin [:mkmelin] (away until July 8)]

Quite often I need to check the details of an event to check description, location, and rsvp statuses. If it's an editable event that always pops up the Edit Event dialog.

A better flow for this would be to open the event summary and have an edit event button in there (since editing is a secondary action for an existing event). This is also similar to how e.g. Google Calendar and others work.

Comment 1

[Stefan Sitter [:ssitter]]

And I usually only open events to edit them because description, location and more information is already shown in event mouse over tooltip. Therefore the proposed new workflow would make editing more complicated for me. If this enhancement request will be realized there should be at least a hidden preference to restore the current behavior of directly opening events for editing.

Comment 3

[Alessandro Castellani [:aleca]]

The most common action a user does on a calendar event is looking at it to check description, participants, and other important details.
Currently we don't allow a simple "look" at an event unless we double click and we prompt the user with an "Do you want to edit?" option.

We should implement a popup panel which presents readable information and common actions like [Edit] [Edit all Instances] [Delete] etc...

I'm gonna make a quick mock-up for this.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: calendar-popup.png

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 9139613 [details]
calendar-popup.png

I like it.

It's planned to show it instead of the tooltip when the mouse hovers the event or is one click needed? I'd prefer the first.

Maybe, when where is enough space, the description could be longer, maybe three lines. A lot of Meeting descriptions begin with `Hello all` or something  similar and the real text is on the next lines.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: contact-popup.png

Maybe the footer buttons should follow the popup styles FX introduced and we use too on the contacts- and update popups.

Comment 7

[Magnus Melin [:mkmelin] (away until July 8)]

Comment on attachment 9139613 [details]
calendar-popup.png

Looks good. Agreed we should show more of the description, probably just make it scrollable if it's long.
Beware there are a lot of details of how to show all this depending on if it's readonly and/or it's a recurring event and other factors.

Comment 8

[Alessandro Castellani [:aleca]]

It's planned to show it instead of the tooltip when the mouse hovers the event or is one click needed? I'd prefer the first.

It should open onclick and not on hover.
That is necessary to allow the user to interact with the popup (copying/clicking links, editing, etc) without needing to stay withing the hovered area to prevent accidental closing, and also we don't want to trigger this big popup every time the user hovers over an event.

The tooltip showing on hover should be simplified a lot.

Maybe, when where is enough space, the description could be longer, maybe three lines.

Oh for sure, that's just a mockup example. We can implement a character limit to it and then add a more link to expand it.

Maybe the footer buttons should follow the popup styles FX introduced and we use too on the contacts- and update popups.

I don't know about that, as those type of buttons have the same importance and give the focus on a primary "Done", which we won't need in this case. We can definitely explore that approach, but I'd be more keen to implement something more specific that we can fully control and is not too strict in terms of UI.

Beware there are a lot of details of how to show all this depending on if it's readonly and/or it's a recurring event and other factors.

Indeed, all these edge cases need to be addressed.
I can create various mockups for all these cases in order to have a clear overview and objective during development.

Comment 9

[Paul Morris [:pmorris]]

Comment on attachment 9139613 [details]
calendar-popup.png

I like it!  Will be a nice improvement.  I'd second Magnus' point about the various cases to handle (recurring, read-only, etc.).  Just a thought, would it make sense to have something similar for tasks?  Probably as a follow-up, if anything, but might be worth considering earlier for consistency.

Comment 10

[Magnus Melin [:mkmelin] (away until July 8)]

The mockups look good, but we may have to defer that until later. For now, we could just do

• open the dialog as the (readonly) summary dialog by default
• if the event can be edited, add a button in the summary dialog to get to the editing dialog

Code should be around what's leading up to here: https://searchfox.org/comm-central/rev/5493261453529a97fd9513a852422dabab92adb9/calendar/base/content/calendar-item-editing.js#516

Comment 11

[Lasana Murray]

I tracked the start of the code path to here:
https://searchfox.org/comm-central/rev/5493261453529a97fd9513a852422dabab92adb9/calendar/base/content/calendar-editable-item.js#94

^ A double click event listener calls the modifyOccurence() function of calendarViewController with the event item(s). Looking at the interface for calIcalendarViewController, it has methods for creating events, editing events and deleting events. There isn't one for strictly showing events (modifyOccurence is used instead):
https://searchfox.org/comm-central/rev/5493261453529a97fd9513a852422dabab92adb9/calendar/base/public/calICalendarViewController.idl

The modifyOccurence() function eventually calls the modifyEventWith function, which calls the openEventDialog Magnus linked to above.

This function detects whether to open the dialog in read-only or edit depending on the calendar configuration. In fact it only accommodates a "modify" or "new" mode of operation. The read-only summary appears to only be used if the calendar can't be modified:
https://searchfox.org/comm-central/rev/5493261453529a97fd9513a852422dabab92adb9/calendar/base/content/calendar-item-editing.js#516

My first inclination is to create a dedicated showOccurence() method , effectively making calICalendarViewController a complete CRUD interface. openEventDialog would then have to be refactored for an intentional "read-only" mode, however, I feel more comfortable about creating a new function than modifying the existing one.

Comment 12

[Lasana Murray]

Edit:
modifyEventWith should be modifyEventWithDialog.

Also I tracked the "Open" command from the context menu and it eventually calls the the same function above.
Thoughts?

Comment 13

[Magnus Melin [:mkmelin] (away until July 8)]

Sounds good I think.

Comment 14

[Lasana Murray]

Attachment: bug1575195B.patch

This is what I have so far. I changed the event handler for double clicking and left the context menu as is for now for comparison.

The summary dialog opens and the user can switch to editing by clicking the "Edit" button at the bottom. Editing and saving seems to work, however errors are thrown before that probably should not be ignored.

I think it's because of where I opened the edit dialog from. I'm going to try opening from the controller in calendar-editable-item instead.

Comment 15

[Magnus Melin [:mkmelin] (away until July 8)]

Comment on attachment 9151886 [details] [diff] [review]
bug1575195B.patch

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

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +276,5 @@
> + * @param {CalendarItem} item
> + */
> +function useEditDialog(item) {
> +  window.addEventListener('unload', () => {
> +    modifyEventWithDialog(item);

I think should work:
```
window.opener.modifyEventWithDialog(item);
```

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
@@ +426,5 @@
>  <!ENTITY summary.dialog.dontsend.tooltiptext    "Change your participation status without sending a reply to the organizer and close the window">
>  <!ENTITY summary.dialog.send.label              "Send a response now">
>  <!ENTITY summary.dialog.send.tooltiptext        "Send out a response to the organizer  and close the window">
> +<!ENTITY summary.dialog.edit.label              "Edit">
> +<!ENTITY summary.dialog.edit.tooltiptext        "Edit this event.">

we don't usually use a period at the end of tooltips

Comment 16

[Lasana Murray]

Attachment: bug1575195.patch

I tested this manually and seems to work. Unfortunately it breaks quite a few tests (at least 30). I'm looking into fixing that at the moment.

Comment 17

[Magnus Melin [:mkmelin] (away until July 8)]

Comment on attachment 9153909 [details] [diff] [review]
bug1575195.patch

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

::: calendar/base/content/calendar-item-editing.js
@@ +332,5 @@
> + * Opens the passed event item for viewing.
> + *
> + * This enables the modify callback so invitation responses can be edited.
> + *
> + * @param item                 The calendar item to view.

The existing code didn't have it, but it's nice when you add new code to add jsdoc comments to it to help future readers. (Also, I'd drop the extra whitespace.)

```
@param {calIItemBase} item - The calendar item to view.
```

@@ +334,5 @@
> + * This enables the modify callback so invitation responses can be edited.
> + *
> + * @param item                 The calendar item to view.
> + */
> +function viewEventWithDialog(item) {

Maybe better to name it per the description, like  openEventDialogForViewing?

@@ +353,5 @@
>   * @param aItem                 The item to modify.
>   * @param job                   (optional) The job object that controls this
>   *                                           modification.
>   * @param aPromptOccurrence     If the user should be prompted to select if the
> + *          Opens the passed event item for viewing.                       parent item or occurrence should be modified.

Odd whitespace. And a typical case of why the extra whitespace will just mess up readability... I'd remove all the extra.

::: calendar/base/content/calendar-views-utils.js
@@ +568,5 @@
> + * Open the items currently selected in the view.
> + */
> +function viewSelectedEvents() {
> +  let items = currentView().getSelectedItems();
> +  if (Array.isArray(items) && items.length >= 1) {

This is always an array

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +278,5 @@
> +function updateAvailableButtons() {
> +  if (window.readOnly === true || window.isInvitation === true) {
> +    document.getElementById("edit-button").setAttribute("hidden", "true");
> +  } else if (window.inInvitation !== true) {
> +    document.getElementById("summary-toolbar").setAttribute("hidden", "true");

for these you can just use .hidden = true;

(This will work also for html, setting the attribute won't since it's a boolean attribute in html: it's either there and set, or it's not there at all)

@@ +284,5 @@
> +}
> +
> +/**
> + * Switch to the "modify" mode dialog so the user can make changes to the event.
> + * @param {CalendarItem} item

Is CalendarItem a type?

::: calendar/base/content/dialogs/calendar-summary-dialog.xhtml
@@ +139,5 @@
> +            tooltiptext="&summary.dialog.edit.tooltiptext;"
> +            label="&summary.dialog.edit.label;"
> +            oncommand="useEditDialog(window.calendarItem);" />
> +
> +  </hbox>

It's better (will look better and be consistent) to use the standard dialog button functionality. See e.g. https://searchfox.org/comm-central/rev/771b2328aa3096e6c907f477d7ebd862c8bf0841/calendar/base/content/dialogs/calendar-ics-file-dialog.xhtml#22-24

Could also be worth starting to use fluent for this file (like the calendar-ics-file-dialog does.)

Comment 18

[Lasana Murray]

Attachment: 1575195D-dialog-changes.patch

Comment 19

[Lasana Murray]

Attachment: 1575195E-update-existing-tests.patch

Comment 20

[Lasana Murray]

Attachment: 1575195E-test.patch

Comment 21

[Lasana Murray]

I split them into 3 commits to avoid confusing myself. First patch is the changes to calendar. Second are updates to existing tests and the third is a test to see if the summary dialog is opened when you click on an event.

Comment 22

[Magnus Melin [:mkmelin] (away until July 8)]

Comment on attachment 9157483 [details] [diff] [review]
1575195D-dialog-changes.patch

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

Looks alright to me

Comment 23

[Paul Morris [:pmorris]]

Comment on attachment 9157483 [details] [diff] [review]
1575195D-dialog-changes.patch

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

Nice work Lasana!  It works when I tried it and the implementation looks good overall.  There are some things still to do but it's pretty much in good shape.  I have some "product" level thoughts about this but I'll put them in their own comment to keep this one focused on the code review.

::: calendar/base/content/calendar-item-editing.js
@@ +335,5 @@
> + * can be edited.
> + * @param {calIItemBase} item - The calendar item to view.
> + */
> +function openEventDialogForViewing(item) {
> +  openEventDialog(item, item.calendar, "view", function(

This is fine as-is, but I often find the code is easier to read if I define the callback function first and then pass it in as an arg:

function callback() { ... }
funcWithCallbackArg(callback)

@@ +418,1 @@
>   * @param callback          The callback to call when the dialog has completed.

While we're here and you're writing code that uses this callback argument, it would be good to add a JSDoc comment for this callback, using @callback (https://jsdoc.app/tags-callback.html).  Feel free to ask me on chat if you have questions about this.

@@ +464,1 @@
>      /* modify */

Let's go ahead and delete this comment since it doesn't add much now.

::: calendar/base/content/calendar-views-utils.js
@@ +42,5 @@
>      }
>    },
>  
>    /**
> +   * View the given occurrence.

I think we should go ahead and add a @param with the argument type here.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +95,5 @@
>      itemSummary.onWindowResize();
>    });
>  
>    // If this item is read only we remove the 'cancel' button as users
>    // can't modify anything, thus we go ahead with an 'ok' button only.

This comment needs to be updated.

@@ +100,5 @@
>    if (window.readOnly) {
>      dialog.getButton("cancel").setAttribute("collapsed", "true");
>      dialog.getButton("accept").focus();
> +    // disable default controls
> +    let accept = dialog.getButton("accept");

Go ahead and move the `let accept` line up so you can use it in the `.focus()` line above.
And I would delete the `// disable default controls` comment, since we have the other comment just above this `if` conditional.

@@ +107,1 @@
>    }

Now that I see the new `updateAvailableDialogButtons` function, I think we should move this `if` and all its code into that function, so we're handling all these button updates in one place.

@@ +272,5 @@
> + */
> +function updateAvailableDialogButtons(dialog) {
> +  let editButton = dialog.getButton("accept");
> +  editButton.addEventListener("click", e => {
> +    e.preventDefault();

Nit: I'd prefer to use "event" instead of "e" here.

@@ +277,5 @@
> +    useEditDialog(window.calendarItem);
> +  });
> +  if (window.readOnly === true || window.isInvitation === true) {
> +    editButton.hidden = true;
> +  } else if (window.inInvitation !== true) {

Typo: isInvitation

@@ +278,5 @@
> +  });
> +  if (window.readOnly === true || window.isInvitation === true) {
> +    editButton.hidden = true;
> +  } else if (window.inInvitation !== true) {
> +    document.getElementById("summary-toolbar").hidden = true;

Hm, I see there's an existing `updateToolbar` function in this file, so it would be better to put the new toolbar update code there instead, and use this function only for the buttons.

::: calendar/lightning/content/calendar-context-menus-and-tooltips.inc.xhtml
@@ +98,5 @@
>  
>    <!-- CALENDAR ITEM CONTEXT MENU -->
>    <menupopup id="calendar-item-context-menu"
>               onpopupshowing="return setupContextItemType(event, currentView().getSelectedItems());">
>      <menuitem id="calendar-item-context-menu-modify-menuitem"

This id (and any related CSS, etc.) should be updated now that this menuitem is not for modifying the item but for viewing it.

Comment 24

[Paul Morris [:pmorris]]

Looking at this more closely, I have some reservations. I like Alex's design for a quick and lightweight popup as an improvement over the current hover/tooltip summary (comment 4), but I'm not sure that opening our current read-only summary dialog and requiring a second click to edit the item is a good idea. We might be better off taking our time and implementing Alex's design rather than doing this intermediate step. Or if we do want to do the intermediate step, I agree with comment 1 that there should be a preference to either enable the new behavior or restore the current behavior.

The biggest pain point we currently have is when you just want to view a repeating event, you open it but then get a dialog that asks about editing just that event or all of them. I think that's the most pressing UX problem to solve.

However, (with the current state of the implementation work), if you want to edit a repeating event you have to open the event in a summary dialog, then click the "Edit" button which opens a second dialog asking whether you want to edit just this event or all of them, and then click one of those options to open a third dialog for editing. So while we're reducing the "view repeating event" flow from two dialogs to one, we are increasing the "edit repeating event" flow from two dialogs to three.

A fix for that problem: for repeating events, instead of an "Edit" button in the summary dialog, offer "Edit only this occurrence" and "Edit all occurrences" buttons. That would remove the need for the "Edit Repeating Event" dialog, basically merging it into the summary dialog, which would then do that job. Then editing a repeating event is back to two dialogs.

So, here's what I would propose:

• when opening a non-repeating event, show the edit event dialog (the current behavior)
• when opening a repeating event, show the read-only event dialog with "Edit only this occurrence" and "Edit all occurrences" buttons
• optionally add a pref to allow opening a non-repeating event in the read-only dialog with an "Edit" button to edit it.

That solves the biggest UX problem while otherwise maintaining the current behavior, to reduce UI/UX churn, until we have time to implement Alex's design for a lightweight popup.

(Aside: another UI/UX question is that (with the current patch), when you open up an event, reminders are editable in that dialog but nothing else is. Average user asks "why?". Having reminders be editable makes sense with current uses of the summary dialog, but it might need some consideration if we're using it in this new way.)

Comment 25

[Paul Morris [:pmorris]]

For comparison, on google calendar:

• a single click brings up the quick preview event panel
• a double-click opens the full edit event UI

Comment 26

[Paul Morris [:pmorris]]

Comment on attachment 9157484 [details] [diff] [review]
1575195E-update-existing-tests.patch

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

Looks fine overall, and the tests passed when I ran them.  Just a few of things to address.  

Something to be aware of is that we are moving away from Mozmill in favor of Mochitest, and this is more or less Mozmill code you're working with here.  Ideally we would convert the tests to Mochitest as we edit them, and avoid adding new Mozmill tests.  However, since you've already done the work here, I'm fine with going ahead with this patch.  Just don't get too attached to Mozmill and its idioms. :-)

::: calendar/test/browser/eventDialog/browser_alarmDialog.js
@@ +13,5 @@
>    createCalendar,
>    deleteCalendars,
>    goToDate,
>    helpersForController,
>    invokeEventDialog,

Are we no longer using invokeEventDialog in this file?  If not, go ahead and remove it here.  Same goes for other files.

::: calendar/test/browser/recurrence/browser_daily.js
@@ +15,5 @@
>    goToDate,
>    handleOccurrencePrompt,
>    helpersForController,
>    invokeEventDialog,
> +  invokeEditingEventDialog,

If this function is not called in this file, then no need to import it here.

::: calendar/test/modules/CalendarUtils.jsm
@@ +321,5 @@
> + * @param {MozMillElement|null} clickBox     - The optional box to click on.
> + * @param {function}            body         - The function to execute while
> + *                                             the event dialog is open.
> + */
> +async function invokeEventDialog(mWController, clickBox, body) {

Should we rename this to `invokeNewEventDialog` if it's only to be used for new events?

@@ +344,5 @@
> + * box and executing the body function. The dialog must be closed in the
> + * body function.
> + *
> + * The body is passed a MozMillController when executed.
> + * NOTE: This function will timesout if the "clickBox" opens a new event instead

typo: time out

Comment 27

[Lasana Murray]

Something to be aware of is that we are moving away from Mozmill in favor of Mochitest, and this is more or less Mozmill code you're working with >here
I'm confused by this, I noticed the Mozmill documentation was deprecated but we use ../mach mochitest to run tests, I thought Mochitest was just the test runner? Can you point me to a test that is pure mochitest so I can study? Thanks!

Comment 28

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #27)

I'm confused by this, I noticed the Mozmill documentation was deprecated but we use ../mach mochitest to run tests, I thought Mochitest was just the test runner? Can you point me to a test that is pure mochitest so I can study? Thanks!

It is definitely a confusing situation. Geoff converted the existing mozmill tests to mochitests, which involved "wrapping" the mozmill test code in mochitest "wrappers" (as it were, not a technical term...). So the tests are technically mochitests, and are run as mochitests, etc. But much of the code still uses mozmill functions, patterns, idioms, etc. (e.g. controllers). So we're migrating away from the mozmill idioms and new tests should ideally avoid them.

So here's a "pure mochitest" that deals with dialogs. (I'm familiar with this one since I wrote it a few months back.)
https://searchfox.org/comm-central/source/mail/components/addrbook/test/browser/browser_mailing_lists.js

See also: https://developer.thunderbird.net/thunderbird-development/testing/writing-mochitest-tests

Comment 29

[Alessandro Castellani [:aleca]]

I agree with Paul observation.

With 1 click the event should only open a popover overview to click links or select tests, without editable fields.
In my mock-up, the only "editable" field was the approval status for an event where the user was invited.

But, as Magnus said in comment 10, we need to defer this new feature for later and implement this with what we have.

when opening a non-repeating event, show the edit event dialog (the current behavior)
when opening a repeating event, show the read-only event dialog with "Edit only this occurrence" and "Edit all occurrences" buttons
optionally add a pref to allow opening a non-repeating event in the read-only dialog with an "Edit" button to edit it.

I like this approach, and at a quick glance it seems consistent and not confusing for the user.

Comment 30

[Paul Morris [:pmorris]]

Comment on attachment 9157485 [details] [diff] [review]
1575195E-test.patch

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

This looks good overall.  We do want to move away from mozmill, but since we didn't communicate that to you before you did this work, I'm inclined to go ahead and get this landed in the mozmill style and then convert it to mochitest style as a follow-up, or later depending on priorities.

Thanks for organizing this in separate patches. Makes it easier to review.  Given Alex's comment, I think the next step would be a patch to basically merge the "Edit Repeating Event" dialog (calendar-occurrence-prompt.xhtml) with the summary dialog, so that repeating events have two edit buttons (edit just this event and edit all of these events).  That could either be added to this bug or a follow-up bug.

::: calendar/test/browser/eventDialog/browser_eventDialog.js
@@ +219,5 @@
> +
> +  let createBox = lookupEventBox("day", CANVAS_BOX, null, 1, 8);
> +  let eventBox = lookupEventBox("day", EVENT_BOX, null, 1, null, EVENTPATH);
> +
> +  //Create a new event.

Nit: space: // Create

@@ +234,5 @@
> +  // Open the event, it should open in the summary dialog or this fails.
> +  await invokeViewingEventDialog(
> +    controller,
> +    eventBox,
> +    async event => {

While we're testing this, it would be good to add some checks here to confirm that it is indeed the summary dialog and not the edit dialog, and that the correct event was loaded, with the same details used when creating it.

Comment 31

[Lasana Murray]

Attachment: bug1575195f-dialog-changes.patch

Made some of the changes requested. Removed the code that collapses the cancel button as it does not appear to make sense with these changes. Added JSDOC for a callback and other things.

Comment 32

[Lasana Murray]

Attachment: bug1575195f-update-existing-tests.patch

invokeEventDialog -> invokeNewEventDialog. Typos fixed.

Comment 33

[Lasana Murray]

Attachment: part3-test.patch

Added some assertions to the test, had to add ids to the fields used for the test event. The invokeViewingEventDialog will fail with an error if anything other than the summary dialog opens so a passing test means the correct dialog opened.

Comment 34

[Lasana Murray]

Given Alex's comment, I think the next step would be a patch to basically merge the "Edit Repeating Event" dialog (calendar-occurrence-prompt.xhtml) with the summary dialog, so that repeating events have two edit buttons (edit just this event and edit all of these events). That could either be added to this bug or a follow-up bug.

I prefer to attack this in a separate bug.

Comment 35

[Paul Morris [:pmorris]]

(In reply to Lasana Murray from comment #34)

I prefer to attack this in a separate bug.

That works for me.

Comment 36

[Paul Morris [:pmorris]]

Comment on attachment 9158569 [details] [diff] [review]
bug1575195f-dialog-changes.patch

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

Looking good, thanks for making the changes.  On looking at this again I have some more nits and a few other todos.  I know the nits are picky picky but I wanted to go ahead and mention them to help get you up to speed on the general styles we use in order to keep things relatively consistent.

Good to tackle showing the different buttons for repeating events in a separate patch.  I think another piece of that is that non-repeating events should be opened in the "edit" dialog by default, so only repeating, read-only, and invitation events would open in the summary dialog.  I think Alex agrees with me based on his recent comment.  With this patch all events are opened in the summary dialog.  I know you probably just want to get this landed, but what do you think about adding a patch to this bug to check whether an event is repeating or not and then open the summary dialog if it is repeating and the edit dialog if it is not?  (That is, for an event that's not read-only and not an invitation.)  We could do that in a follow-up instead, but it would be fewer changes for beta users if we made that change as we land these patches.  Either way would work.  What do you think?

::: calendar/base/content/calendar-item-editing.js
@@ +329,5 @@
>  }
>  
>  /**
> + * Opens the passed event item for viewing.
> + *

Nit: Just a style thing but I'd drop the empty line (333) and move the sentence below it up inline right after the first sentence.

@@ +332,5 @@
> + * Opens the passed event item for viewing.
> + *
> + * This enables the modify callback in openEventDialog so invitation responses
> + * can be edited.
> + * @param {calIItemBase} item - The calendar item to view.

Nit: we usually put an empty line above the first @param.  Looks like there are a few places in this patch like this, if you wouldn't mind changing them, just for consistency's sake.

@@ +338,5 @@
> +function openEventDialogForViewing(item) {
> +  function onDialogComplete(newItem, calendar, originalItem, listener, extresponse) {
> +    doTransaction("modify", newItem, calendar, originalItem, listener, extresponse);
> +  }
> +  openEventDialog(item, item.calendar, "view", onDialogComplete);

Looks good, thanks!

@@ +409,5 @@
> + * @param {calIItemBase}           newItem
> + * @param {calICalendar}           calendar
> + * @param {calIItemBase}           originalItem
> + * @param {?calIOperationListener} listener
> + * @param {?Object}                extresponse

Great to have this documented better.  Nit: usually just one space between the type and the name (more on that below).

@@ +427,5 @@
> + * @param {?calIDateTime}                    The initial date for new task
> + *                                           datepickers.
> + * @param {?Object}          counterProposal An object representing the
> + *                                           counterproposal - see description
> + *                                           for modifyEventWithDialog()

Thanks for adding the types here, which are very helpful.  A nit on the spacing: we typically do it like this: 

@param {?Object} job             The job object for the modification.

with just one space between the param type and the name (that also gives you more room for the description).  

Note that Philipp prefers having more space between the name and description like you've done here, with the descriptions aligned vertically, so I tend to do that for calendar code.  (Philipp is the owner of calendar component.)  But for non-calendar Thunderbird code we do the following (with the " - " and no extra space), which Magnus prefers:

@param {?Object} job - The job object for the modification.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ -98,5 @@
> -  // can't modify anything, thus we go ahead with an 'ok' button only.
> -  if (window.readOnly) {
> -    dialog.getButton("cancel").setAttribute("collapsed", "true");
> -    dialog.getButton("accept").focus();
> -  }

Good call to remove the collapsing of the cancel button.  I think we do want to focus the accept button for `readOnly` though.  I haven't checked but I think that means you can just hit "enter" key to close the dialog.  I suppose that would now go in `updateAvailableDialogButtons`.

@@ +182,5 @@
>  }
>  
>  function updateToolbar() {
> +  if (window.readOnly || window.isInvitation !== true) {
> +    document.getElementById("summary-toolbar").hidden = true;

Looks good.  Here `window.isInvitation !== true` could be simplified to just `!window.isInvitation` if you like the shorter version better.

@@ +255,5 @@
> +/**
> + * This will show/hide the edit button and toolbar based on various conditions:
> + * 1) The calendar is read-only              - Hide the edit button.
> + * 2) The item is an invitation              - Hide the edit button.
> + * 3) The item is not an invitation          - Hide the summary toolbar.

This doc comment needs an update since the toolbar is no longer being hidden here, so 3 would just be "Show the edit button." right?

@@ +263,5 @@
> +  let editButton = dialog.getButton("accept");
> +  editButton.addEventListener("click", event => {
> +    event.preventDefault();
> +    useEditDialog(window.calendarItem);
> +  });

Looking at this again, I don't think we want to add the click listener when the button will be hidden, so we should only add it when the window is not `readOnly` and also not `isInvitation`.  Right?  (I haven't checked but it may work to hit the "enter" key to dismiss the dialog, and this event listener will preventDefault, making that not work, that is, if it even works now.)

Also instead of "click" I think "command" would be better so it works for keyboard activation of the button as well.

Comment 37

[Magnus Melin [:mkmelin] (away until July 8)]

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

when opening a non-repeating event, show the edit event dialog (the current behavior)

I think that would be inconsistent - why would it matter if it's a repeating event or not?
The issue I'm hoping to solve in this bug is that likelihood of just wanting to view the data once it's in the calendar is much higher than that's you'd want to edit it. Repeating or not doesn't make a difference.

Comment 38

[Paul Morris [:pmorris]]

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

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

when opening a non-repeating event, show the edit event dialog (the current behavior)

I think that would be inconsistent - why would it matter if it's a repeating event or not?
The issue I'm hoping to solve in this bug is that likelihood of just wanting to view the data once it's in the calendar is much higher than that's you'd want to edit it. Repeating or not doesn't make a difference.

Here's a counter argument. Editing and viewing events should be equally easy to do, regardless of which may be done more often by any given user. (Some users may need to edit events very often.) Currently, this is the case, viewing and editing are both equally easy. You can view and/or edit an event by opening it in the edit dialog (because the edit dialog lets you both view and edit). Also, you can view some of an event's data by hovering the event to see it in a tooltip.

Opening the summary dialog by default doesn't really improve the user's ability to view events (since you can already view them via the edit dialog or tooltip), but it does make editing harder by requiring the user to click through the summary dialog to get to the edit dialog.

The biggest issue we currently have is that viewing repeating events is harder (than viewing non-repeating events) because you have to click through the "edit this event or all events" dialog to get to the edit dialog. A way to improve this is to show the summary dialog for repeating events, with two buttons to "edit this event or edit all events", effectively combining those two dialogs. That way viewing repeating events becomes easier, and editing events remains just as easy as before. (Interestingly, google calendar handles this by asking when you save changes whether they should apply to all events or just one event, etc.)

(End of counter argument.) Is there a way to keep editing just as easy as viewing, while achieving the goals of this bug? (E.g. in google calendar single click to view and double click to edit.) I like Alex's design and I think it's worth taking our time to implement it. If we are going to make opening the summary dialog the default, then I like the idea of having a pref so users can customize the behavior to fit their use case (which may involve editing events frequently).

Comment 39

[Lasana Murray]

Attachment: part2-update-existing-tests.patch

This became outdated with this commit:
https://hg.mozilla.org/comm-central/rev/a248d78e15180c4a9c62e07d6d07291c796a254e

Comment 40

[Magnus Melin [:mkmelin] (away until July 8)]

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

Here's a counter argument. Editing and viewing events should be equally easy to do, regardless of which may be done more often by any given user. (Some users may need to edit events very often.) Currently, this is the case, viewing and editing are both equally easy.

I think currently they are not equally easy. The reason you open the event in the first place is usually to get the information out of there: perhaps most prominently to open URLs for meetings, but there are many more cases the exposing the content would come in handy. You could e.g. add actions to follow-up with the people invited (you can atm, but only through the context menus in the editing window - sick!), add copyable summaries, guest lists, and all sorts of ways to copy that data out of there for when you need it elsewhere. These actions do not really have a good fit for the editing window.

Look at your phone calendar: Nothing opens editable from start.

Comment 41

[Paul Morris [:pmorris]]

Comment on attachment 9159342 [details] [diff] [review]
part2-update-existing-tests.patch

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

Looks good!

Comment 42

[Lasana Murray]

Attachment: bug1575195g-dialog-changes.patch

Made the changes requested. Normally I avoid truthy values in JavaScript to avoid the propagation of bugs so I left window.isInvitation === true as is.

Comment 43

[Paul Morris [:pmorris]]

Comment on attachment 9158571 [details] [diff] [review]
part3-test.patch

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

Looks good, thanks.

Comment 44

[Lasana Murray]

My two cents, if I may; I always found bringing up editing screens when a user wants to inspect an item annoying. It makes it easier to make accidental changes when you just want to review something, especially if your device runs slowly and is occasionally unresponsive.

I'm not really a fan of the current iteration of Google calendar, I mostly only use it to create events and get out as fast as I can. To me it complicates the simple tasks of opening or editing an event and its details.

In general, I prefer linear paths for carrying out actions, example: click on event -> see event details -> click edit -> edit event item as opposed to branching steps (if the event repeats, if the user double clicks etc.) even if they are shorter. That's because it's easier to remember and recognize these workflows than ones that change based on implicit circumstances. As you get more familiar with an application and become a "power user" this becomes more important.

That's my opinions as a user, as a developer, I have no opinions. :)
Just waiting for a decision so I can implement it.

(In reply to Paul Morris [:pmorris] from comment #38)
(In reply to Magnus Melin [:mkmelin] from comment #37)

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

when opening a non-repeating event, show the edit event dialog (the current behavior)

I think that would be inconsistent - why would it matter if it's a repeating event or not?
The issue I'm hoping to solve in this bug is that likelihood of just wanting to view the data once it's in the calendar is much higher than that's you'd want to edit it. Repeating or not doesn't make a difference.

Here's a counter argument. Editing and viewing events should be equally easy to do, regardless of which may be done more often by any given user. (Some users may need to edit events very often.) Currently, this is the case, viewing and editing are both equally easy. You can view and/or edit an event by opening it in the edit dialog (because the edit dialog lets you both view and edit). Also, you can view some of an event's data by hovering the event to see it in a tooltip.

Opening the summary dialog by default doesn't really improve the user's ability to view events (since you can already view them via the edit dialog or tooltip), but it does make editing harder by requiring the user to click through the summary dialog to get to the edit dialog.

The biggest issue we currently have is that viewing repeating events is harder (than viewing non-repeating events) because you have to click through the "edit this event or all events" dialog to get to the edit dialog. A way to improve this is to show the summary dialog for repeating events, with two buttons to "edit this event or edit all events", effectively combining those two dialogs. That way viewing repeating events becomes easier, and editing events remains just as easy as before. (Interestingly, google calendar handles this by asking when you save changes whether they should apply to all events or just one event, etc.)

(End of counter argument.) Is there a way to keep editing just as easy as viewing, while achieving the goals of this bug? (E.g. in google calendar single click to view and double click to edit.) I like Alex's design and I think it's worth taking our time to implement it. If we are going to make opening the summary dialog the default, then I like the idea of having a pref so users can customize the behavior to fit their use case (which may involve editing events frequently).

Comment 45

[Lasana Murray]

Attachment: bug1575195h-dialog-changes.patch

I forgot to run eslint on the previous patch. Sorry!

Comment 46

[Paul Morris [:pmorris]]

Comment on attachment 9159701 [details] [diff] [review]
bug1575195h-dialog-changes.patch

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

Looking good, very close, just one remaining nit and one other tiny change.  Then it'll be good to go for a try run.

::: calendar/base/content/calendar-item-editing.js
@@ +409,5 @@
> + * @param {calIItemBase}           newItem
> + * @param {calICalendar}           calendar
> + * @param {calIItemBase}           originalItem
> + * @param {?calIOperationListener} listener
> + * @param {?Object}                extresponse

nit: just one space between type and name for consistency: 
{calIItemBase} newItem

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +262,5 @@
> + */
> +function updateAvailableDialogButtons(dialog) {
> +  let acceptButton = dialog.getButton("accept");
> +  if (window.readOnly === true || window.isInvitation === true) {
> +    acceptButton.focus();

Hm, previously the accept button was only focused when `window.readOnly === true`, so I think we should keep that behavior and not focus it when `window.isInvitation === true`.

Comment 47

[Paul Morris [:pmorris]]

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

The reason you open the event in the first place is usually to get the information out of there: perhaps most prominently to open URLs for meetings, but there are many more cases the exposing the content would come in handy.

Thanks, this helps me understand the goals of this approach and where you want to go with this. Anyway, I think I've voiced my concerns and I don't think further discussion is needed at this point (at least I don't have more to add). I'm happy to defer to Alex on these UI questions and how we roll out the changes to users.

Comment 48

[Lasana Murray]

Attachment: part1-dialog-change.patch

Fixed spacing and only focuses accept button on readOnly windows.

Comment 49

[Lasana Murray]

Thanks Paul!

Ran this on try-comm-central and I see some failures related to calendar but some are saying they may be due to bug 1611307.

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

I'll try on my desktop again.

Comment 50

[Lasana Murray]

These fail on try-comm-central but pass on my local machine. Should I go ahead and request check-in?
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308071468&repo=try-comm-central

Comment 51

[Magnus Melin [:mkmelin] (away until July 8)]

comm/calendar/test/browser/eventDialog/browser_alarmDialog.js seems to fail on all platforms in the try, so that looks like a real failure

Comment 52

[Lasana Murray]

Attachment: error.png

Looks like something changed in calendar recently that rendered the way I did this patch wrong. I get this error message when I try to save changes to an event.

Investigating...

Comment 53

[Lasana Murray]

These fail on try-comm-central but pass on my local machine. Should I go ahead and request check-in?
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308071468&repo=try-comm-central

Comment 54

[Lasana Murray]

Ignore the above comment.

Comment 55

[Lasana Murray]

The attempt to save changes to an existing event fails here:
https://searchfox.org/comm-central/source/calendar/providers/storage/CalStorageCalendar.jsm#476

Comment 56

[Lasana Murray]

Attachment: bug1575195-part2.patch

I figured it out. There is a listener configured at the module level for the "dialogaccept" event. It is also fired when the edit button is clicked. I added a guard to not run if the event is not an invitation.

Comment 57

[Lasana Murray]

Here is my try run. Failed tests do not appear to be related to these patches.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c5a77e6f81f544c8f802d5a7841e095732859a44

Comment 58

[Paul Morris [:pmorris]]

Comment on attachment 9161853 [details] [diff] [review]
bug1575195-part2.patch

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

Good find!  Looking at this again, I think that, rather than having a "dialogaccept" listener that's always listening but only doing things in some cases, it would be worth refactoring so that we have different event listener functions that are used for each use case (readonly, invitation, click-to-edit).  Then they could all be set up in one place (which is easier to follow), like in the new update button function (which is now doing more than just showing/hiding buttons).  You could listen to either the "dialogaccept" event or the accept button "command" event.  I'm not sure if there's a reason to prefer one over the other, but probably better to use one or the other.

Comment 59

[Lasana Murray]

Attachment: part4-resolve-double-dialogs.patch

updateAvailableDialogButtons -> updateDialogAcceptButton
Moved the "acceptDialog" listeners into this function. Got rid of the seemingly obsolete use of dispose() .

Comment 60

[Lasana Murray]

Here is my try run. I don't think the failures are due to this patch. Things seem to work locally for me.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fbceef2fa4434030132bdae933c2946988f9a423

Comment 61

[Paul Morris [:pmorris]]

Comment on attachment 9162286 [details] [diff] [review]
part4-resolve-double-dialogs.patch

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

Changes look good and it worked when I tested a readonly event, a repeating event, and a non-repeating event.  There are a number of follow-ups to do, likely in separate bugs:

- For UX, for repeating events, offer not just "edit" but "edit this occurrence" and "edit all occurrences" buttons in this dialog, and drop the extra step of clicking through the separate dialog.  I'd say this is the top priority.
- For regular non-read-only events, reminders seems to be editable because there is a drop down menu for them, but there is no way to save a change.  We should either make editing and saving reminders possible from the summary dialog, or don't offer the menu.
- For UX, in the context menu when you right-click on an event in a calendar view we should add an "Edit..." item that opens the edit dialog directly.
- Now we aren't calling `dispose()` (because the job arg on the window appears to be unused), we should update the functions that open this dialog to not take a job argument and to not set that arg (as null) on the dialog window, etc.

I can understand wanting to go ahead and land this, but I think it would be best to do the first of the follow-ups before landing this (i.e. land it with this at the same time) to avoid degrading the UX for editing repeating events.  If we don't do that then I would prioritize getting that follow-up done as soon as possible after this lands.  That's just my two cents.

Comment 62

[Lasana Murray]

Agreed, I'll create issues for the other items later today.

Comment 63

[Lasana Murray]

These patches need to be applied in this order otherwise hg import is going to complain:

1. part1-dialog-change.patch
2. part2-update-existing-tests.patch
3. part3-test.patch
4. part4-resolve-double-dialogs.patch

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/25f70d318972
Open event summary instead of edit dialog by default. r=pmorris
https://hg.mozilla.org/comm-central/rev/389a5c299a13
Update tests that fail due to edit dialog change. r=pmorris
https://hg.mozilla.org/comm-central/rev/9ec81bf588c4
Add test to ensure opening event uses summary dialog. r=pmorris
https://hg.mozilla.org/comm-central/rev/252127cb7d25
Prevent double save attempt when edit button clicked. r=pmorris DONTBUILD

Comment 65

[Richard Leger]

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

Quite often I need to check the details of an event to check description, location, and rsvp statuses. If it's an editable event that always pops up the Edit Event dialog.

A better flow for this would be to open the event summary and have an edit event button in there (since editing is a secondary action for an existing event). This is also similar to how e.g. Google Calendar and others work.

Hi Magnus,

As reported here https://thunderbird.topicbox.com/groups/ux/T73a2c61477f63319/in-tb-81-0b2-64-bit-no-longer-able-to-edit-caldav-event-from-double-clik-on-event-in-calendar-view the current behaviour in TB 81.0b2 on Windows is:

• single click on event = event selected
• double click on event = open pop-up to read-only event data + Edit button

==> This is counter productive as it require an extra click in addition of double-click to edit an event!
When you edit a lot of event per day it is really annoying to have additional extra steps every time that was non-existant in previous TB version...

Previous TB behavior:

• single click on event = event selected
• double click on event = open event edit window

Current behaviour in Gmail:

• single click on event = event selected + pop-up (on the side) to read-only some event data
• double click on event = open event edit window

So current behaviour in TB 81.0b2 neither match previous TB behaviour nor current Google Calendar behaviour while being quite cumbersome!

My suggestion would be (Google like behaviour):

• single click on event = event selected + pop-up (on the side) to read-only some event data (still allowing selected event to be dragged & dropped)
• double click on event = open event edit window

Though I am not totally convinced that copying Google Calendar behaviour is best here... it would need to be confirmed but in Outlook event are open in editable mode with links underlined and clickable as links while still allowing them to be editable... (to be confirmed)...

Alternatively my personal view is that you don't really need this new feature "pop-up to read-only some event data" you could simple make links in event edit windows either clickable by converting them into pills (which could still be editable) or providing a link button next to it to click on it and reach the target...
This way you also have the advantage to have all the information of the event available to read as opposed to the pop-up that show only very small quantity of information...

This secondary approach may be better and simpler to use and maintain... than to have two type of view: one read-only (pop-up) and one for edit (event edit window)...

Regards,
Richard

Comment 66

[Magnus Melin [:mkmelin] (away until July 8)]

(In reply to Richard Leger from comment #65)

==> This is counter productive as it require an extra click in addition of double-click to edit an event!
When you edit a lot of event per day it is really annoying to have additional extra steps every time that was non-existant in previous TB version...

Why are you editing the events? Usually an event is set up once and then you go to it to get the information out when you're about to participate.

Comment 67

[Alessandro Castellani [:aleca]]

An event preview popup is something that have been considered, and we will probably implement in a future iteration.

This initial update, as discussed in the bug, is to accommodate the most common behaviour when interacting with a calendar invite, which is to look at the invite to copy/click on links, phone numbers, etc.

You might need to edit a lot of events at once, but that's mostly an edge case as I doubt you or any other user will need to edit multiple events per day every day.

Comment 68

[Richard Leger]

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

(In reply to Richard Leger from comment #65)

==> This is counter productive as it require an extra click in addition of double-click to edit an event!
When you edit a lot of event per day it is really annoying to have additional extra steps every time that was non-existant in previous TB version...

Why are you editing the events? Usually an event is set up once and then you go to it to get the information out when you're about to participate.

I think you under estimate use cases of editing events...

Because I create and modify a lot of meetings per day that often need to be amended either title, dates, time, reminder, body content, location, attachment... as they are cancelled extended, reduced, moved etc... there are also use to take notes/key-points before, during meetings or after... for example so I do edit a lot of events every day... I am not sure how to describe better... same goes for Tasks evidently :-)

Event are not necessarily just meeting you attend or be reminded about... that are just created once and only read... they can be all sort of events... you can do much more with them... some workflow of end-user like me requires a lot of event editing so Editing shall be as equally important and easily accessible as reading details of the event...

Personally I still think that the Edit event windows via double click is sufficient to do both ReadOnly or Read&Write, just makes links clickable/editable in the Edit window if that issue you are trying to sort... with the same information being readable/writeable in the same place... to keep it very simple! Some user users, add-ons or online services may be a link in location to a Zoom,WebEx, etc... meeting/webinar event in that case it would be handy to have the link clickable to easily join the meeting on the day...
This way you also have one UX to handle/improve... (for example you could add quick option to change priority or other event options hidden in menus... especially useful for task... but not related to this bug...)

Mouse over the event already provide some info about the event maybe that could be improved instead or reformatted? I don't know...

But if you really want to implement single click to see some event data in a pop-up, fine perhaps, I don't really see the usefulness but I understand others may do... though don't do it just because Google Calendar has it!
Simply:

• don't remove the current behaviour "double click to edit event" which is the current TB behaviour that all users are currently accustomed to...
• make sure it does not affect drag&drop of event in calendar view
• allow end-user to disable the single click pop-up behaviour via pref if they don't like it... I know I don't but I am open to try as far as double click to edit remain available as it always worked (and that how Google Calendar works!) and analyse how many users use that preference via telemetry that should give an idea of the pertinence to have it enabled by default or not...

With regards to recurring events, currently if you double click on an event you are prompted and asked to edit only this occurrence or all of the occurrences... I like this feature and use it as well... it would be nice to keep on a double click... I prefer it as is when you open, than Google at Save time which always make me feel I did something wrong while editing the event :-)

By curiosity were there a lot of end-user in the community requesting such changes? I mean has anyone complained that double clicking on Event to View/Edit has been an issue? If not, I really don't see any reason to remove such legacy behaviour...

Also a side effect of new behaviour if you click an event in the intention to drag&drop it, you activate pop-up windows, which is not necessarily what end-user want to... the intention being just to drag&drop the event on calendar view... just an additional thought... would the pop-up move with the event or disappear in that case (during the move) and reappear once set in place?

Don't broke what ain't broken, just fix what's really needed :-)
Still waiting on critical regression such as Bug 1642292 to be fixed, it would be much better time spend than this non-critical bug that is counter productive for some end-user out there!

Comment 69

[Alessandro Castellani [:aleca]]

With regards to recurring events, currently if you double click on an event you are prompted and asked to edit only this occurrence or all of the occurrences... I like this feature and use it as well... it would be nice to keep on a double click... I prefer it as is when you open, than Google at Save time which always make me feel I did something wrong while editing the event :-)

This has always been super annoying for the whole team and it was one of the main reasons to change it.

By curiosity were there a lot of end-user in the community requesting such changes?

We also implement features and change behaviours based on our experience and proposals, not only by community feedback.

Also a side effect of new behaviour if you click an event in the intention to drag&drop it, you activate pop-up windows, which is not necessarily what end-user want to...

The popup is not implemented yet, so this behaviour doesn't present. Anyway, in case of a drag action, we would hide the popup so this is a non-issue.

Don't broke what ain't broken, just fix what's really needed :-)

Isn't "don't fix what ain't broken"?
Anyway, the 68 behaviour wasn't optimal for the whole team.
This new behaviour is just on beta and it's only the first step to improve the area, therefore we're gathering feedback and defining the next steps.

Thanks for your detailed feedback.

Comment 71

[[e-mail]]

At this point, if you have a recurring invitation already in your calendar, the only way to edit the series (e.g. to change the reminder) is to hold down the Ctrl key while double-clicking the event. That's assuming that you remember that it's a series, because there doesn't appear to be any obvious indication of that any more, but maybe I just don't know the product well enough?

Comment 72

[Richard Leger]

While the ability to click a link to join an online meeting is quite enjoyable, after weeks of using beta and this new feature my brain muscle still does not adjust to the fact that to edit an event I have to read it first! It remain annoying every time... is there any chance that the double click will be returned, to directly edit an event, before the next ESR?

Comment 75

[[e-mail]]

Comment on attachment 9160129 [details] [diff] [review]
part1-dialog-change.patch

```diff
>+function viewSelectedEvents() {
>+  let items = currentView().getSelectedItems();
>+  if (items.length >= 1) {
>+    openEventDialog(items[0], items[0].calendar, "view");
```

Unfortunately this call is incorrect as it takes four parameters; it should be `openEventDialogForViewing(items[0])` instead. The upshot is that you cannot change the reminder time on an invitation that you opened by double-clicking (it still works if you use the "Edit" button to open it).

Comment 76

[Magnus Melin [:mkmelin] (away until July 8)]

@neil: I can't really reproduce that problem