1399202 - better encapsulate language group selection methods

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

In bug 1375978, comment 38, nhnt11 suggests a way to better encapsulate gFontsDialog._selectLanguageGroup and gMainPane._selectDefaultLanguageGroup.  We should do that or something similar.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: encapsulate language group selection

Here's another option that encapsulates _selectLanguageGroup promise handling without adding an extra parameter to that function's signature.  It makes _selectLanguageGroup a regular function that calls an internal anonymous async function.

The inner function gets an extra two spaces of indentation, which pushes the longest lines in the function (the items in the "prefs" array) from 121 to 123 columns.  So I altered the formatting of those lines to shorten them.

nhnt11: What do you think?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8907219 [details] [diff] [review]
encapsulate language group selection

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

Okay, so I saw this days ago when you first posted it, but I was pretty conflicted about it so I took my time to respond. But anyway, here are my thoughts!

+ There's a weird sort of elegance to this that I really like, for some reason.
o Prior to this, I specifically was trying to think of ways to do this without increasing the indent of the whole function.
- One thing that I pondered on was the readability of the .catch() on the anonymous function call.

It's been a few days and I couldn't think of anything better, so r+ if you want to land this - I didn't test it locally though!

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: encapsulate language group selection

(In reply to Nihanth Subramanya [:nhnt11] from comment #2)
> o Prior to this, I specifically was trying to think of ways to do this
> without increasing the indent of the whole function.

I had the same consideration, but I couldn't figure out a way to do it (at least not one that would preserve readability).


> - One thing that I pondered on was the readability of the .catch() on the
> anonymous function call.

I wondered about that as well.  This patch puts it on the next line and indents it two spaces.   How does that look?


> It's been a few days and I couldn't think of anything better, so r+ if you
> want to land this - I didn't test it locally though!

For consistency, this patch makes the same changes to _selectDefaultLanguageGroup.  I've tested locally, and it works as expected.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: encapsulate language group selection

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)

> For consistency, this patch makes the same changes to
> _selectDefaultLanguageGroup.  I've tested locally, and it works as expected.

Erm, I spoke too soon.  The tests all passed, but I just observed some menulist display artifacts in the Fonts dialog during manual testing.  It turns out the new async functions weren't properly awaiting their promises because the value of *this* was incorrect.  The fix in this patch is to make them arrow functions.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910838 [details] [diff] [review]
encapsulate language group selection

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

Thanks, and apologies for the delayed review!

FWIW, putting the catch() on the next line isn't that much more readable to me, I'm okay with not moving it (but I'm also okay with keeping it as it is in this patch).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ecf69d752d
encapsulate language group selection; r=nhnt11

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> FWIW, putting the catch() on the next line isn't that much more readable to
> me, I'm okay with not moving it (but I'm also okay with keeping it as it is
> in this patch).

Thanks for the review!  I ended up leaving it as it is in this patch.  I'm always open to further refinements, if you think of something to make it more readable.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/73ecf69d752d