Closed Bug 1621791 Opened 5 months ago Closed 5 months ago

Disable some calendar keyboard shortcuts when calendar is not used

Categories

(Calendar :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(3 files, 2 obsolete files)

+++ 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.)"

I'd suggest wontfix for this.

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.

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

Attachment #9133346 - Flags: review?(geoff)

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.

Attachment #9133347 - Flags: review?(geoff)
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.
Attachment #9133346 - Flags: review?(geoff) → review+
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.
Attachment #9133347 - Flags: review?(geoff) → review+

Addresses review comments.

Attachment #9133346 - Attachment is obsolete: true
Attachment #9133707 - Flags: review+

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.)

Attachment #9133347 - Attachment is obsolete: true
Attachment #9133708 - Flags: review+

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

Attachment #9133710 - Flags: review?(geoff)
Status: NEW → ASSIGNED
Attachment #9133710 - Flags: review?(geoff) → review+

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 76
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.