-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Transcript semantic parsing #6852
Transcript semantic parsing #6852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added some comments below
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
parser/feed/src/main/java/de/danoeh/antennapod/parser/feed/PodcastIndexTranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/feed/src/main/java/de/danoeh/antennapod/parser/feed/PodcastIndexTranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/feed/src/main/java/de/danoeh/antennapod/parser/feed/PodcastIndexTranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/feed/src/main/java/de/danoeh/antennapod/parser/feed/PodcastIndexTranscriptParser.java
Outdated
Show resolved
Hide resolved
...src/test/java/de/danoeh/antennapod/parser/feed/element/PodcastIndexTranscriptParserTest.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/de/danoeh/antennapod/parser/transcript/PodcastIndexJsonTranscriptParser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. I'm currently on a business trip that is quite packed with things to do. Finally found a bit of time. Comments below :)
model/src/main/java/de/danoeh/antennapod/model/feed/Transcript.java
Outdated
Show resolved
Hide resolved
...cript/src/main/java/de/danoeh/antennapod/parser/transcript/PodcastIndexTranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/transcript/src/main/java/de/danoeh/antennapod/parser/transcript/SrtTranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/transcript/src/main/java/de/danoeh/antennapod/parser/transcript/SrtTranscriptParser.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/de/danoeh/antennapod/parser/transcript/PodcastIndexTranscriptParserTest.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/de/danoeh/antennapod/parser/transcript/PodcastIndexTranscriptParserTest.java
Outdated
Show resolved
Hide resolved
No worries, enjoy your trip. |
Looking at the code again, I think we should make the properties of |
I assume you mean these properties below? In order for a future PR to dynamically display transcripts, I will need to modify the contents of
Here is a future PR
|
Instead of modifying the existing transcript specifically for the UI, I would rather create a copy with the new line breaks. That way we ensure that the broken-up transcript doesn't somehow end up in the other UI pages where it needs to be broken up differently. |
Making a copy seems very wasteful of memory and execution time. As I dig into the details of displaying transcripts and supporting users scrubbing to random locations, I end up needing to dynamically chunk up the transcript segments. Another way I could do this is probably with a cursor that walks the transcript segments for display. I will give that a try |
Don't we need a copy anyway because we need to re-layout on screen size changes? There we probably want to start fresh instead of keeping to modify the same transcript on subsequent size changes/rotations, which would otherwise look different every time and probably even degenerate at some point. How large are these objects? Is having them twice really such a big problem? |
I'll start implementing without a copy and see if I can solve it with a cursor instead. I'm not sure why I personally cannot visualize how to solve it by copying since the transcript segments will need to be layout more than once since the user can scrub the time, fast forward, rewind and rotate the screen any time. Doesn't seem to make sense to copy and redo the layout for the entire Transcript when only the current and next N segment of text needs to be rearranged. |
This is done, the values are final in TranscriptSegment |
@ByteHamster What do you think of merging? |
I did a few changes, let me know what you think :) About displaying, I think we need to discuss a bit again. In my opinion, most users don't need the live transcript, just the full one for search/sharing/etc. So displaying it on the main cover screen by default adds quite some complexity and "movement" for everyone while helping just a few users. Maybe we can discuss this at the community call next Saturday. |
looks good, thanks! |
Thanks! |
fix #4935 Parsing of podcast:transcript with a simple unit test. After this is merged, my next PR will be able to display 2 lines of transcript in the CoverFragment
@ByteHamster let me know if you wanted more unit tests