1617786 - Adapt the UI when users are not using Calendar

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Paul Morris [:pmorris]]

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

According to the plan worked out in bug 1592987, the calendar UI will adapt when users are not using calendar functionality. Namely, the UI will adapt when users have all calendars disabled (i.e. calendar properties -> uncheck "switch this calendar on"). This bug is to implement this functionality. Here's the current plan for how various UI items will adapt:

NO CHANGE

• "Create a new calendar" link on account central page
• calendar and task buttons in the tabs bar (they may be relocated eventually)

HIDDEN

• today pane
• today pane button in the status bar
• "Events and Tasks" menu

SOME ITEMS HIDDEN, SOME ITEMS VISIBLE AND USABLE

• menu items in "File", "Edit", etc.
• menu items in context menus
• keyboard shortcut keys (some disabled, others still work, following the corresponding menu items)

SPECIAL CASES

• IMIP bar ("this message contains an invitation...")
  Always show this notification to allow users to quickly add the event. If calendar code is disabled, include an option for "don't offer to add any more calendar events".
• Calendar, Tasks, Event, and Task tabs
  Basically unchanged, but if calendar is unused, show a message to explain the current status, and provide a smooth path for users to start using calendar features.

Comment 1

[Paul Morris [:pmorris]]

Attachment: wip-calendar-deactivator-0.patch

Uploading a WIP patch for early feedback on the general approach.

With this patch, when all calendars are disabled (right click on calendar in calendar list, select "Properties", uncheck "switch this calendar on"), the today pane, today pane button, "Events and Tasks" menu (in both menu bar and app menu) are hidden. The today pane's per tab shown/hidden state is restored when re-enabling a calendar.

Next step is to add menu items and keyboard shortcut keys. I may handle the "special cases" (see previous comment) separately from the rest.

Comment 2

[Geoff Lankow (:darktrojan)]

Comment on attachment 9128680 [details] [diff] [review]
wip-calendar-deactivator-0.patch

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

What if the deactivator was in a JSM and handled the enabling/disabling events once for all windows? A bit like the colour tracker in calViewUtils.jsm.

You could also do the hiding by CSS – just set an attribute on the root element and have rules that match it – which would make it independent of the logic.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9128680 [details] [diff] [review]
wip-calendar-deactivator-0.patch

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

I like this approach, just a tiny suggestion on a condition.
I know this is not a review, so apologies if my comment was annoying.

::: calendar/base/content/calendar-deactivator.js
@@ +54,5 @@
> +        this.idsToHide.forEach(id => (document.getElementById(id).hidden = false));
> +      } else {
> +        this.idsToHide.forEach(id => (document.getElementById(id).hidden = true));
> +      }
> +    }

This looks a bit verbose.
What if we interrupt if the variables match?
```
if (someCalsEnabled == this.mIsActivated) {
  return;
}
```

