1130852 - Add opt-out notification for calendar integration

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[[:MakeMyDay]]

To promote Lightning as shipped part of Thunderbird we need a dialog to enable the user to opt-in for calendar integration as discussed in the last status meeting. See also the etherpad [1] where we collected the different options and the duties to ship Lightning with Thunderbird.

[1] https://calendar.etherpad.mozilla.org/thunderbird-integration

Comment 1

[[:MakeMyDay]]

Attachment: OptInDialog.png

Comment 2

[[:MakeMyDay]]

Attachment: LightningOptInDialog-V1.diff

The proposed dialog is intented to be only displayed once after upgrade / creating a new profile. If the user decides to use the calendar, Lightning will be enabled and TB gets restarted to make it effectively available. If the checkbox in the dialog was ticked, the new calendar wizard should show up after restart.

This is not yet intended for review but any feedback would be greatly appreciated.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 8561014 [details] [diff] [review]
LightningOptInDialog-V1.diff

Your patch doesn't work here. I get a totally broken UI. Here are the errors:

Sun Feb 08 2015 17:20:48
Error: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
Source file: chrome://messenger/content/msgMail3PaneWindow.js
Line: 20
 ----------
Sun Feb 08 2015 17:20:49
Error: TypeError: TabsInTitlebar is undefined
Source file: chrome://messenger/content/msgMail3PaneWindow.js
Line: 411

But also f- when I'm using your screenshot.
The text in the dialog is only mentioning the Finish (Just opt-in to use it by clicking on Finish button) and the Cancel (If you click on Cancel button instead, the calendar keeps disabled) buttons. Nothing about the checkmark.

Wouldn't it be better to remove the Cancel button (and if possible the disabled Back button) and give in the text more focus to the checkmark? Finish will close the dialog in either state the checkmark has.

Comment 4

[[:MakeMyDay]]

Attachment: LightningOptInDialog-V2.diff

Thank you for your feedback, Richard. I've updated the patch considering your comments. Eventually the patch still has some nits, I have had only time to test the dialog part yet.

Comment 5

[[:MakeMyDay]]

Attachment: OptInDialog-v2.png

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Optin dialog - v3

Personally I think there is too much text in the dialog, I think we should try to shorten it. This can be done by using two buttons, which also reduces it by one click.

The exact wording is pending our decision on how to integrate it, but unless we go for options E or F in [1] it will still be an addon. Lightning doesn't really act differently than before other than us being more aggressive about installing it. I think we should stay more neutral in the text, neither saying its an addon nor saying that its now integrated. 

It might also be nice to add a poster screenshot to lure the users in. I've attached a quick mockup. I didn't put much thought into the text aside from adhering to what I mentioned above, its not meant to be final of course.

What do you think?

Comment 7

[Magnus Melin [:mkmelin]]

I'd agree there shouldn't be too much text.
In general dialogs aren't that liked though. Have you considered something like the "Know your rights" notification bar that's appears on the bottom of the window? That also leaves more time for the user to consider the decision. 

---------------------------------------------------------------------------------
| Lightning offers calendar integration in Thunderbird.     [Enable Lightning]  |   
---------------------------------------------------------------------------------

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8561104 [details]
Optin dialog - v3

Yes, that would be the clearest way for the user.

Comment 9

[[:MakeMyDay]]

Comment on attachment 8561104 [details]
Optin dialog - v3

A more short text is for sure an option, but we should consider to make it obvious that this option is different from the add-on promotion in the get addons section of the addon manager and that the dialog is a one-time shot. The final for sure depends on the final approach of integration. If we use images, these should be OS dependent.

Beside text, images and buttons, we should also think about the right place - in terms of user workflow - for the dialog. Should we hook in add the end of the initiual account creation? Should we offer to open the calendar wizard after Lightning was enabled?

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Another thing we could do is add "integration" into the new account wizard. Something like "Also set up calendar". When checked, an extra message could show up that says that Lightning will now be installed and Thunderbird will restart.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Just a few days left until string freeze. We have to make a decision on how to proceed here. We can decide this in the Meeting on Tuesday, is there anything left to discuss before we get there?

Comment 12

[[:MakeMyDay]]

I think we need to decide on 3 items to determine to proceed here:

1. What is the way to go for integration?
2. Should there be a trigger to start calendar wizard after enabling?
3. Where's the best place to inject the dialog?

We need at least 1. and 2. decided to get the strings ready before the merge.

Comment 13

[aleth [:aleth]]

