Closed Bug 1482351 Opened Last year Closed Last year

Calendar preferences not shown if they were selected last time

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Probably also related is the behaviour described in https://bugzilla.mozilla.org/show_bug.cgi?id=1473294#c12
Blocks: 1478608
Attached patch 1482351-pref-pane-select-1.diff (obsolete) — Splinter Review
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.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8999490 - Flags: review?(philipp)
Attached patch 1482351-pref-pane-select-2.diff (obsolete) — Splinter Review
Updated after bug 1484449.
Attachment #8999490 - Attachment is obsolete: true
Attachment #8999490 - Flags: review?(philipp)
Attachment #9002651 - Flags: review?(makemyday)
Don't forget, as I keep doing, that preferences.xml is preprocessed and needs a rebuild before this is going to work.
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.
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.
Attachment #9002651 - Attachment is obsolete: true
Attachment #9002651 - Flags: review?(makemyday)
Attachment #9003109 - Flags: review?(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.
Attachment #9003109 - Flags: feedback+
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).
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
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).
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.
Flags: needinfo?(richard.marti)
(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 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.
Attachment #9003109 - Flags: review?(makemyday)
Attachment #9003109 - Flags: review?(acelists)
Attachment #9003109 - Flags: review+
Attachment #9003109 - Flags: feedback+
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.
Attachment #9003109 - Flags: review?(acelists) → review+
Sorry, forgot to note there is some bitrot in the preferences.js file.
Flags: needinfo?(acelists)
Attachment #9003109 - Attachment is obsolete: true
Attachment #9007524 - Flags: review+
Keywords: checkin-needed
Blocks: 1489825
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a8d134e692bd
Add placeholder for calendar preferences, and remove it if Lightning is not enabled. r=aceman,MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Beta uplift to TB 63?
Flags: needinfo?(geoff)
Yes, good plan.
Flags: needinfo?(geoff)
Comment on attachment 9007524 [details] [diff] [review]
1482351-pref-pane-placeholder-2.diff

Now how approves one hunk in Calendar and two in mail/ ;-) ?
Attachment #9007524 - Flags: approval-calendar-beta?(philipp)
Attachment #9007524 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
You need to log in before you can comment on or make changes to this bug.