I really like this feature! It looks cool to take away the text and see only the chapter audio with the questions and answers. And our GUI adapts to the absence pretty well. It will be really interesting to see this once all the open PR's for scripture audio get merged, particularly the more usability-focused ones. In fact, I think we should wait til some of them are before merging this, so that we can make sure the workflow really clicks. For example, how does it work when we swap the Delete button for Close? How does this work when finishing the chapter hides the audio player?

I also like that you can minimize the player while listening to get more screen space. Good thinking!

I found a minor issue where if you enable the setting and delete all audio files, it breaks things. Plus, you can't uncheck the setting until you had audio back.

I'm also wondering if we need a place to indicate to admin's when audio projects have chapters that don't have audio yet. What happens if checkers log into these projects and there's no way to listen to or read the verses? Aren't they screwed? What do you think? Maybe that's a separate feature.

Reviewed 17 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marksvc)

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.html line 138 at r2 (raw file):

                unit="pixel"
              >
                <as-split-area [size]="hideChapterText ? 109 : $any('*')" [maxSize]="hideChapterText ? 109 : null">

Is there any way we can not use magic numbers here? It makes this part brittle. For example, when the audio is unavailable/missing, it takes up too much space, since the player is shorter.

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.scss line 125 at r2 (raw file):

    #answer-panel {
      height: 100%;
      padding-top: 12px;

I think this guy looks funky now. We probably still need it, just elsewhere?

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1178 at r2 (raw file):

  }

  private showOrHideScriptureText(): void {

This is misnamed (or missing functionality). It only shows the player, never hides it.

src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.html line 130 at r2 (raw file):

                </div>
              </div>
              <div class="tool-setting" *ngIf="featureFlags.scriptureAudio.enabled && hasTextAudioDocs">

I'm a little skeptical about the extra hasTextAudioDocs check here. Let's say they want to check this setting and then add their audio files. They will come here and not find anything. Thoughts?

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 268 at r2 (raw file):

    "translations_will_be_suggested": "Translations for words and phrases will be suggested as you edit Scripture based on the project or resource selected above.",
    "users_can_share_the_project": "Allow users to invite people outside Paratext to the project. They will receive the same role as the user who invited them.",
    "hide_community_checking_text": "Do not show Scripture text in Community Checking area. Checkers will need to listen to the Scripture audio.",

In my opinion, this is a little wordy. We could save some space by saying "Hide Scripture text" and moving the second sentence to a tooltip with app-info. I don't think we need to specify Community Checking again, since we're in the Community Checking section. What do you think?

I don't know why there is trouble with your green checkbox. Here is a video showing how it's working on my machine. I wonder if it would be helpful for you to enable the Scripture audio feature before going to the Settings page; maybe that might influence the behaviour? We can play with it together on a video call and see if we can figure it out.

[See chat for video scr-audio-checkbox.webm]

In fact, I think we should wait til some of them are before merging this, so that we can make sure the workflow really clicks. For example, how does it work when we swap the Delete button for Close? How does this work when finishing the chapter hides the audio player?

Okay. I think my preference would be to get it merged in without waiting. Then as we make changes like to the Delete button or hiding the audio player, it can be in light of this positioning and also the reality that text will be hidden sometimes.

Plus, you can't uncheck the setting until you had audio back.

Right! That's something we'll have to decide how to handle. My thinking is, if a project does not have any audio files, but has hide-text enabled, then we should still show the hide-text checkbox so they can uncheck it. (Okay, as discussed later, I made the checkbox visible regardless.)

I found a minor issue where if you enable the setting and delete all audio files, it breaks things.

Yeah. We'll need to decide what to do in situations where a project admin wants hide-text, but also doesn't have audio for a chapter. Probably there should be a graphic or something to indicate that audio is missing for that chapter.

I'll start on this now. It could go into a followup PR if we finish the review before I'm done.

I'm also wondering if we need a place to indicate to admin's when audio projects have chapters that don't have audio yet. What happens if checkers log into these projects and there's no way to listen to or read the verses?

So here's some useful framing. My thinking before was that maybe we could show text for chapters that don't have audio (like even if hide-text was enabled). But Nathaniel's picture of the hide-text situation is that some language projects don't want to show the text at all. So I'm seeing the hide-text setting as an all-or-nothing setting. You either hide all the text, or you don't hide all the text.

So, yes, I do think it would be useful to project admins to be able to see that the have audio uploaded for Mark 1-8 and 10-16, but are missing audio for Mark 9. But because of the above framing, if there is no audio for Mark 9, then in a hide-text situation there will be no Scripture for the checkers to work with until it's uploaded. (Rather than us trying to compensate by showing the text.)

As we play with the audio feature we may find that it's easy to upload all your chapters, or obvious to an admin if a chapter is missing. We can see.

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

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.html line 138 at r2 (raw file):

Right. I would prefer to make it some how be set to the size of the height of the audio player, or perhaps the height of the visible contents. I wasn't successful at finding a way to do that, and so manually determined the required px needed. Do you have any suggestions?

Regarding when the audio is missing, I don't think we need to worry about that for this situation. We can design a no-audio-available thing to show. It can still be 109 px tall, and if there is no audio available, and hide-text is enabled, then it's not a chapter for checkers to work in.

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.scss line 125 at r2 (raw file):
Regarding the white padding above "1 Answers": If I turn off hide-text and show the Scripture, I do see that the Scripture text does not have a corresponding bottom padding. It might be that the question area padding at the top should be eliminated. That might be something to throw out to the team to get UI opinions on, and make a jira issue for.

looks funky now

I don't think this is something new with this PR, is it? I see the behaviour when hide-text is off.

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1178 at r2 (raw file):

Ahh. But consider! In ngOnInit(), we do an initial showOrHideScriptureText(). Then we listen to this.projectDoc.remoteChanges$, upon which we again call showOrHideScriptureText().

That way the display in the community checking area will be dynamically updated in response to project settings changes regarding hide-text. For example, if you open one window and go to a chapter with audio. And you open another window and go to Settings. Then you select hide-text in the Settings window, the Checking window will instantly update by hiding the text and showing the audio player. If in Settings you clear hide-text, then in the Checking window, the text is again revealed (but it doesn't close the open audio player).

See the below video for a demonstration.

So I would say the method does cause the text to be hidden or shown. And there is a bonus effect of showing the audio player if hide-text is enabled.

But maybe there's something I can improve so it's more evident?

[see slack for hide-dynamic.webm]

src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.html line 130 at r2 (raw file):

Hmm. If a user has a text project and no Audio files, my thinking was that I don't want them to go turn off showing their Scripture text. And talking about hiding Scripture text and showing audio might not even make sense if they are all about thinking of showing the text and people engaging with the text.

That said, (1) making the hide-text checkbox sometimes show and sometimes not show might be less desirable for support and help documentation. And (2) if they admin goes and turns off Scripture text, and the text disappears, that's what they told it to do and they can come turn it back on again.

So maybe we should just have the checkbox be visible all the time. I'll go ahead and do that.

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 268 at r2 (raw file):

Good point. We could fit in both thoughts if we use "Hide Scripture text. Checkers will listen to audio." What do you think about that?

Okay, check it out, if there is no audio for a chapter, then part of the player is shown, but also "Audio unavailable". The cod eto do this was already there, but there was a particular situation that we were hitting where we only saw the back,play,forward,trash buttons, and not the "Audio unavailable" message. I adjusted the code so that it more consistently shows this message when there is no audio. The player is not 109 px high. I'm not sure that's a big deal. And if a user gets this player showing on their screen in a situation where they do want to be looking at Scripture text, it may be good for it not to be taller. Do you think it's significant for the "Audio unavailable" player to be matching the full height of the text+audio split area?

See video of behaviour:
audio-unavail-after-delete.webm

Reviewable status: 13 of 19 files reviewed, 9 unresolved discussions (waiting on @josephmyers)

There's assuredly more work to be done on this, but what you have looks good. Thanks for sticking with it!

Reviewed 6 of 6 files at r17, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @github-code-scanning[bot])

Thanks for looking at the design with me!

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @github-code-scanning[bot])