Also, we should use `for` instead of `forEach` as it's drastically faster.
It probably doesn't matter much since we're looping through and array of strings anyway, and not nodes, but you could do something like this:
```
for (id of this.idsToHide) {
  document.getElementById(id).hidden = someCalsEnabled;
}

Comment 4

[Paul Morris [:pmorris]]

Attachment: wip-calendar-deactivator-1.patch

Thanks Geoff and Alex for the helpful and timely feedback.

(In reply to Geoff Lankow (:darktrojan) from comment #2)

What if the deactivator was in a JSM and handled the enabling/disabling
events once for all windows? A bit like the colour tracker in
calViewUtils.jsm.

You could also do the hiding by CSS – just set an attribute on the root
element and have rules that match it – which would make it independent of
the logic.

I've done both these now and it's much better, thanks. I initially tried a JSM but retreated since you can't access the window/document directly from a JSM. Following the approach in calViewUtils.jsm makes that a non-issue.

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

This looks a bit verbose.
What if we interrupt if the variables match?

I generally prefer to avoid that return early pattern (it's simpler if functions have a single return rather than multiple), but we use it a lot, so it probably makes sense here.

Also, we should use for instead of forEach as it's drastically faster.

Yeah, switched to using for in one place (also because calendars is now a set rather than an array), although using CSS now avoids that loop entirely.

Comment 5

[Paul Morris [:pmorris]]

Attachment: calendar-deactivator-2.patch

Ready for review. This handles the most obvious things, hiding them when all calendars are disabled:

HIDDEN

• today pane
• today pane button in the status bar
• "Events and Tasks" menu

SOME ITEMS HIDDEN, SOME ITEMS VISIBLE AND USABLE

• menu items in "File", "Edit", etc.

I didn't find any menu items in context menus that needed to be hidden. 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.)

Anyway, I've left out disabling commands for now, and I'd like to handle that and other items in a follow-up so we can go ahead and get the basics in place.

Try run just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74406d0abc59360a2852b95ae042b7c6994aa091

Comment 6

[Geoff Lankow (:darktrojan)]

Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.

I didn't find any menu items in context menus that needed to be hidden.

What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.

Comment 7

[Paul Morris [:pmorris]]

(In reply to Geoff Lankow (:darktrojan) from comment #6)

Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.

That makes sense to me. Funny enough, it's what I had started to do in the previous version of the patch, with a 'hide-when-calender-deactivated' class on the various elements. I asked Paenglab about it in chat earlier today and he steered me in the current direction. Here's his comment (I had used a "calendar-disabled" class rather than an attribute):

How about using an attribute on the window? Then you could use :root[calendar-disabled] selector {}. Not sure but this could be probably faster than the class. We use this a lot for theme things. And instead of using the hide-when-calendar-deactivated class, hide directly the element with the :root[calendar-disabled] selector {}.

(In reply to Geoff Lankow (:darktrojan) from comment #6)

What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.

Oh good catch, I missed it. I think that would be a good candidate for hiding. I just checked and found that it is disabled when all calendars are disabled.

Comment 8

[Richard Marti (:Paenglab)]

It was only a proposal. I'm also okay with the class.

Comment 9

[Worcester12345]

(In reply to Geoff Lankow (:darktrojan) from comment #6)

Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.

I didn't find any menu items in context menus that needed to be hidden.

What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.

Should not trade off end user speed for ease of use to programmers. The ratio is just too far off, especially if the programmers are also users. Must do everything possible to improve and NOT degrade the speed.

Comment 10

[Paul Morris [:pmorris]]

Attachment: part1-avoid-ltn-prefix-0.patch

Removes the use of 'ltn' as a prefix in element ids outside of the calendar directory. This will make it easier to identify calendar UI elements in mail code outside of the calendar directory. Eventually it would be good to remove all uses of 'ltn' but there are quite a few (looks like between 100 and 200 for use in ids).

Comment 11

[Paul Morris [:pmorris]]

Attachment: part2-calendar-deactivator-3.patch

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

It was only a proposal. I'm also okay with the class.

Okay, thanks! Now using a class (rather than ids) to target the elements that are hidden when calendar is deactivated. Now also hides the mailContext-calendar-convert-menu menuitem.

(In reply to Worcester12345 from comment #9)

Should not trade off end user speed for ease of use to programmers.

I'd expect that in this case the difference in speed would be negligible.

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 9129872 [details] [diff] [review]
part2-calendar-deactivator-3.patch

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

::: calendar/base/modules/calCalendarDeactivator.jsm
@@ +92,5 @@
> +  // calICalendarManagerObserver methods
> +  onCalendarRegistered(calendar) {
> +    this.calendars.add(calendar);
> +
> +    if (!calendar.getProperty("disabled")) {

You could skip this check, if isCalendarActivated is true. Alternatively you could always refresh the state. Using only one of the things you already know here, while not wrong, seems a bit odd. Do you see what I mean?

::: mail/components/customizableui/content/panelUI.inc.xhtml
@@ +163,5 @@
>                         class="subviewbutton subviewbutton-iconic"
>                         label="&newNewMsgCmd.label;"
>                         key="key_newMessage2"
>                         command="cmd_newMessage"/>
> +        <toolbarbutton id="appmenu_calendar-new-event-menu-item hide-when-calendar-deactivated"

That's not right.

Comment 13

[Paul Morris [:pmorris]]

Attachment: part1-avoid-ltn-prefix-1.patch

I did a try run, which looks fine. The failures are due to the unrelated "test_sort_orders.js" issue.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eada8332bbf623c62f0ab125b129f40d2b9bd6d3&selectedJob=291272101

This patch fixes the linting error flagged in the try run.

Comment 14

[Paul Morris [:pmorris]]

Attachment: part2-calendar-deactivator-4.patch

Thanks for the review. Makes sense, and good catch. This fixes those two things.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/77c269c63ddc
Avoid using 'ltn' prefix outside calendar directory. r=darktrojan
https://hg.mozilla.org/comm-central/rev/46b1f5963bc7
Hide part of calendar UI when all calendars are disabled. r=darktrojan