(In reply to Magnus Melin from comment #7)
> I'd agree there shouldn't be too much text.
> In general dialogs aren't that liked though. Have you considered something
> like the "Know your rights" notification bar that's appears on the bottom of
> the window? That also leaves more time for the user to consider the
> decision. 

Another option would be to use the start page area to show this dialog, i.e. replace the start page with an "in-content dialog" until the user makes a decision/clicks 'OK'. This would make it a bit more visible and informative than a footer and a bit less "in your way" than a dialog would be.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

> 1. What is the way to go for integration?

I was about to decide for option C+D, but I have a plan...

== Option G ==

I've come up with a new option G. Its like C, but we package Lightning's extension in Thunderbird, as a plain xpi in dist/bin/lightning.xpi (or similar). This xpi is obviously not auto-installed because its neither in distribution/ nor extensions/. It just sits there waiting to be installed.

Then, we implement the install on demand dialog from C. It doesn't have to download Lightning from the internet, but uses the packaged copy and will therefore always have a working Lightning available.

Also, we extend bug 1133139 to not only mention there is a problem, but also offer to install the Thunderbird-provided version to remedy the situation. With this in place, we have no need to package the Lightning binaries with Thunderbird (option D) because there will always be an option to install a compatible Lightning.

The above covers the first-install case and almost all problems encountered during updates. In the standard case where there is no trouble during upgrade, the AMO updates of Lightning will install the correct version.

== Problems ==

One issue not solved is if there is a problem during upgrade/downgrade, Lightning may be disabled. We could go a step further and in case we detect its disabled, ask if it was intentional. Then offer to install a working version and install the packaged lightning.xpi if the user says yes. If the user has explicitly opted out in the past, don't show that dialog. We might get away with just leaving this out for this release.

Another potential issue is packaging, the final lightning.xpi with all locales and platforms is not available at build/repack time. Possibly its enough to install the per-platform/per-locale xpi that we have available after the build/repack though.

== Other comments ==

> 2. Should there be a trigger to start calendar wizard after enabling?
I don't think we should change this right now, we can revisit this when we integrate with the Thunderbird autoconfig dialog.


> 3. Where's the best place to inject the dialog?
Presumably the same place where the Thunderbird new account dialog is started.


> Have you considered something like the "Know your rights" notification bar ...
I'm not convinced about a more subtle option like the bar at the bottom, many will not notice it, so I think it should be a dialog.


Going through the remaining options and the main reason I think we shouldn't do it:

A) Disabled addons get re-enabled on upgrade, users not wanting Lightning will be annoyed.
B) Will eventually go away, also profile-installed Lightning is preferred.
E) Too many changes needed, harder to share between Seamonkey/Postbox/Thunderbird
F) Looks nicely integrated, but profile-installed Lightning is preferred.


== Next steps ==

* Tell me if my plan has a flaw
* WONTFIX bug 1133986 which I had just filed.
* Agree on strings for the dialog in this bug, and the revised strings for bug 1133139
* Work on the auto-installing part of the dialog
  - I have some code that goes to AMO and downloads and installs Lightning. This can be
    adapted to install from a local location.
* Figure out how to package the Lightning xpi with Thunderbird

Thoughts? Volunteers?

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Oh one more thing: given the dialog UI is part of Thunderbird, I can't make the final call on that. rkent and magnus wanted to get together and think about how it should be presented in Thunderbird. They are both not really fond of an extra dialog. rkent suggested to add a checkbox to the new account wizard that would install Lightning once complete, magnus suggested the bottom bar above.

I'm open to the new account wizard thing, if we do that I might have second thoughts on what I said before on not showing the new calendar wizard after installation. It would magically fit that after you do the mail setup you can optionally also set up a calendar, then it will restart with Lightning installed and present the calendar wizard.

In the future we could add more calendar installation hooks this way, e.g being able to attach an event to an email message could trigger installing Lightning too.


We will hopefully have a final decision on the call on Thursday so we can move forward.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

aceman raises a valid point, that users that already have an account won't see the new account wizard and therefore not install Lightning. We'd need another hook for that case.

Comment 17

[[:MakeMyDay]]

G is an interesting option. Where would the xpi reside until its installation?

To consider: the Linux distros are not forced to follow the described packaging approach (even though this is true for some other options as well, the buulding/packaging requires more effort for the maintainer here). But maybe an according information would be enough for mitigation here.

Re the dialog, I agree that a too subtle approach would not be best option. Integration into the new account wizzard is a good idea, but not enough as already mentioned above. If we add it to the account wizzard we can seemlessly continue with the new calendar wizard - with the binaries in TB, this could be restartless(?). The only odd thing would be that we have already created a local calendar by default anyway, so may we could disable that default creation for this special case? For all the users who have already an account, we need a separate dialog, maybe without the calendar wizard option.

Last but not least a word to the current patch: it expects an installed but disabled Lightning atm.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to MakeMyDay from comment #17)
> G is an interesting option. Where would the xpi reside until its
> installation?
I would put this in some new subfolder of dist/bin or Contents/Resources/, maybe something like "extrepo" ?

> 
> To consider: the Linux distros are not forced to follow the described
> packaging approach (even though this is true for some other options as well,
> the buulding/packaging requires more effort for the maintainer here). But
> maybe an according information would be enough for mitigation here.
We could include a pref that controls if an attempt is made to install the packaged Lightning, maintainers could add this pref to their package if they want to disable it and use their own mechanism. I have a few maintainers emails, I'll send them a heads up when the patch is ready.


> 
> Re the dialog, I agree that a too subtle approach would not be best option.
> Integration into the new account wizzard is a good idea, but not enough as
> already mentioned above. If we add it to the account wizzard we can
> seemlessly continue with the new calendar wizard - with the binaries in TB,
> this could be restartless(?).
Unfortunately not, there is much more needed to make an addon restartless. We can't use xul overlays, we have to bootstrap everything from bootstrap.js and also manually remove all traces when uninstalled.

> The only odd thing would be that we have
> already created a local calendar by default anyway, so may we could disable
> that default creation for this special case? 
Yes, we could probably also make sure that everything is correctly disabled when there are no calendars, and augment a few commands like create_new_event_command to start the calendar wizard when there are no calendars. Then we could completely remove that restriction.


> For all the users who have
> already an account, we need a separate dialog, maybe without the calendar
> wizard option.
Yeah, maybe that is an option. To recap: if the user has no accounts (new profile) then don't show a dialog, but have it set up through the TB new account dialog. If the user does have accounts, show the on-demand dialog once. rkent, magnus, would that be an acceptable alternative?

Comment 19

[Merike Sell (:merike)]

Has it ever been considered to have a calendar button on main toolbar by default instead of annoying users with extra notifications and wizards? There is already a button to open calendar after Lightning is installed and enabled that can be added to toolbar. Before enabling calendar it could ask enabling calendaring when clicked. Anyone annoyed by extra button can remove it from toolbar. Sorry to mention it so late in the process but I don't remember seeing it mentioned anywhere I have read and I only had this idea today.

