SF-3558 Update draft generation UI on language change

[pmachapman]

This change is 

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.30%. Comparing base (e7a5145) to head (334c391).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3474      +/-   ##
==========================================
- Coverage   82.30%   82.30%   -0.01%     
==========================================
  Files         613      613              
  Lines       36870    36869       -1     
  Branches     6044     6020      -24     
==========================================
- Hits        30347    30345       -2     
- Misses       5629     5642      +13     
+ Partials      894      882      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Nateowami]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 235 at r1 (raw file):

      book = Canon.bookNumberToId(book);
    }
    return this.transloco.translate(`canon.book_names.${book}`, {}, this.localeCode ?? defaultLocale.canonicalTag);

Why does this work? Also, this.localeCode is guaranteed to be defined.

[pmachapman]

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

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 235 at r1 (raw file):
Why does this work?

@Nateowami Not sure what you mean? I needed to specify the locale code as trySetLocale() sets the transloco language after it notifies the BehaviourSubject:

    this.currentLocale$.next(locale);
    this.transloco.setActiveLang(locale.canonicalTag);

I had considered swapping the order of these two lines, but was concerned about unintended consequences given the component is very widely used. I can do that if you think its worth the risk.

Also, this.localeCode is guaranteed to be defined.

Removed. I think I added it to work around a failing test I ended up fixing.

[Nateowami]

Upon further testing, it still doesn't behave correctly when switching to a new language for the first time. This is because there is a delay in loading the localization files. For a proper test, refresh the page while the language is set to English, then change the language. The book names don't update. If you switch back to English and then back to the language again, it then does update, since the localization files have been loaded already.

Basically, our localization system is not designed to have components/services translate non-dynamically.

@Nateowami reviewed 1 of 1 files at r2.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 235 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Why does this work?

@Nateowami Not sure what you mean? I needed to specify the locale code as trySetLocale() sets the transloco language after it notifies the BehaviourSubject:

    this.currentLocale$.next(locale);
    this.transloco.setActiveLang(locale.canonicalTag);

I had considered swapping the order of these two lines, but was concerned about unintended consequences given the component is very widely used. I can do that if you think its worth the risk.

Also, this.localeCode is guaranteed to be defined.

Removed. I think I added it to work around a failing test I ended up fixing.

Ah, makes sense. I think swapping the order is probably the best option. It makes sense that subscribers to the locale should be informed after the i81n service updates its internal state to support the new locale.!

Actually, while I think it does make sense to swap the order, that doesn't fully fix the problem, for the reasons noted elsewhere (localization files not being fully loaded yet).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 301 at r2 (raw file):

  ) {
    this.i18n.locale$.pipe(quietTakeUntilDestroyed(this.destroyRef)).subscribe(() => {
      if (this._entry != null) this.entry = this._entry;

To me this feels like a perfect example of why precalculating lots of things (calculating when a property is set, as opposed to calculating at render time) in a component isn't a good idea. Seeing this.entry = this._entry immediately feels like the wrong approach is being used in this component. Our localization system just isn't designed to work with it.

[pmachapman]

I have reworked this PR to perform the localization in the view, which should address the issues you noticed.

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @Nateowami)

[Nateowami]

@Nateowami reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)