1482351 - Calendar preferences not shown if they were selected last time

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Geoff Lankow (:darktrojan)]

Similar to, but not quite, bug 1481941. The calendar preferences panel is added after the tab decides which panel to show, which means it is too late if it was the one supposed to be shown.

Comment 1

[[:MakeMyDay]]

Probably also related is the behaviour described in https://bugzilla.mozilla.org/show_bug.cgi?id=1473294#c12

Comment 2

[lokesh]

Wow very interesting post. Really amazing one. It is a new information for me.
http://aktechnoservice.com/samsung-refrigerator-service-center-in-hyderabad/

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1482351-pref-pane-select-1.diff

With this we record the previously selected pane if it can't be found at load time. When the overlay loads, we look to see if the pane should be selected.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1482351-pref-pane-select-2.diff

Updated after bug 1484449.

Comment 5

[Geoff Lankow (:darktrojan)]

Don't forget, as I keep doing, that preferences.xml is preprocessed and needs a rebuild before this is going to work.

Comment 6

[[:MakeMyDay]]

Just for completeness as mentionen on irc:

There's quite a delay until reloading happens for me, so that you can see that it loads the General pane first and switches afterwards - I assume this will be worse with an average users machine. Is there any option to delay the initial loading if the main detects a setup to restore that it cannot resolve?

You seemed to have already an idea of bypassing something.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1482351-pref-pane-placeholder-1.diff

I call this the "throw everything out and start again with a much simpler idea" option. It does however depend on ExtensionSupport.loadedLegacyExtensions existing which won't happen until bug 1483087 is fixed.

Comment 8

[[:MakeMyDay]]

Comment on attachment 9003109 [details] [diff] [review]
1482351-pref-pane-placeholder-1.diff

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

This works nicely now, however I suggest to make the mail part more general and not specific for Lightning, e.g. like adding a hidden dummy pane and add it's clone for every pane tat is tried to be restored from want's in xulstore.json but not available in the DOM at that time.

Comment 9

[[:MakeMyDay]]

Aceman, Paenglab: since you are mainly working on prefs in tab, do you have any opinion here? (n.b., to apply this patch, the one from bug 1478608 is needed).

Comment 10

[:aceman]

Comment on attachment 9003109 [details] [diff] [review]
1482351-pref-pane-placeholder-1.diff

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

This looks simple enough and in line with what the chat tab does.

::: mail/components/preferences/aboutPreferences.xul
@@ +50,5 @@
>  #include chat.inc.xul
>  #include privacy.inc.xul
>  #include security.inc.xul
>  #include applications.inc.xul
> +  <prefpane id="paneLightning"/>

So what happens when LT is disabled? Yes, the radio gets hidden by the code in preferences.js, but what about this? Will it be properly invisible while empty?

Maybe you could #ifdef this element when calendar is not compiled in (ac_add_options --enable-calendar not set). But then, can LT still be installed from AMO (ATN) as a standalone extension?

This also grants LT a special position (yes, it has it already by being bundled), which other addons do not have (and they have their options in own dialogs or tabs where features are killed by m-c).

Comment 11

[Richard Marti (:Paenglab)]

Aceman is more experienced than I. When he says it's okay then it is.

I only think the #ifdef doesn't work as LT still can be installed.

Comment 12

[[:MakeMyDay]]

(In reply to :aceman from comment #10)
> Maybe you could #ifdef this element when calendar is not compiled in
> (ac_add_options --enable-calendar not set). But then, can LT still be
> installed from AMO (ATN) as a standalone extension?

I think an IFDEF doesn't work here since that would be a build-time decision, but we need to decide at load-time, because the addon can also be user installed at any time even if TB is build without Calendar.
 
One could probably add the prefpane tag, add a simple js check whether Lighting is enabled and hide the prefpane if it's not available.

Comment 13

[[:MakeMyDay]]

Comment on attachment 9003109 [details] [diff] [review]
1482351-pref-pane-placeholder-1.diff

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

r+ for the calendar part.

Aceman, can you the review for the mail part? The patch does what I mentioned in in comment 12.

Comment 14

[:aceman]

Comment on attachment 9003109 [details] [diff] [review]
1482351-pref-pane-placeholder-1.diff

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

OK, I have played with this in all cases (LT on, LT off, LT not compiled in).
Previously, if LT was enabled, the LT item (pane header) in the menu appeared as a whole after some time after opening the prefs.
Now after the patch, the item is always immediately there but without label, gets its icon loaded together with other items.
Then if LT is enabled, then a label appears. If LT is disabled, the item is hidden.
So assuming LT enabled is the default case, as we ship it, this isn't as bad. Just the missing label for a moment may be irritating.
I r+ this as a fix for the immediate problem, but maybe you could file a new bug for this so that LT guys can think whether this UX can be improved further.

Comment 15

[:aceman]

Sorry, forgot to note there is some bitrot in the preferences.js file.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1482351-pref-pane-placeholder-2.diff

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a8d134e692bd
Add placeholder for calendar preferences, and remove it if Lightning is not enabled. r=aceman,MakeMyDay

Comment 18

[Jorg K (CEST = GMT+2)]

Beta uplift to TB 63?

Comment 19

[Geoff Lankow (:darktrojan)]

Yes, good plan.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9007524 [details] [diff] [review]
1482351-pref-pane-placeholder-2.diff

Now how approves one hunk in Calendar and two in mail/ ;-) ?

Comment 21

[Jorg K (CEST = GMT+2)]

TB 63 beta, Cal 6.5:
https://hg.mozilla.org/releases/comm-beta/rev/5ae858c40bf95883d0198953b09c77a7ab144913