Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transcripts: Display speaker info #2665

Merged
merged 13 commits into from
Aug 21, 2024
Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Aug 18, 2024

Description

This displays speaker info for transcripts.

Testing Instructions

  1. Load an SRT format transcript with speaker info (e.g. from Cautionary Tales)
  2. ✅ Notice that speaker info is displayed properly
  3. Prioritize order for VTT format in TranscriptsManagerImpl.supportedFormats at line 22
  4. Install the app and load a VTT format transcript with speaker info (e.g. from Build your Saas)
  5. ✅ Notice that speaker info is displayed properly
  6. Search for speaker name
  7. ✅ Notice that speaker names are included in search results and highlighted correctly

Screenshots or Screencast

SRT VTT

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr added this to the Future milestone Aug 18, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 18, 2024

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commit223cfb2
Direct Downloadpocketcasts-app-prototype-build-pr2665-223cfb2.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commit223cfb2
Direct Downloadpocketcasts-automotive-prototype-build-pr2665-223cfb2.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commit223cfb2
Direct Downloadpocketcasts-wear-prototype-build-pr2665-223cfb2.apk

Comment on lines 367 to 369
builder.text = parseCueText(id, markup, styles);
// parseCueText strips tags like TAG_VOICE "v" and set it on the cue text. This sets original markup wih tags in cue to allow extracting it in the application.
builder.markup = markup;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cue.text for vtt format parser strips out tags like TAG_VOICE "v" without setting it on the cue. This sets original markup with tags on the cue to allow extracting any tag in the application in 7548a3f.

I copied original source code for WebvttCueParser and Cue to make the change as the classes were final and could not be subclassed. Curious to know if there's a better way to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you report the issue in the media repository? Hopefully they can add needed APIs and we can remove copied classes in the future. If they agree that this is something they could add we could make a PR to their repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked here: androidx/media#1632

@ashiagr ashiagr marked this pull request as ready for review August 18, 2024 06:48
@ashiagr ashiagr requested a review from a team as a code owner August 18, 2024 06:48
@ashiagr ashiagr requested review from geekygecko and removed request for a team August 18, 2024 06:48
@ashiagr ashiagr mentioned this pull request Aug 18, 2024
Copy link
Member

@geekygecko geekygecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked well and look amazing.

@ashiagr
Copy link
Contributor Author

ashiagr commented Aug 21, 2024

👋 @geekygecko

I'm reverting support for speaker info in VTT transcripts. I'm having issues including library class duplicates in the release apk in minifyReleaseWithR8 task.

Also, including library class duplicates may cause issues in code maintenance. I'll try supporting speaker info in VTT transcripts separately.

@ashiagr ashiagr merged commit d7a5230 into main Aug 21, 2024
14 checks passed
@ashiagr ashiagr deleted the task/2398-display-speaker-info branch August 21, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants