SF-2909 Biblical Terms Enhancements

[pmachapman]

This PR enhances Biblical Terms to include support for user requested features from Paratext.

These include:

• Moving Biblical Terms to a tab
• Adding filtering by category for Biblical Terms
• Adding filtering by verse, category, and book for Biblical Terms.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 94.95798% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.06%. Comparing base (5068503) to head (a020d11).

Files with missing lines	Patch %	Lines
...anslate/biblical-terms/biblical-terms.component.ts	94.20%	2 Missing and 2 partials ⚠️
...ntApp/src/app/translate/editor/editor.component.ts	87.50%	1 Missing ⚠️
...p/translate/editor/tabs/editor-tab-menu.service.ts	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2671      +/-   ##
==========================================
+ Coverage   81.02%   81.06%   +0.04%     
==========================================
  Files         525      526       +1     
  Lines       30535    30569      +34     
  Branches     4993     4976      -17     
==========================================
+ Hits        24740    24782      +42     
+ Misses       5350     5344       -6     
+ Partials      445      443       -2     

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

[josephmyers]

Hey, great work so far. I think this way is more discoverable than toggling it from the settings cog. Here are a couple areas where we could use some padding:

I am also wondering if we should move the "transliterate" toggle into this tab somewhere. As it is, it's pretty hard to find. Have you considered this? Thanks

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-header/tab-header.component.scss line 77 at r1 (raw file):

    align-items: center;
    justify-content: center;
    vertical-align: middle;

Of these four, I am only seeing vertical-align have an effect

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

      <tr mat-footer-row *matFooterRowDef="['id']; sticky: true"></tr>
      <tr mat-row *matNoDataRow>
        <td class="mat-cell" [attr.colspan]="columnsToDisplay.length"></td>

We should put some text here, explaining that there are no results for the selection.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 60 at r1 (raw file):

const defaultLocaleCode = I18nService.defaultLocale.canonicalTag;

type ViewFilter = 'current_verse' | 'current_chapter' | 'current_book';

