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.

Sign up for GitHub

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

Jump to bottom

Chapter dividers for the progress bar #4915

Merged
merged 9 commits into from
Mar 1, 2021
Merged

Chapter dividers for the progress bar #4915

merged 9 commits into from
Mar 1, 2021

Conversation

jonasburian
Copy link
Contributor

The chapter dividers proposed by @ByteHamster in #4898.
To make things easier, I've introduced a new class called ChapterSeekBar, inheriting from AppCompatSeekBar, in which the elements (progress bar, chapter dividers and thumb) are drawn in the correct order. Unfortunately, I had to disable the drop shadow by the thumb as it gets drawn under the progress bar. I found no way to change this for now.

Screenshot

@keunes
Copy link
Member

keunes commented Feb 1, 2021

Nice! I think the gaps between chapters might be just a wee bit smaller. What d'you think?

And another thought: should we implement support for chapters from the Podcast Index namespace, podcasters may have 'gaps' between chapters (e.g. excluding in-between jingles from the chapter time annotations. Would that potentially pose a problem for displaying this? (Or am I getting ahead of myself here?)

@ByteHamster
Copy link
Member

I would say let's just take the starting time of each chapter and assume that it completely fills the time to the next chapter (just like the current chapter support)

@jonasburian
Copy link
Contributor Author

jonasburian commented Feb 1, 2021 •
edited
Loading

I think the gaps between chapters might be just a wee bit smaller.

Here is a small comparison with the different margins in descending order (the top one is the current):

Screenshot_1612221095 png
Screenshot_1612221130 png
Screenshot_1612221146 png
Screenshot_1612221164 png

I think the third one looks the best.

@ByteHamster
Copy link
Member

I think the third one looks the best.

I agree 👍

ByteHamster
ByteHamster reviewed Feb 4, 2021
View reviewed changes
Copy link
Member

@ByteHamster ByteHamster left a comment

Choose a reason for hiding this comment

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

Works great :) Noticed a little problem related to rotation (see inline comment).

I looked at an episode that uses a huge number of short chapters. The progress bar looks kind of broken with so many gaps. Maybe it would be good to draw the background bar manually instead of using the progress drawable. That way, we can highlight the currently active chapter while dragging (like on YouTube). I think this makes it more clear that the gaps are intentional. Would you be up for doing that?

Looking at it on my real device, I think that the gaps could be a little bit smaller.

Additional idea: Out-of-scope for this PR but I think it would be pretty amazing to show the chapter title in that popup box while dragging the seek bar.

app/src/main/java/de/danoeh/antennapod/view/ChapterSeekBar.java Outdated Show resolved Hide resolved
@jonasburian
Copy link
Contributor Author

jonasburian commented Feb 4, 2021 •
edited
Loading

Would you be up for doing that?

Sure! But it could take a while (I'm a bit busy right now)

I think that the gaps could be a little bit smaller

Made it smaller. I think if it is even smaller, it could be too small for someone.

I think it would be pretty amazing to show the chapter title in that popup box while dragging the seek bar.

Thought about that too. Would be pretty nice :D

@ByteHamster ByteHamster added this to the 2.3.x milestone Feb 7, 2021
@jonasburian
Copy link
Contributor Author

jonasburian commented Feb 11, 2021 •
edited
Loading

draw the background bar manually

Made a first implementation of it. I need to do a bit more testing (and maybe change the colors and some other things), but I think it should be all right.

Something I thought about while doing this: What about having a small delay, something of a snap whenever you get to a chapter mark while dragging. This could be a solution for the UI problems in #4898 (and would somehow deprecate it). I'll continue to work on #4898 soon, when this is resolved.

@ByteHamster
Copy link
Member

Made a first implementation of it.

Nice work! I will have a closer look soon. I think this change should go into AntennaPod 2.3.x (together with #4898). Version 2.2.x already has some nice new features and now that the 2.1.3 bugfix is released, we can start the 2.2.x beta releases soon.

What about having a small delay, something of a snap whenever you get to a chapter mark while dragging.

Not sure if that will work for average episodes. Some of the podcasts I am listening to use a huge number of chapters. Navigating with the snappy slider could get pretty fiddly/annoying there.

Screenshot_20210214-223913_AntennaPod_Debug

@ByteHamster
Copy link
Member

Looks good to me. Ready for merging?

@jonasburian
Copy link
Contributor Author

I'm ready :D

@ByteHamster
Copy link
Member

Nice! This change will be released in AntennaPod 2.3.x

2.2.x beta releases will start soon.

@ByteHamster ByteHamster merged commit a476ce2 into AntennaPod:develop Mar 1, 2021
@jonasburian jonasburian deleted the feature-chapter-marks branch March 2, 2021 21:00
@ByteHamster
Copy link
Member

While playing around with #5075, I found 3 issues with the chapters in the SeekBar.

  • When the current position is just before the chapter separator, the separator gets eaten up by the buffering indicator. Then it looks like there is no chapter separator. You can find an extreme case in the episode "Chapters with 0 duration" in the feed https://tools.bytehamster.com/podcast/rss.xml
  • Using the same episode ("Chapters with 0 duration"), pressing the first chapter somehow does not work reliably
  • Using the episode "Chapters" (which has a wrong duration specified in the feed), the chapter mark separators are all squashed to the front of the SeekBar. I think the marked positions of the chapters should be updated when we get more reliable duration information from PlaybackService. It already happens for the duration text but not for the SeekBar.
    • Start episode "Chapters"
    • Pause + Force-Stop AntennaPod
    • Re-open
    • Observe that the duration TextView on the right shows 2 hours (that's what's specified in the feed) and the chapters are all squashed to the left (that's expected at this point)
    • Press play
    • Observe that the duration is now correct but the chapters are still squashed to the left

@jonasburian
Copy link
Contributor Author

Thanks for sharing! I'll look into it as soon as possible.

@jonasburian jonasburian mentioned this pull request May 3, 2021
Merged
3 tasks
@antennapod-bot
Copy link

This pull request has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/antennapod-2-3-0-release-notes/1047/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ByteHamster ByteHamster ByteHamster left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.3.x
Development

Successfully merging this pull request may close these issues.
4 participants
@jonasburian @keunes @ByteHamster @antennapod-bot