(In reply to Philipp Kewisch [:Fallen] from comment #14)
> Another potential issue is packaging, the final lightning.xpi with all
> locales and platforms is not available at build/repack time. Possibly its
> enough to install the per-platform/per-locale xpi that we have available
> after the build/repack though.
Per locale xpi would regress extraction functionality somewhat. It makes use of all locales it can find in currently released xpi so that users can extract events from emails in languages other than the one Thunderbird is in. It does not need full localization only the patterns for extraction but that probably wouldn't make packaging easier.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

We could give the packaged version of the xpi a version number like "4.0packaged". Then on the next addons update check, it would find the AMO update to 4.0. Packaging completely means running an automated job to pack it all up, then run another round of repack jobs that adds Lightning back to each localized Thunderbird build. Its not impossible, but it is surely not very standard and might be fragile.

Comment 21

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-LightningPart-V1.diff

In the call last Thursday it was decided to change the integration approach from opt-in to opt-out with a notification bar to enable the user to do so.

This patch provides a three button notification bar:

- confirm to keep Lightning and get rid of the notification bar
- opt out and remove Lightning
- learn more about Lightning (currently opening an external link pointing to https://support.mozilla.org/de/products/thunderbird/calendar)

The code is mostly Lightning code, for TB and SM there is only an additional preference added for each, which will also be available if Lightning is not installed anymore because the user opted out. This can be used to hook in if we need a mechanism to to prevent re-installing Lightning after the next update if the user opted out before.

In the call last Thursday it was decided to change the integration approach from opt-in to opt-out with a notification bar to enable the user to do so.

This patch provides a three button notification bar:

- confirm to keep Lightning and get rid of the notification bar
- opt out and remove Lightning
- learn more about Lightning (currently opening an external link pointing to https://support.mozilla.org/de/products/thunderbird/calendar)

The code is mostly Lightning code, for TB and SM there is only an additional preference added for each, which will also be available if Lightning is not installed anymore because the user opted out. This can be used to hook in if we need a mechanism to to prevent re-installing Lightning after the next update if the user opted out before.

Some feedback in general and especially on the strings is greatly appreciated as string freeze is about to come.

Comment 22

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-ThunderbirdPart-V1.diff

Comment 23

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-SeamonkeyPart-V1.diff

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8567630 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V1.diff

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

::: calendar/base/content/calendar-chrome-startup.js
@@ +62,5 @@
> +/**
> + * Display opt-out notification for calendar integration if appropriate
> + */
> +function calendarIntegrationNotification() {
> +    const kIntegration = "mail.calendar-integration.default";

the name of this should be something saying the user oped out.

@@ +64,5 @@
> + */
> +function calendarIntegrationNotification() {
> +    const kIntegration = "mail.calendar-integration.default";
> +    const kNotification = "calendar.integration.notification";
> +    const kSupportUri = "https://support.mozilla.org/de/products/thunderbird/calendar";

remove the /de/ part

@@ +74,5 @@
> +            let ltnBrand = cal.calGetString("lightning", "brandShortName", null, "lightning");
> +            let label = cal.calGetString("lightning", "integrationLabel", [appBrand, ltnBrand], "lightning");
> +            let buttons = [
> +                {
> +                     label:     cal.calGetString("lightning","integrationLearnMoreButton", null, "lightning"),

nit: space around comma

@@ +82,5 @@
> +                         Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
> +                                  .getService(Components.interfaces.nsIExternalProtocolService)
> +                                  .loadUrl(cal.makeURL(kSupportUri));
> +                        // to prevent closing notification bar, we need to throw an error
> +                        throw new Error('prevent nb close');

I believe you can just return true

@@ +109,5 @@
> +                                                      } catch (e) {
> +                                                          aAddon.userDisabled = true;
> +                                                      }
> +                                                  });
> +                        // let the user know that removal will be applied after restart

What do you think about opening the add-on manager if we end up here? 
That 
 * also tells the user if restart is required, and 
 * it make the user aware of how they would go about enabling/installing it again should they change their mind. 
 * lets you "undo" if you clicked the wrong button accidentially

@@ +128,5 @@
> +                                                     "restart-required",
> +                                                     null,
> +                                                     notifyBox.PRIORITY_INFO_MEDIUM,
> +                                                     button);
> +                        notification.persistence = 3;

I don't think you need persistence for this case...

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +634,5 @@
>  modifyConflictPromptMessage=The item being edited in the dialog has been modified since it was opened.
>  modifyConflictPromptButton1=Overwrite the other changes
>  modifyConflictPromptButton2=Discard these changes
> +
> +# LOCALIZATION NOTE (integration)

the stuff in parenthesis should be the localization key
(integrationLabel)

@@ +636,5 @@
>  modifyConflictPromptButton2=Discard these changes
> +
> +# LOCALIZATION NOTE (integration)
> +# Used by the notification bar after installing Lightning by default
> +integrationLabel=%1$S now ships with an integrated %2$S calendar extension.

Maybe
%1$S now contains calendaring functionality by integrating the %2$S extension.

... or at least something that doesn't talk about shipping which is quite techy

@@ +643,5 @@
> +integrationOptOutButton=No thanks, remove it!
> +integrationOptOutAccessKey=r
> +integrationKeepItButton=Keep it!
> +integrationKeepItAccessKey=K
> +integrationRestartLabel=%1$S will be removed with the next restart of %2$S.

%1$S will be removed the next time you restart %2$S.

Comment 25

[Richard Marti (:Paenglab)]

Comment on attachment 8567630 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V1.diff

I tried the patch with TB.

First I had to revert 'calendar.integration.notification' to see the notification. After restart the window was huge because in every button where was a error message it can't find the strings in lightning.properties. After filling the strings there it's working.

+1 for Magnus's comment for the string change. But after the reboot, is Lightning removed or only disabled? If only disabled, shouldn't the strings be changed from 'remove' to 'disable' to reflect this? Because when a user opens the Add-ons and Lightning is still there but only disabled the user could be confused why it's still here.

Comment 26

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-LightningPart-V2.diff

Thank you for your feedback and sorry for the hassle to inject the strings manually - strings always require some packaging in Lightning.

Lightning will be uninstalled if the user chooses to remove - disabling is only a fallback option if uninstalling doesn't work for what reason ever.

In the updated patch, I considered all of Magnus comments except to show the AddonManager. Instead I added a undo button to the bar and an hint to the string where to add it again later on if required.

Comment 27

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-ThunderbirdPart-V2.diff

Pref changed due to updated Lightning part.

Comment 28

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-SeamonkeyPart-V2.diff

Pref changed due to updated Lightning part.

Comment 29

[[e-mail]]

Comment on attachment 8567662 [details] [diff] [review]
CalendarIntegrationOptOutNotification-SeamonkeyPart-V2.diff

The code as written won't work in SeaMonkey anyway, as our notificationbox has a different id and the code to open a web page is different.

Please can you explain why you have two preferences? You always turn off notification when you out out anyway, so that seems like duplication of effort. Also, what happens to people who had already installed Lightning?

Comment 30

[[:MakeMyDay]]

Neil, thanks for taking a look.

In general, what is the SM strategy on Lightning. Do you also want to ship it bundled as TB will? If not, we can simply remove the SM part of this patch and you stay as you do today.

If you want to bundle Lightning too, we need to include the special handling for SM (eventually in a separate bug) for notification bar and openeing the web page. Do you have any reference for me on that?

Regarding the pref: We ship with Lightning enabled in TB, and therefor with opt-out:false. If the user doesn't opt-out, we need a pref to control whether or not to show the notification bar any longer.

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 8567660 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V2.diff

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

::: calendar/base/content/calendar-chrome-startup.js
@@ +83,5 @@
> +                         Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
> +                                  .getService(Components.interfaces.nsIExternalProtocolService)
> +                                  .loadUrl(cal.makeURL(kSupportUri));
> +                        // to prevent closing notification bar, we need to throw an error
> +                        return true;

the comment should now be removed

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +643,5 @@
> +integrationOptOutButton=No thanks, remove it!
> +integrationOptOutAccessKey=r
> +integrationKeepItButton=Keep it!
> +integrationKeepItAccessKey=K
> +integrationRestartLabel=%1$S will be removed the next time you restart %2$S. You can add it again at any time from the AddonManager.

Add-ons Manager

and should that be "through" or "using" instead of "from"?

Comment 32

[Magnus Melin [:mkmelin]]

I also wonder how you plan to handle existing users? For those this notification isn't really appropriate.

Comment 33

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8567660 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V2.diff

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

(In reply to MakeMyDay from comment #26)
> Lightning will be uninstalled if the user chooses to remove - disabling is
> only a fallback option if uninstalling doesn't work for what reason ever.
I think we should actually disable Lightning in any case. If we remove it, it will definitely be installed on the next major upgrade. If we just disable it, we can fix the addons manager to upgrade but not enable the addon.

I'm going to push the strings for this patch with comment(s) taken into account, we should get the rest of the code in ASAP.

::: calendar/base/content/calendar-chrome-startup.js
@@ +61,5 @@
> +
> +/**
> + * Display opt-out notification for calendar integration if appropriate
> + */
> +function calendarIntegrationNotification() {

The function needs to go into calendar/lightning somewhere.

@@ +64,5 @@
> + */
> +function calendarIntegrationNotification() {
> +    const kOptOut = "mail.calendar-integration.opt-out"; // default: false
> +    const kNotify = "calendar.integration.notify"; // default: true
> +    const kSupportUri = "https://support.mozilla.org/products/thunderbird/calendar";

We should probably have a support page dedicated to this integation.

@@ +72,5 @@
> +        if (Preferences.get(kNotify, false)) {
> +            let notifyBox = document.getElementById("mail-notification-box");
> +            let appBrand = cal.calGetString("brand", "brandShortName", null, "branding");
> +            let ltnBrand = cal.calGetString("lightning", "brandShortName", null, "lightning");
> +            let label = cal.calGetString("lightning", "integrationLabel", [appBrand, ltnBrand], "lightning");

There is a helper in lightning-utils.js called ltnGetString we could use. Its not in a module, but we are in UI code so it should be ok.

@@ +81,5 @@
> +                     popup:     null,
> +                     callback:  function(aNotificationBar, aButton) {
> +                         Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
> +                                  .getService(Components.interfaces.nsIExternalProtocolService)
> +                                  .loadUrl(cal.makeURL(kSupportUri));

I think makeURL does nothing more than Services.io.newUri and comes from times before Services.jsm. I'd like to deprecate makeURL eventually, please use the io service variant instead.

@@ +101,5 @@
> +                    popup:     null,
> +                    callback:  function(aNotificationBar, aButton) {
> +                        Preferences.set(kNotify, false);
> +                        Preferences.set(kOptOut, true);
> +                        AddonManager.getAddonByID("{e2fda1a4-762b-4020-b5ad-a41df1933103}",

We should use a constant for the Lightning uuid.

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +634,5 @@
>  modifyConflictPromptMessage=The item being edited in the dialog has been modified since it was opened.
>  modifyConflictPromptButton1=Overwrite the other changes
>  modifyConflictPromptButton2=Discard these changes
> +
> +# LOCALIZATION NOTE (integrationLabel)

The strings need to go into lightning.properties instead.

@@ +639,5 @@
> +# Used by the notification bar after installing Lightning by default
> +integrationLabel=%1$S now contains calendaring functionality by integrating the %2$S extension.
> +integrationLearnMoreButton=Learn more
> +integrationLearnMoreAccessKey=m
> +integrationOptOutButton=No thanks, remove it!

I'd prefer less exciting labels, "Disable" and "Keep"

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset dd67418e4d1b

Comment 35

[[e-mail]]

(In reply to MakeMyDay from comment #30)
> In general, what is the SM strategy on Lightning. Do you also want to ship
> it bundled as TB will? If not, we can simply remove the SM part of this
> patch and you stay as you do today.
I don't have the authority to give a definitive statement, but I understand that it's been under consideration.

> If you want to bundle Lightning too, we need to include the special handling
> for SM (eventually in a separate bug) for notification bar and openeing the
> web page. Do you have any reference for me on that?
The id of our notificationbox is "messagepanebox".
To open a web page you would probably want to use openUILink.

> Regarding the pref: We ship with Lightning enabled in TB, and therefor with
> opt-out:false. If the user doesn't opt-out, we need a pref to control
> whether or not to show the notification bar any longer.
You only need the one pref to show the notification bar. If the user opts out then you end up removing/disabling Lightning so you'll never actually run the code again anyway.

Comment 36

[[:MakeMyDay]]

@Philipp: I would be in favour to uninstall if possible. Allthough you've already checked in the strings, if there is any option to make it at more general term, maybe it's worth to modify it that way, so that we're not forced to decide this by string freeze. This was also the reason why I phrased it remove and not uninstall. What I've seen on the AddonManager API, it should be possible to intercept the installation as well, so that we can deal with the major upgrade auto installation.

@Neil: thanks, I will consider this to get the code also working for SM. Regarding the pref: we need that to persist the opt-out decision even after disabling/uninstalling Lightning to prevent adding it again if not wanted. Also, this is the buy-in of the app to indicate Lightning that it is bundled. There are products beyond TB and SM using Lightning.

@Magnus: you are right, we need something further to detect installations having previously installed Lightning. It's nor as easy it would have been in the opt-in approach, but I think we can refer to number and type of available calendars and eventually on the number of existing events therein.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to MakeMyDay from comment #36)
> @Philipp: I would be in favour to uninstall if possible. Allthough you've
> already checked in the strings, if there is any option to make it at more
> general term, maybe it's worth to modify it that way, so that we're not
> forced to decide this by string freeze. This was also the reason why I
> phrased it remove and not uninstall. What I've seen on the AddonManager API,
> it should be possible to intercept the installation as well, so that we can
> deal with the major upgrade auto installation.
I've done so in https://hg.mozilla.org/comm-central/rev/1e5560a1eb96

Comment 38

[[e-mail]]

(In reply to MakeMyDay from comment #36)
> Regarding the pref: we need that to persist the opt-out decision even after
> disabling/uninstalling Lightning to prevent adding it again if not wanted.
> Also, this is the buy-in of the app to indicate Lightning that it is
> bundled. There are products beyond TB and SM using Lightning.
No, you don't need to persist it, because you only notify once.

Comment 39

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-LightningPart-V3-withoutStrings.diff

> No, you don't need to persist it, because you only notify once.
This is wrong - the bar is not only displayed once but until the user has made his/her choice. So we need that second pref to not show the bar again if the user permanently opted in.

The updated patch considered the Philipp's comments and Neil's hints on SM compatibility. Additionally, it now notifies only, if this is newly installed Lightning.

@Philipp: I'm re-requesting review as the code changed a little bit due to incorporating the detection functionality to recognized a previously installed Lightning.

In short, the notification bar will appear only if and as long as the user 
 - hasn't opted out already and
 - hasn't permantently opted-in and
 - has a newly installed Lightning, which hasn't been used before.

N.B.: all checks done at startup, so manual in life pref changes or adding an event to the default calendar would not let disappear the bar immediately but on the next restart.

I haven't tested this on SM, so maybe somebody can do and report back if the bar is displayed and openeing the link by hitting the Learn more button does work there.

Try builds are available for all plattforms at [1]. For testing, create a new profile and install Lightning therein (which later will be done by the bundle installation). You also need to create a boolean pref "mail.calendar-integration.opt-out" and disable it to make the bar to appear after the next startup.

[1] https://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/[e-mail]-825fb71b58ff/

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8571018 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V3-withoutStrings.diff

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

I haven't actually tested it, here just a few code comments. I'll give this a final test once I get the packager stuff fixed.

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +216,5 @@
> +             label:     ltnGetString("lightning", "integrationLearnMoreButton"),
> +             accessKey: ltnGetString("lightning", "integrationLearnMoreAccessKey"),
> +             popup:     null,
> +             callback:  function(aNotificationBar, aButton) {
> +                 openUILink(kSupportUri, aButton);

Could you check indent here? If you like, you can also save a level like so:

let buttons = [{
    ...
    }, {
    ...
}];

@@ +331,5 @@
> +                        this.calItems++;
> +                    }
> +                };
> +                // we look at all items at any time, but we can stop if the first item was found
> +                cals[0].getItems(Components.interfaces.calICalendar.ITEM_FILTER_ALL_ITEMS,

You can use calAsyncUtils.jsm here to clean up the code a little.

Comment 41

[[e-mail]]

(In reply to MakeMyDay from comment #39)
> > No, you don't need to persist it, because you only notify once.
> This is wrong - the bar is not only displayed once but until the user has
> made his/her choice.
Sorry, I meant to say you don't notify once the user has made his/her choice.

> So we need that second pref to not show the bar again
> if the user permanently opted in.
You don't need a pref to remember the decision itself, only whether the user (still) needs to make the decision.

Comment 42

[[:MakeMyDay]]

(In reply to [e-mail] from comment #41)
> You don't need a pref to remember the decision itself, only whether the user
> (still) needs to make the decision.
While Lightning's notify pref controls whether the bar is still to be displayed, the TB/SM opt-out pref is to decide whether to show the bar at all (this is the buy-in for bundling, to distinguish bundled/unbundled installations) and to have a trigger to prevent a reinstallation when updating TB/SM in case of bundling after a previous opt-out (although there's no solution to prevent this yet, but that will be another bug anyway). So, I'm afraid I still don't see your point here.

I'm about to push the final patch for this bug any day soon, so can you have a look at the SM specific adaptions (->messagepanebox id, openUILink) in the current one?

Comment 43

[Philipp Kewisch [:Fallen][📱📆]]

Neil, Magnus, I'd appreciate if you could review/comment the remaining bits so we can get this in asap.

Comment 44

[[e-mail]]

(In reply to MakeMyDay from comment #42)
> (In reply to comment #41)
> > You don't need a pref to remember the decision itself, only whether the user
> > (still) needs to make the decision.
> While Lightning's notify pref controls whether the bar is still to be
> displayed, the TB/SM opt-out pref is to decide whether to show the bar at
> all. So, I'm afraid I still don't see your point here.

Either pref will stop the bar from being displayed. So you only need to change one of them. So you don't need the other pref at all. Examples:
1. Lightning wasn't bundled. Preference isn't bundled, so don't show the opt-out.
2. Lightning was bundled, but the opt-out hasn't been shown yet. Preference says show the opt-out.
3. Lightning was bundled, but the opt-out was ignored. Preference still says show the opt-out.
4. Lightning was bundled, but the installation was retained. Preference was reset so don't show.
5. Lightning was bundled, but opted out. Preference was reset, although code won't run anyway.
6. Lightning was bundled, opted out, then reinstalled. Preference is still reset so don't show.
Which case have I missed?

> I'm about to push the final patch for this bug any day soon, so can you have
> a look at the SM specific adaptions (->messagepanebox id, openUILink) in the
> current one?

The id change looks good, but I would only pass one parameter to openUILink (the second parameter would have to be an event, which you don't have).

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 8567661 [details] [diff] [review]
CalendarIntegrationOptOutNotification-ThunderbirdPart-V2.diff

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

I think Neil is right, you don't need this pref. Please re-request review if there's some case missed

Comment 46

[[:MakeMyDay]]

Thanks for eleboration the use cases. If we do not consider the situation of a (major) application update of TB (or SM), this can work.

From post mortem perspective, 4-6 look the same, so if we need the to distinguish to prevent re-installing after upgrade of the hosting application on our own, that approach will not be enough, I'm afraid. Unfortunately, we currently haven't a final solution (at least no tested one, I assume) for that upgrade scenario.

Philipp, as you're working on the packaging stuff, what do you think, do we need a separate unique indicator?

Comment 47

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to MakeMyDay from comment #46)
> Philipp, as you're working on the packaging stuff, what do you think, do we
> need a separate unique indicator?

I'm not sure how the packaging will affect this. It will be installed into the distribution folder, afaik new versions will always be copied over. It may also be that Lightning is upgraded from AMO, e.g. on 4.0.x versions.

Personally, I'd go with the least risky option. If it turns out we don't need the extra pref we can still throw it out, if we only add one pref and it turns out that due to a mistake in thinking we *do* need that extra pref, then we would risk showing the bar to users who don't need to see it.

I don't think we should wait much longer though, so if y'all think one pref is enough lets do it.

Comment 48

[[:MakeMyDay]]

Attachment: CalendarIntegrationOptOutNotification-LightningPart-V4-withoutStrings.diff

So, it's currently still undetermined, whether we may need the second pref in case of the major update to prevent re-installing Lightning after previous opt-out. As also mentioned in comment #46, if we would go the other way and it turned out that we would have needed the second pref, there's no way back. And on top of this, we will not have a major update on beta/aurora channel before the release anymore, so there's no opportunity left to test this under conditions close to production.

Therefore, at least atm, the one pref approach for me is no option from risk management perspective and I will leave the patch with the two pref approach. Find the latest version attached, considering Philipp's latest review comments regarding indents and use of calAsyncUtils.jsm.

That said, I'm requesting the review for both not changed minor parts of the patch for TB and SM once again.

Neil, I hope this is also acceptable for you, but I don't want to hold the train for TB any longer.

Comment 49

[[:MakeMyDay]]

Comment on attachment 8567661 [details] [diff] [review]
CalendarIntegrationOptOutNotification-ThunderbirdPart-V2.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: needed for next beta of TB38/Ltn4.0
Testing completed (on c-c, etc.): tested with try builds
Risk to taking this patch (and alternatives if risky): no

Comment 50

[[:MakeMyDay]]

Comment on attachment 8588452 [details] [diff] [review]
CalendarIntegrationOptOutNotification-LightningPart-V4-withoutStrings.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: needed for next beta of TB38/Ltn4.0
Testing completed (on c-c, etc.): tested with try builds
Risk to taking this patch (and alternatives if risky): no

Comment 51

[[:MakeMyDay]]

Comment on attachment 8567662 [details] [diff] [review]
CalendarIntegrationOptOutNotification-SeamonkeyPart-V2.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: needed for next beta of TB38(/SM2.35)/Ltn4.0
Testing completed (on c-c, etc.): tested with try builds
Risk to taking this patch (and alternatives if risky): no

Comment 52

[[e-mail]]

Comment on attachment 8567662 [details] [diff] [review]
CalendarIntegrationOptOutNotification-SeamonkeyPart-V2.diff

Well, if the module owner says two prefs, then so be it.

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 8567661 [details] [diff] [review]
CalendarIntegrationOptOutNotification-ThunderbirdPart-V2.diff

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

Oh well. r=mkmelin

Comment 54

[Kent James (:rkent)]

I assume these patches be landed on comm-central as usual, prior to uplift to aurora and beta, right?

Comment 55

[[:MakeMyDay]]

Sure. I just didn't manage to check in yet.

Comment 56

[[:MakeMyDay]]

Phillip, as discussed on IRC, please take care to checkin once the tree reopens.

Comment 57

[Philip Chee]

Comment on attachment 8567662 [details] [diff] [review]
CalendarIntegrationOptOutNotification-SeamonkeyPart-V2.diff

a=me for SeaMonkey CLOSED TREE

Comment 58

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Lightning: https://hg.mozilla.org/comm-central/rev/694999c39566
Thunderbird: https://hg.mozilla.org/comm-central/rev/fb77e3b7fd2b
SeaMonkey: https://hg.mozilla.org/comm-central/rev/12e1abf30011

Comment 59

[Philipp Kewisch [:Fallen][📱📆]]

As this is a Thunderbird bug and its approval-comm-* I can't make the approvals. It seems the checkin didn't make the tree look worse than before, can we backport get this approved and pushed?

Comment 60

[Philipp Kewisch [:Fallen][📱📆]]

remote:   https://hg.mozilla.org/releases/comm-aurora/rev/8c3263316950
remote:   https://hg.mozilla.org/releases/comm-aurora/rev/2e97545a8c1a
remote:   https://hg.mozilla.org/releases/comm-aurora/rev/e122397eeb86

remote:   https://hg.mozilla.org/releases/comm-beta/rev/5fbe0977bfbf
remote:   https://hg.mozilla.org/releases/comm-beta/rev/77eb7b8f50db
remote:   https://hg.mozilla.org/releases/comm-beta/rev/6d21c19230dd