UI language selector (#1250)

[tombogle]

This change is 

[tombogle]

See follow-on task: #1386

[jolierabideau]

Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @tombogle)

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 200 at r4 (raw file):

      if (Array.isArray(setting) && settingKey === 'platform.interfaceLanguage') {
        component = (
          <UiLanguageSelector

I was thinking you would want to pass debouncedHandleChange to your component so that you can use the built in interfaceLanguageValidator from core-settings-info.data.ts. You might to make some adjustments to the validator, since you can no longer type.

[lyonsil]

Reviewed 5 of 19 files at r1, 7 of 17 files at r2, 7 of 11 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tombogle)

---

lib/platform-bible-react/dist/index.d.ts line 1458 at r4 (raw file):

};
export type UiLanguageSelectorProps = {
	/** Full set of known languages to display. */

Are the property names/keys of this object BCP-47 tags? It would be good to clarify this.

Assuming they are BCP-47 tags, is the intent for this list to only have language codes (first part) or language code plus region code or just whatever we or someone else feels like? Like how does it get worked out whether we'd ever want to put en only or en-GB, en-NZ, en-US, etc.? Or es-ES, es-MX, etc.? It would be interesting to understand who/what populates this list. Assuming it ends up being "the list of all locales, with or without region codes, that we have localization resources for," that makes sense to me. Regardless, it would be good to call this out somewhere.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 103 at r4 (raw file):

    setSelectedLanguage(code);
    if (handlePrimaryLanguageChange) handlePrimaryLanguageChange(code);
    // REVIEW: Should fallback languages be preserved when primary language changes?

I would think that yes, changing the primary language would not change the fallback languages. If you filter the new primary from the fallback options, then cycling through different primary choices "eats away" all the fallback choices unless you push the former language choice into the fallback options. Ultimately, though, the right behavior of this function is probably defined by the user experience with the control. There isn't necessarily a "right answer" for a function like this on its own.

A function with a signature that doesn't leave ambiguous questions like this would probably just take a whole array of languages as the input. Then you could check if the primary and/or fallback languages actually changed. This is how we work with the USFM editor control and loading USFM.

No matter what the final decision is, this comment should probably be removed before merging.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 111 at r4 (raw file):

  };

  const getLanguageDisplayName = (lang: string, uiLang: string) => {

It's a little confusing how lang and uiLang are different and what they are intended to convey. Could you give them more descriptive names and/or add JSDoc-style comments (e.g., /** ... docs here */) to clarify the meaning of the parameters?

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 122 at r4 (raw file):

  };

  /*   const handleFallbackLanguageClick = () => {

I assume this was left in accidentally. Should this be removed?

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 167 at r4 (raw file):

                : `${knownUiLanguages.en.autonym}`}
            </Label>
            {/* <MultiSelector>

Is this a placeholder for the fallback language UI to be added later? If so, could you add a quick comment to that end?

[rolfheij-sil]

Nice work, thanks!

Reviewed 18 of 19 files at r1, 7 of 17 files at r2, 7 of 11 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @tombogle)

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 141 at r4 (raw file):

        <SelectContent
          // Need to get over the floating web view z-index 200
          style={{ zIndex: 250 }}

Better to use tailwind instead of inline css

className='tw-z-[250]' should do it

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 160 at r4 (raw file):

            {/* Do not localize or "improve". This label is temporary. */}
            <Label>
              Currently:{'\u00A0'}

Better to do this with a template literal string

{`Currently: `}

And maybe only render this if fallbackLanguages is not empty?

