Islandora-Labs / islandora_solution_pack_oralhistories

Adds all required Fedora objects to allow users to ingest and retrieve Oral Histories (video/audio) files through the Islandora interface

GNU General Public License v3.0

13 stars 23 forks source link

update vtt converter lib #141

Closed Natkeeran closed 5 years ago

Natkeeran commented 5 years ago

What does this Pull Request do?

Addresses this issue: https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/issues/137

How should this be tested?

Get the PR
Upload webvtt transcripts
Verify that they are correctly parsed and displayed in the transcribe table/viewer

Additional Notes:

We have to add an exception here to catch invalid vtt issues: https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/blob/ca77d27ef03b2147c8893f899d8470a1e131437c/includes/utilities.inc#L34

Interested parties

@MarcusBarnes

MarcusBarnes commented 5 years ago

Tested and appears to work as expected. I'll give those who reported the issue a little time to double check before I merge. Thanks!

MarcusBarnes commented 5 years ago

@theMusician would you be able to get things rolling so that your team (including Tony Kurtz) would be able to test this pull-request? Thanks in advance.

theMusician commented 5 years ago

Testing in progress.

theMusician commented 5 years ago

Here is the feedback from our testing. Not sure if this is duplicate speaker name issue has to do with the change or if that is just an issue with our theme. This was tested on the most recent 7.x branch.

The transcript cues are all synced correctly now, which is fantastic. However, the transcript viewer now displays the speaker name twice: once as the speaker (which you can toggle on or off) and again as the first value in the cued line of the transcript itself. The effect is that, if you toggle the speaker name ON, the speaker name displays twice (and overlaps in our interface); if you toggle the speaker name OFF, the speaker name still displays in the transcript column as the first value. Below are screenshots of this in action (from here):

Transcript and Speaker both toggled ON [speaker name displays twice]:

Transcript toggled ON, Speaker toggled OFF [speaker name still shows up at beginning of transcript content]:

MarcusBarnes commented 5 years ago

@theMusician Thanks for testing. Would you be able to send me a sample object via email to make sure we can reproduce on our end? Thanks!

tonykurtz commented 5 years ago

@MarcusBarnes Hi - I work with @theMusician. Here's a copy of the Transcript (vtt) file I uploaded for the sample video. Will this work? ChuckandDeeRobinson_en.txt

MarcusBarnes commented 5 years ago

Thanks @tonykurtz. I'll give it a try.

MarcusBarnes commented 5 years ago

@theMusician @tonykurtz I've been able to reproduce. The speaker name is getting included in the Transcript text.

MarcusBarnes commented 5 years ago

@theMusician @tonykurtz We've addressed the Speaker name issue you spotted. Would you be able to pull down the latest changes added to this pull-request and give things one more test before I merge? Thank you in advance.

MarcusBarnes commented 5 years ago

Merging. Tests confirm that issue #137 appears to be fixed. The appearance of the speaker name appearing in the transcript during earlier testing of the pull-request has also been addressed.

theMusician commented 5 years ago

Bingo is what we have heard. Working great. Thank you!