Port Bug 1147311: migrateUI() should migrate font.language.group to a supported value

RESOLVED FIXED in Thunderbird 38.0

Status

Thunderbird
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Trunk
Thunderbird 38.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
tracking-thunderbird38: --- → ?
(Assignee)

Comment 1

3 years ago
Created attachment 8583243 [details] [diff] [review]
migratelangggroup.diff

Updated

3 years ago
tracking-thunderbird38: ? → +

Comment 2

3 years ago
Comment on attachment 8583243 [details] [diff] [review]
migratelangggroup.diff

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

::: mail/base/modules/mailMigrator.js
@@ +310,5 @@
>          foStream.QueryInterface(Ci.nsISafeOutputStream).finish();
>          foStream.close();
>        }
>  
> +      if (currentUIVersion < 11) {

Please add a comment before this line documenting what this migration does and why. As all other versions above do. You have some comment later in the code so it could be moved here and completed.
Attachment #8583243 - Flags: feedback+

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
(In reply to :aceman from comment #2)
> Please add a comment before this line documenting what this migration does
> and why. As all other versions above do. You have some comment later in the
> code so it could be moved here and completed.

This was just a straight port of the m-c patch. I didn't request review because bug 1147311 has not been checked in yet.
(Assignee)

Comment 4

3 years ago
Created attachment 8584095 [details] [diff] [review]
migratelangggroup.diff v2

Matches version that is going to land on m-c, with comment moved as recommended by aceman.
Attachment #8583243 - Attachment is obsolete: true
Attachment #8584095 - Flags: review?(acelists)

Comment 5

3 years ago
Comment on attachment 8584095 [details] [diff] [review]
migratelangggroup.diff v2

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

This works for me feature-wise.

::: mail/base/modules/mailMigrator.js
@@ +310,5 @@
>          foStream.QueryInterface(Ci.nsISafeOutputStream).finish();
>          foStream.close();
>        }
>  
> +      // Latin language groups were consolidated.

Please make this more explaining, like "Several Latin language groups were consolidated into x-western".

@@ +318,5 @@
> +          group = Services.prefs.getComplexValue("font.language.group",
> +                                                 Ci.nsIPrefLocalizedString);
> +        } catch (ex) {}
> +        if (group &&
> +            ["tr", "x-baltic", "x-central-euro"].some(g => g == group.data)) {

Would [].contains(group.data) work and be more readable?
Attachment #8584095 - Flags: review?(acelists) → feedback+

Comment 6

3 years ago
(In reply to :aceman from comment #5)
> Would [].contains(group.data) work and be more readable?
Sorry, [].includes().
(Assignee)

Comment 7

3 years ago
Created attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

Amended comment.

> @@ +318,5 @@
> > +          group = Services.prefs.getComplexValue("font.language.group",
> > +                                                 Ci.nsIPrefLocalizedString);
> > +        } catch (ex) {}
> > +        if (group &&
> > +            ["tr", "x-baltic", "x-central-euro"].some(g => g == group.data)) {
> 
> Would [].contains(group.data) work and be more readable?

Apart from the fact that I really dislike changing ported code as it makes future comparisons with m-c much harder, Array.includes is nightly-only.
Attachment #8584095 - Attachment is obsolete: true
Attachment #8584118 - Flags: review?(acelists)

Comment 8

3 years ago
Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

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

They could have at least used .indexOf(). This .some() construct seems unnecessarily complicated.
Attachment #8584118 - Flags: review?(mkmelin+mozilla)
Attachment #8584118 - Flags: review?(acelists)
Attachment #8584118 - Flags: feedback+

Comment 9

3 years ago
Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

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

Looks good, thx! r=mkmelin
Attachment #8584118 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
(Assignee)

Comment 11

3 years ago
Comment on attachment 8584118 [details] [diff] [review]
migratelangggroup.diff v3

[Approval Request Comment]
[Feature/regressing bug #]: 756022
[User impact if declined]: This prevents possible old pref values in affected locales
Attachment #8584118 - Flags: approval-comm-aurora?

Updated

3 years ago
Attachment #8584118 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Updated

3 years ago
status-thunderbird38: --- → fixed
Target Milestone: Thunderbird 39.0 → Thunderbird 38.0
You need to log in before you can comment on or make changes to this bug.