Closed Bug 456663 Opened 13 years ago Closed 7 years ago

(Re)move |pref("mailnews.ui.threadpane.version", 1);|

Categories

(Thunderbird :: Preferences, defect)

defect
Not set
trivial

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: sgautherie, Assigned: foss)

References

()

Details

Attachments

(1 file, 2 obsolete files)

(Iirc, Mark suggested this bug when I asked David about my bug 309057 patch E...)

If TB wants |pref("mailnews.ui.threadpane.version", 5);| now,
simply remove |#ifdef MOZ_SUITE|...

If |Thunderbird still needs version 1| is still true,
move its |#else| part to </mail/app/profile/all-thunderbird.js>,
and document what this depends on.

NB: This is the only/last TB-only pref in <mailnews.js> ;-)
Bug 498209 changed the value to 7,
but did not move the preference to the TB file.
Depends on: 498209
I say move it to the TB file and get rid of the #ifdef
Attached patch patch (obsolete) — Splinter Review
Removes the #ifdef and moves TB-especific code to all-thanderbird.js.
Attachment #695856 - Flags: review?(irving)
Comment on attachment 695856 [details] [diff] [review]
patch

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

::: mail/app/profile/all-thunderbird.js
@@ +837,5 @@
>  // (or, more precisely, profiles that start with no accounts created).
>  pref("mail.main_menu.collapse_by_default", true);
> +
> +// for manual upgrades of certain UI features.
> +p// Thunderbird uses this pref in msgMail3PaneWindow.js for bad reasons.

Remove extra 'p'.

::: mailnews/mailnews.js
@@ +655,1 @@
>  pref("mailnews.ui.threadpane.version", 5);

Should this part be moved to a /suite file, too ?

http://mxr.mozilla.org/comm-central/search?string=mailnews.ui.threadpane.version&case=on&find=%2Fmail
Attachment #695856 - Flags: feedback?(iann_bugzilla)
Comment on attachment 695856 [details] [diff] [review]
patch

The suite version of the pref is now used only in /suite/mailnews, therefore I think it should move to a suite specific pref.js file, rather than being in mailnews.js where it isn't relevant to the rest of mailnews.
Attachment #695856 - Flags: feedback-
Comment on attachment 695856 [details] [diff] [review]
patch

As far as I can see neither TB or SM use this pref any more so I would say just remove it and the associated code in the two msgMail3PaneWindow.js files.
Attachment #695856 - Flags: feedback?(iann_bugzilla) → feedback-
(In reply to Ian Neal from comment #6)
> Comment on attachment 695856 [details] [diff] [review]
> patch
> 
> As far as I can see neither TB or SM use this pref any more so I would say
> just remove it and the associated code in the two msgMail3PaneWindow.js
> files.

I'm not sure what you looked at Ian, but I think it is clearly used:

http://mxr.mozilla.org/comm-central/search?string=mailnews.ui.threadpane.version
(In reply to Mark Banner (:standard8) from comment #7)
> (In reply to Ian Neal from comment #6)
> > Comment on attachment 695856 [details] [diff] [review]
> > patch
> > 
> > As far as I can see neither TB or SM use this pref any more so I would say
> > just remove it and the associated code in the two msgMail3PaneWindow.js
> > files.
> 
> I'm not sure what you looked at Ian, but I think it is clearly used:
> 
> http://mxr.mozilla.org/comm-central/search?string=mailnews.ui.threadpane.
> version

Depends what you classify as "used". I would not count the code that currently references it as "use" - the preference is checked, and if not high enough, set to that level, but nothing is done as such, so is there any point in doing the check and set?

Unless I am missing something:
Last use for TB was removed in May 2012 with http://hg.mozilla.org/comm-central/rev/75dec57445dc
Last use for SM was removed in September 2007 by Bug 384712
UpgradeProfileAndBeUglyAboutIt() doesn't look like it's being called either.

Do we have some other mechanism for upgrading UI settings? If not, we should leave the pref so we can do future upgrades.
There's some soup, but it looks like the centralized place is MailMigrator._migrateUI:
http://mxr.mozilla.org/comm-central/source/mail/base/modules/mailMigrator.js#98

Related upgrade stuff in terms of triggering tabs on upgrade and the info-bar prompts happens from:
http://mxr.mozilla.org/comm-central/source/mail/base/content/specialTabs.js#459
  called at:
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#406

With the rest of the upgrade stuff being one-off functions that use their own preferences or the like, called from:
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#429
Attachment #695856 - Flags: review?(irving)
Clearing request for review because of comment 4. It is being discussed about what needs to be done here, so I will wait until then :-)
Assignee: nobody → leofigueres
(In reply to David :Bienvenu from comment #9)
> UpgradeProfileAndBeUglyAboutIt() doesn't look like it's being called either.
> 

Here http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#935 there is a call. OnLoadThreadPane() is called from InitPanes().

> Do we have some other mechanism for upgrading UI settings? If not, we should
> leave the pref so we can do future upgrades.

In mailnews.js?
Attached patch patch 2 (obsolete) — Splinter Review
Corrected the "p".

Moving the pref to the preference files especific for each application.

Note: this attachment is not intended fo a review by the moment.
Attachment #695856 - Attachment is obsolete: true
Assignee: leofigueres → nobody
Bug 498209 moved the UI updater code to mail/base/content/folderDisplay.js (FolderDisplayWidget) in changeset 3062.

The code in folderDisplay.js makes no reference to mailnews.ui.threadpane.version preference.

threadpane.version is only checked in mail/base/content/msgMail3PaneWindow.js and, whatever value it is stored, will be changed to 7, which is value stored as default on mailnews.js.

Changeset 5777 (bug 569161) moved code that marked all IMAP folders as offline to autosync.js. The remaining only part which this patch left was the one that updated ui version to 7. Again, as told in previous paragraphs, preference is not referenced outside msgMail3PaneWindow.js.

All that said, I would:

- remove preference from mailnews.js
- remove any reference to the preference in code (i.e. JavaScript)
- update documentation, stating this preference has been removed and indicating latest known versions for SeaMonkey and Thunderbird so any add-on could, if needed, know it.
Assignee: nobody → leofigueres
Status: NEW → ASSIGNED
Attachment #8485905 - Flags: review?(iann_bugzilla)
:philor reviewed code modified for /mail on submitted patch.

:IanN reviewed code modified for /suite on submitted patch.

I have not obsoleted previous patch (attachment 704253 [details] [diff] [review]) because it is still unclear that preference should be removed, although I think it should be.
Comment on attachment 8485905 [details] [diff] [review]
Remove mailnews.ui.threadpane.version

Been a long long time since I was a Tb reviewer, though.
Attachment #8485905 - Flags: review?(philringnalda) → review?(mkmelin+mozilla)
Comment on attachment 8485905 [details] [diff] [review]
Remove mailnews.ui.threadpane.version

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

Looks good to me! r=mkmelin fo the mail/ parts
Attachment #8485905 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8485905 - Flags: review?(iann_bugzilla) → review+
Attachment #704253 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/a2f40581a7eb -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.