1515959 - Using up/down keys to select reminder in the custom reminder dialog modifies the reminder

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Stefan Sitter [:ssitter]]

Attachment: customreminder.png

Thunderbird 66.0a1 (BuildID 20181221092739) with Lightning 6.8a1

History:
Create event and setup multiple reminders in the custom reminder dialog
Save event and close it
Reopen the custom reminder dialog
Use the up/down arrows on the keyboard to change the selected reminder

Error:
The settings from the initially selected reminder are copied to all reminders, see screenshot.
I'd expect that only the selection changes without modifying the reminder.

Comment 1

[Stefan Sitter [:ssitter]]

Same problem using Thunderbird 64.0b4 (BuildID 20181129151909) with Lightning 6.6b4.

Comment 2

[[:MakeMyDay]]

Looks like the detail controls are not updated when changing the list entry but its value is updating the list entry on blur. Changing the entries by clciking in the list works fine.

Geoff, is this a regression of the richlistbox conversion?

Comment 3

[Geoff Lankow (:darktrojan)]

Yeah, I'd say it could be. I'll come back and have another look at this when I get some time.

Comment 4

[Geoff Lankow (:darktrojan)]

Nope, it's a regression from the XBL to CustomElement conversion. Colour me not surprised at all.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1515959-alarms-dialog-1.diff

Two bugs for the price of one!

I discovered while testing that the alarm dialog doesn't respect the prefs calendar.alarms.(event, todo)alarmunit. So I fixed that too.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9033740 [details] [diff] [review]
1515959-alarms-dialog-1.diff

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

Sold! I do have some bargaining points though:

::: calendar/base/content/dialogs/calendar-event-dialog-reminder.js
@@ +254,5 @@
>      let listbox = document.getElementById("reminder-listbox");
>      let listitem = listbox.selectedItem;
>  
>      if (listitem) {
> +        suppressListUpdate = true;

Maybe wrap this in a try/finally to make sure it is unset if something wrong happens and doesn't bork alarm display.

@@ +395,5 @@
>      // Default is a relative DISPLAY alarm, |alarmlen| minutes before the event.
>      // If DISPLAY is not supported by the provider, then pick the provider's
>      // first alarm type.
>      let offset = cal.createDuration();
> +    if (alarmunit == "days") {

Can we use cal.alarms.setDefaultValues() instead?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/40bfc93a1dcc
Fix reminder dialog to respect default duration, and not overwrite existing reminders when changing the UI; r=Fallen

Comment 8

[Geoff Lankow (:darktrojan)]

I didn't use setDefaultValues as that creates a new alarm and adds it to the event/task, and we don't want that.