But I see this might only be a temporary label, so non-blocking

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 117 at r1 (raw file):

}: CombinedSettingProps) {
  const validateSetting = validateUserSetting || validateProjectSetting;
  const defaultLanguages: Record<string, LanguageInfo> = {

This needs to be stable, so it should be moved outside of the component (or wrapped in useMemo)

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 214 at r1 (raw file):

            fallbackLanguages={setting.slice(1)}
            handleLanguageChanges={(newUiLanguages) => {
              if (!isLoadingLanguages) setSetting(newUiLanguages);

This might silently fail if the languages are loading. Shouldn't it be more explicit in telling the user that setting new UI languages failed in that case?

[tombogle]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jolierabideau, @lyonsil, and @rolfheij-sil)

---

lib/platform-bible-react/dist/index.d.ts line 1458 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Are the property names/keys of this object BCP-47 tags? It would be good to clarify this.

Assuming they are BCP-47 tags, is the intent for this list to only have language codes (first part) or language code plus region code or just whatever we or someone else feels like? Like how does it get worked out whether we'd ever want to put en only or en-GB, en-NZ, en-US, etc.? Or es-ES, es-MX, etc.? It would be interesting to understand who/what populates this list. Assuming it ends up being "the list of all locales, with or without region codes, that we have localization resources for," that makes sense to me. Regardless, it would be good to call this out somewhere.

They are BCP-47 tags. For Chinese, we do specifically expect to have two variants. I doubt we'll have two variants of English in Paratext and its official extensions, but I know Transcelerator has en-GB strings. Brazilian Portuguese is enough different from European Portuguese that it could easily justify a distinct variant. I think the fallback logic already automatically falls back to base variants of the language if the user has chose a specific variant/script. In any case, once we implement the Ui to choose fallbacks, they could make that explicit.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 103 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I would think that yes, changing the primary language would not change the fallback languages. If you filter the new primary from the fallback options, then cycling through different primary choices "eats away" all the fallback choices unless you push the former language choice into the fallback options. Ultimately, though, the right behavior of this function is probably defined by the user experience with the control. There isn't necessarily a "right answer" for a function like this on its own.

A function with a signature that doesn't leave ambiguous questions like this would probably just take a whole array of languages as the input. Then you could check if the primary and/or fallback languages actually changed. This is how we work with the USFM editor control and loading USFM.

No matter what the final decision is, this comment should probably be removed before merging.

I believe this is an open UX question and is not going to be resolved in the first pass. That's why I added the REVIEW comment. Maybe we treat REVIEW comments differently in Platform, but in other software I've worked on, we use them to indicate a potentially important decision for future consideration, not just something to be reviewed as part of a PR.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 111 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It's a little confusing how lang and uiLang are different and what they are intended to convey. Could you give them more descriptive names and/or add JSDoc-style comments (e.g., /** ... docs here */) to clarify the meaning of the parameters?

Commented

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 122 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I assume this was left in accidentally. Should this be removed?

Initially, the fallback language selection UI was going to be a button that displayed some kind of popover or dialog. But I think you're right. This commented-out stub isn't very helpful now that we don't have any idea what the UI might be.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 141 at r4 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Better to use tailwind instead of inline css

className='tw-z-[250]' should do it

Funny. I'm 99% sure I beat my brains out trying this to no avail. Today, I tried it again and it worked just fine.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 160 at r4 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Better to do this with a template literal string

{`Currently: `}

And maybe only render this if fallbackLanguages is not empty?

But I see this might only be a temporary label, so non-blocking

Since English is always the ultimate fallback, there's always something to display if English isn't the UI language. Only just barely useful, but this is just a temporary placeholder.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 167 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is this a placeholder for the fallback language UI to be added later? If so, could you add a quick comment to that end?

Done.

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 214 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This might silently fail if the languages are loading. Shouldn't it be more explicit in telling the user that setting new UI languages failed in that case?

Originally, I was thinking that if the languages were to load slowly and the user chose one from the default list we wouldn't want to recognize that choice as it could lead to something bad. But at least in the current implementation, the list always seems to be loading with no delay, and now that I've correctly hooked up validation, I think we wouldn't care. Anyway, the choices in the little default list should be valid, so I think the

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 200 at r4 (raw file):

Previously, jolierabideau wrote…

I was thinking you would want to pass debouncedHandleChange to your component so that you can use the built in interfaceLanguageValidator from core-settings-info.data.ts. You might to make some adjustments to the validator, since you can no longer type.

Done.

[tombogle]

Reviewable status: 15 of 24 files reviewed, 10 unresolved discussions (waiting on @jolierabideau, @lyonsil, and @rolfheij-sil)

---

src/renderer/components/settings-tabs/settings-components/setting.component.tsx line 117 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This needs to be stable, so it should be moved outside of the component (or wrapped in useMemo)

Done.

[rolfheij-sil]

One more small thing, non-blocking

Reviewed 8 of 9 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jolierabideau, @lyonsil, and @tombogle)

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 87 at r6 (raw file):

  primaryLanguage = 'en',
  fallbackLanguages = [],
  onLanguageChanges,

nit: onLanguageChanges -> onLanguageChange

[lyonsil]

Thanks for all the updates!

Reviewed 8 of 9 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolierabideau and @tombogle)

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 103 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I believe this is an open UX question and is not going to be resolved in the first pass. That's why I added the REVIEW comment. Maybe we treat REVIEW comments differently in Platform, but in other software I've worked on, we use them to indicate a potentially important decision for future consideration, not just something to be reviewed as part of a PR.

Ok - This is not a convention I've seen used elsewhere, but if it's a normal thing at SIL/UBS that makes sense.

I would still assert that:

1. The right answer for the function will depend on the user experience and how it plays a role in that.
2. Having a function that takes the entire array of primary + fallbacks is a less fragile design as it doesn't leave open questions to address in the future like this.

I'm making this as resolved for this PR since it is waiting on UX input.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 111 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Commented

Thanks, this is very clear!

[Sebastian-ubs]

Hi Tom I tried it, was wondering about colors in the example, made some improvement to that in #1394, added a second picker and was thereby wondering if there is an issue with updating the selected language value when set from elesewhere? - should not be an issue when changing the lang requires a restart, but maybe it is...?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolierabideau and @tombogle)

[jolierabideau]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)

