1621791 - Disable some calendar keyboard shortcuts when calendar is not used

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Paul Morris [:pmorris]]

+++ This bug was initially created as a clone of Bug #1621130 +++

Following up on bug 1617786, disable any keyboard shortcuts that should be disabled. Here's a relevant bit from the 5th comment on that bug.

"For keyboard shortcuts I found a couple <command>s to disable ("calendar_toggle_todaypane_command", "calendar_go_to_today_command"). Luckily the toggle_todaypane keyboard shortcut is already getting disabled without any extra effort required. The go_to_today one is not, but it's tricky because even if you disable it, when you open a calendar tab it gets re-enabled. (Disabling it is pretty low priority. If a user accidentally hits alt+end in a mail tab a calendar tab will open, which isn't so bad but not ideal.)"

Comment 1

[Magnus Melin [:mkmelin]]

I'd suggest wontfix for this.

Comment 2

[Geoff Lankow (:darktrojan)]

At the very least, calendarUpdateNewItemsCommand needs to be called when a calendar is enabled or disabled (this probably should already happen, but doesn't) because if Thunderbird starts without an enabled calendar and one becomes enabled, the commands are still disabled. At a quick glance I'd say here is the right place.

Comment 3

[Paul Morris [:pmorris]]

Attachment: part1-update-new-item-commands-0.patch

This handles calling calendarUpdateNewItemsCommand when a calendar is enabled or disabled.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part2-synchronize-calendars-command-0.patch

I noticed that the "Synchronize" toolbar button/command was not being disabled when all remote calendars were disabled. This patch basically fixes that, but a glitch remains.

When all remote calendars are disabled, and then you enable one, the command is updated immediately. Going the other way, when you disable the last one that is enabled, the update is not immediate but does eventually take effect, like when you switch away from the calendar tab and then back to it. I haven't had a chance to dig into how to fix that part yet.

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 9133346 [details] [diff] [review]
part1-update-new-item-commands-0.patch

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

::: calendar/base/content/calendar-command-controller.js
@@ +897,5 @@
>      "calendar_new_todo_context_command",
>      "calendar_new_todo_todaypane_command",
>    ];
>  
> +  commandsToUpdate.forEach(goUpdateCommand);

Nice tidy-up. You could forget the variable and put the .forEach directly on the array.

::: calendar/base/content/calendar-management.js
@@ +624,5 @@
> +    // Update commands when a calendar is enabled or disabled.
> +    if (name == "disabled") {
> +      calendarUpdateNewItemsCommand();
> +    }
> +  },

This is a bit nit-picky, but I'd rather keep these methods in the order they're declared in calICalendar.idl. I'd accept either onPropertyChanged and onLoad put back with the others, or both here but with onPropertyChanged after onLoad. Your call.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9133347 [details] [diff] [review]
part2-synchronize-calendars-command-0.patch

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

Surely the fix is to add calendar_reload_remote_calendars to the updated commands in the other patch.

::: calendar/base/content/calendar-command-controller.js
@@ +571,5 @@
>     * Returns a boolean indicating if all calendars are local
>     */
>    get no_network_calendars() {
>      return cal.getCalendarManager().networkCalendarCount == 0;
>    },

This is now unused and could be removed.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part1-update-new-item-commands-1.patch

Addresses review comments.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part2-synchronize-calendars-command-1.patch

Addresses review comments. It seemed worthwhile to update all the commands when a calendar was enabled or disabled, enabling/disabling is not a high frequency occurrence, and this approach will be more future-proof.

(I did discover the reason for the asymmetrical behavior I was seeing. When a network calendar is enabled the onLoad function is called (and commands are updated), but there's no corresponding call when disabling a networked calendar. Also onLoad is not called when a local calendar is enabled.)

Comment 9

[Paul Morris [:pmorris]]

Attachment: part3-use-a-set-for-commands-list-0.patch

Some more tidying up while I was at it. Use a Set instead of an Object-used-like-a-set to store the command lists (now that JS has Sets).

Try runs (look fine):

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad24a3a2ef9eec1de4cc7e3ce6123723cc73c297&selectedJob=293386292

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bd92c86971d7
Update new item commands on calendar enable/disable. r=darktrojan
https://hg.mozilla.org/comm-central/rev/f02ab1a74c31
Improve disabling of synchronize calendars command. r=darktrojan
https://hg.mozilla.org/comm-central/rev/49d25b882f41
Store calendar commands in a Set not an Object. r=darktrojan DONTBUILD