I'd rather call this (and related properties) something like RangeFilter, since Category is also a way of filtering.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

  }));

  it('should update the categories when the language changes', fakeAsync(() => {

This is a good test; however, I notice in production that changing the language resets the category and range without actually updating the list. Actually, it is clearing the list.

It'd be safer and more robust to convert the filter and category properties, which are currently just simple strings, to have setters, within which we refresh the display. Similar with selectedCategory and categories, which I believe are a source of the problem. I'm not a huge fan of "dumb" simple types driving business logic, as what is essentially state logic is bug prone.

The bug occurs because, after we've updated rows, we reset the selectedCategory if the current selection is not present in the category list. This will always be the case when we change language, due to us not keeping our properties up to date.

This would have been a tiny use case, but we should fix it anyway. And please add a unit test for it!

[pmachapman]

Here are a couple areas where we could use some padding:

Done. Thank you!

I am also wondering if we should move the "transliterate" toggle into this tab somewhere. As it is, it's pretty hard to find. Have you considered this?

I am going to think on this over the weekend. I will see if there is a nice way to include it in the UI without it being buried in settings (like you, I don't like how it is currently buried).

Reviewable status: 27 of 32 files reviewed, 4 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-header/tab-header.component.scss line 77 at r1 (raw file):

Previously, josephmyers wrote…

Of these four, I am only seeing vertical-align have an effect

Done. Thank you!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, josephmyers wrote…

We should put some text here, explaining that there are no results for the selection.

It displays while loading, so I was hesitant to put a message here. I only added the row so during the loading phase, the bottom row would not be jammed up against the header row.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 60 at r1 (raw file):

Previously, josephmyers wrote…

I'd rather call this (and related properties) something like RangeFilter, since Category is also a way of filtering.

Done. Thank you for the naming suggestion - I struggled to name this one!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

Previously, josephmyers wrote…

This is a good test; however, I notice in production that changing the language resets the category and range without actually updating the list. Actually, it is clearing the list.

It'd be safer and more robust to convert the filter and category properties, which are currently just simple strings, to have setters, within which we refresh the display. Similar with selectedCategory and categories, which I believe are a source of the problem. I'm not a huge fan of "dumb" simple types driving business logic, as what is essentially state logic is bug prone.

The bug occurs because, after we've updated rows, we reset the selectedCategory if the current selection is not present in the category list. This will always be the case when we change language, due to us not keeping our properties up to date.

This would have been a tiny use case, but we should fix it anyway. And please add a unit test for it!

I'm not sure I understand - can we meet early next week to discuss this?

[josephmyers]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

It displays while loading, so I was hesitant to put a message here. I only added the row so during the loading phase, the bottom row would not be jammed up against the header row.

Actually, I just added an isLoaded property to DataLoadingComponent, which helps us determine the difference between first-time loading and dynamic loading. I think you could harness that.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.scss line 58 at r2 (raw file):

.mat-mdc-header-cell:first-child,
.mat-mdc-cell:first-child {
  padding-left: 1.25em;

I haven't looked, but does work in RTL situations? We typically prefer padding-inline-start

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I'm not sure I understand - can we meet early next week to discuss this?

Sure, sorry for not being clear. Let's talk next week!

[pmachapman]

I am also wondering if we should move the "transliterate" toggle into this tab somewhere. As it is, it's pretty hard to find. Have you considered this?

I have added a settings popup menu at the bottom of the tab. Lett me know if this works better for you!

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, josephmyers wrote…

Actually, I just added an isLoaded property to DataLoadingComponent, which helps us determine the difference between first-time loading and dynamic loading. I think you could harness that.

Thank you! I have added support for this, and slightly extended your implementation.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.scss line 58 at r2 (raw file):

Previously, josephmyers wrote…

I haven't looked, but does work in RTL situations? We typically prefer padding-inline-start

Fixed - thank you for catching that!

[pmachapman]

Reviewable status: 22 of 33 files reviewed, 3 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

Previously, josephmyers wrote…

Sure, sorry for not being clear. Let's talk next week!

I have fixed the bug, and updated the unit test.

[josephmyers]

Can you add tooltips to the Actions buttons?

Reviewable status: 22 of 33 files reviewed, 3 unresolved discussions

[pmachapman]

Can you add tooltips to the Actions buttons?

I have added very brief tooltips, as the functionality for the actions varies a lot based on whether the user has edit permission, or add note permission for the project.

Reviewable status: 22 of 33 files reviewed, 3 unresolved discussions (waiting on @josephmyers)

[josephmyers]

I do notice that if the BT tab is open and you disable BT in the Settings, the BT tab stays open upon returning to the editor. Small issue, but we should fix it.

Reviewed 2 of 6 files at r3, 6 of 7 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Thank you! I have added support for this, and slightly extended your implementation.

I notice that while the list is loading, especially page load, we get the "No Biblical Terms match your criteria" message. Is there a way to avoid this?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 31 at r5 (raw file):

            <mat-icon [style.transform]="row.transformNotesIcon">{{ row.notesIcon }}</mat-icon>
          </button>
          <button mat-icon-button (click)="editRendering(row.id)" [matTooltip]="t('renderings')">

Can this say "Edit Renderings"?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 36 at r5 (raw file):

        </td>
        <td *matFooterCellDef [attr.colspan]="columnsToDisplay.length">
          <div class="toolbar">

I don't want to be too picky here, but why is the toolbar at the bottom of this tab? Was this intentional? All our other tabs place the toolbar above the content, and it's weird that this one is backwards. Do we gain something from putting it at the bottom?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 66 at r5 (raw file):

              [matMenuTriggerFor]="menu"
              class="mat-menu-popup-button"
              [matTooltip]="t('settings')"

I think this tooltip should just be "Settings" -- simpler

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I have fixed the bug, and updated the unit test.

Thanks. You did fix the bug, but you've introduced some slightly odd behavior to cope with the persistent "properties are out of date" issue in this class. By that I mean that it is weird and sad that we have to reset their category and range when the language changes. We're doing that to cope with the fact that the category, which gets updated, isn't found in the list of categories, because they're not updated til later. It's a code smell.

I'm sorry for belaboring the point, so I'm resolving this.

[pmachapman]

I do notice that if the BT tab is open and you disable BT in the Settings, the BT tab stays open upon returning to the editor. Small issue, but we should fix it.

Good spotting. Thank you!

Reviewable status: 28 of 33 files reviewed, 4 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, josephmyers wrote…

I notice that while the list is loading, especially page load, we get the "No Biblical Terms match your criteria" message. Is there a way to avoid this?

Done. This was harder than I thought, but the net result is much better for the observables and subscriptions, resulting in much less computation occuring.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 31 at r5 (raw file):

Previously, josephmyers wrote…

Can this say "Edit Renderings"?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 36 at r5 (raw file):

Previously, josephmyers wrote…

I don't want to be too picky here, but why is the toolbar at the bottom of this tab? Was this intentional? All our other tabs place the toolbar above the content, and it's weird that this one is backwards. Do we gain something from putting it at the bottom?

I was kind of forced. The column headings are sticky to the top, and having another sticky row at the top did not work in my experiments. I would really, really, prefer them to be at the top, but my limited CSS skills are failing me.

If you have a way to have both the toolbar and the column headings sticky at the top, that would be amazing, and I really would be thankful!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 66 at r5 (raw file):

Previously, josephmyers wrote…

I think this tooltip should just be "Settings" -- simpler

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.spec.ts line 184 at r1 (raw file):

Previously, josephmyers wrote…

Thanks. You did fix the bug, but you've introduced some slightly odd behavior to cope with the persistent "properties are out of date" issue in this class. By that I mean that it is weird and sad that we have to reset their category and range when the language changes. We're doing that to cope with the fact that the category, which gets updated, isn't found in the list of categories, because they're not updated til later. It's a code smell.

I'm sorry for belaboring the point, so I'm resolving this.

Yes, it is a code smell. Or perhaps a data smell. :-(

[josephmyers]

Cool, that's fixed. I am also seeing, though I'm not sure how long it's been here, a small graphical bug with the filter on page load. This happens when the Category is set to something other than All. Here's a brief video:
Recording 2024-08-28 082834.mp4

I may have found another bug. Unless we're wanting to allow multiple instances of the tab?

I think both the above issues should have test coverage, though I could give the former a pass if it's difficult.

I'm also seeing this tab take a noticeable amount of time to close. It's about a second, and I wasn't seeing this yesterday. Any ideas?

Reviewed 5 of 5 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 65 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. This was harder than I thought, but the net result is much better for the observables and subscriptions, resulting in much less computation occuring.

Looks so much better! Thanks for doing this. Can you add two tests that verify the message is not shown when loading and shown when done?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.html line 36 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I was kind of forced. The column headings are sticky to the top, and having another sticky row at the top did not work in my experiments. I would really, really, prefer them to be at the top, but my limited CSS skills are failing me.

If you have a way to have both the toolbar and the column headings sticky at the top, that would be amazing, and I really would be thankful!

It's not CSS -- it's HTML. Just move the <div class\="toolbar"\> above the table

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 183 at r6 (raw file):

  categories: string[] = [];
  columnsToDisplay = ['term', 'category', 'gloss', 'renderings', 'id'];
  rangeFilters: RangeFilter[] = ['current_verse', 'current_chapter', 'current_book'];

Can this and the one above be readonly?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 294 at r6 (raw file):

      this.projectDoc = await this.projectService.getProfile(projectId);
      this.projectUserConfigDoc = await this.projectService.getUserConfig(
        this.activatedProjectService.projectId ?? projectId,

Why do we need both here? Is there a chance that projectId is wrong? Whichever we need, it feels like the other one would cause problems.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 303 at r6 (raw file):

      // Subscribe to any project, book, chapter, verse, locale, biblical term, or note changes
      this.loadingStarted();
      let biblicalTermsAndNotesChanges$: Observable<any> = await this.getBiblicalTermsAndNotesChanges(projectId);

Make const, please

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts line 310 at r6 (raw file):

          this.chapter$,
          this.verse$,
          this.i18n.locale$,

Obviously there is some inefficiency in our data structure for BT, but conceptually, why does changing the locale affect the filtering? That doesn't seem right.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1187 at r6 (raw file):

            let projectDoc: SFProjectProfileDoc | undefined = undefined;
            if (tabData.projectId != null) {
              projectDoc = await this.projectService.getProfile(tabData.projectId);

So, looking at this if block, wouldn't this include BT tabs whose projectId is populated? If it has a projectId, a BT tab will use the projectService, but if it doesn't, it will use the activatedProject. That seems to contradict the intent of your comment below. Maybe further comments are necessary? Or flip these two conditionals?

I see the tests you added for this, but we should probably cover this specific use case, as well.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-factory.service.ts line 25 at r7 (raw file):

            svgIcon: 'biblical_terms',
            headerText: await firstValueFrom(
              this.i18n.translate('editor_tab_factory.default_biblical_terms_tab_header')

The tab title is weird when you reload the page. Is that coming from here?