[tombogle]

I'm not sure we'd ever want/need/expect a second control that was also hooked up to the same setting. Certainly an extension could display the control and have it hooked up to some kind of override setting relevant only to that extension. (For example, Transcelerator has separate versions of English questions in British English, but if a user chooses British English for the Transcelerator questions, it doesn't change the default UI language in Paratext.) But maybe you're thinking of the idea of putting an interface language picker on the toolbar as well as having one in the settings...? If we do that, we'll definitely want to keep them in sync. @tjcouch-sil or @lyonsil could probably weigh in on the question of how difficult that would be. Presumably it would be essentially what is needed to also get the UI to be able to be more fully responsive to a change in the UI language.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 103 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Ok - This is not a convention I've seen used elsewhere, but if it's a normal thing at SIL/UBS that makes sense.

I would still assert that:

1. The right answer for the function will depend on the user experience and how it plays a role in that.
2. Having a function that takes the entire array of primary + fallbacks is a less fragile design as it doesn't leave open questions to address in the future like this.

I'm making this as resolved for this PR since it is waiting on UX input.

It may very well be that we should just scrap onPrimaryLanguageChange and onFallbackLanguagesChange entirely. Currently, they are only used in the example. Even if we do that, we'll still need to get the behavior to make sense for the user. My gut feeling is that if the user changes the primary language, they would most likely expect that to clear any fallback languages also. But there could be edge cases where that doesn't make sense.

---

lib/platform-bible-react/src/components/advanced/ui-language-selector.component.tsx line 87 at r6 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

nit: onLanguageChanges -> onLanguageChange

I made it plural because it covers changes to either the primary language, fallback language(s) or both. Clearly, I was not consistent though, because onFallbackLanguagesChange outs the plural on Languages, not on Changes. Thinking about it critically, the event fires once per "change" (even if that change affects more than one language), so it probably really should be onLanguagesChange, but that sounds weird to my ear. Maybe I should just make it onChange.

[Sebastian-ubs]

I'm not actually looking for a way to have 2 controls there, just wondering if testing with two controls next to each other, having the same input but not syncing, reveals a problem here...?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

[tombogle]

Got it. Yes, I see that selectedLanguage and primaryLanguage are being used interchangeably, when there's no actual guarantee that they would be kept in sync. To best illustrate the correct behavior, I think we want three of these controls: two tied together (using the same underlying setting) and one that is independent.

Reviewable status: 15 of 24 files reviewed, 1 unresolved discussion (waiting on @lyonsil and @rolfheij-sil)

[lyonsil]

Related to Tom's message where I was tagged, the useSetting hook should be used by the webviews that use this control to show the UI language settings. If that is used, then if one UI updates the setting, the other UI should see the change and redraw itself with the new setting. There is no technical problem with showing the same setting in more than one place in the UI.

Having said this, using this hook is not sufficient to remove the need to restart the application when the setting changes. There is a lot of non-React code that cannot use the React hook and for which it is not practical to monitor this one setting. We should require the application to restart when this setting changes.

Reviewed 9 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

[rolfheij-sil]

Reviewed 9 of 9 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Sebastian-ubs]

looks good :-)

Reviewable status: complete! all files reviewed, all discussions resolved

[rolfheij-sil]

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved