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

[V3 Audio] Add track length restriction #2465

Merged
merged 4 commits into from Feb 24, 2019
Merged

[V3 Audio] Add track length restriction #2465

merged 4 commits into from Feb 24, 2019

Conversation

@aikaterna
Copy link
Member

@aikaterna aikaterna commented Feb 17, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

  • Added [p]audioset maxlength, which takes seconds or 00:00-style of formatted input for restricting the player to songs that have a length under that threshold.
aikaterna added 2 commits Feb 17, 2019
Extra whitespace on title headers is to prevent setting output from wrapping on desktop clients.
async def maxlength(self, ctx, seconds):
"""Max length of a track to queue in seconds. 0 to disable.
Accepts seconds or a value formatted like 00:00:00 (h:m:s) or 00:00 (m:s).
Copy link
Member

@TrustyJAID TrustyJAID Feb 21, 2019

Choose a reason for hiding this comment

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

(h:m:s) should be (hh:mm:ss) or (h\\:m\\:s) otherwise discord renders the :m: as an emoji. Alternatively surround the time markers with backticks.

Loading

return True

async def _time_convert(self, length):
match = re.compile(r"(?:(\d+):)?([0-5][0-9]):([0-5][0-9])").match(length)
Copy link
Member

@TrustyJAID TrustyJAID Feb 21, 2019

Choose a reason for hiding this comment

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

Replacing this regex with (?:(\d+):)?([0-5]?[0-9]):([0-5][0-9]) can help improve usability where users can supply 8:00 instead of 08:00 for setting the max length a minor usability change.

Loading

Copy link
Member Author

@aikaterna aikaterna Feb 23, 2019

Choose a reason for hiding this comment

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

Thank you for this.

Loading

length = round(track.length / 1000)
except AttributeError:
length = round(track / 1000)
if length > 259200:
Copy link
Member

@TrustyJAID TrustyJAID Feb 21, 2019

Choose a reason for hiding this comment

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

I'm curious about this and if it might have potential problems. Is this in case we're playing a live stream we don't care about the maxlength? Will this cause issues for streams that don't return a value of 72 hours?

Loading

Copy link
Member Author

@aikaterna aikaterna Feb 23, 2019

Choose a reason for hiding this comment

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

Decided to move this value more towards the upper limit instead and added a comment regarding livestream length.

Loading

Copy link
Member

@TrustyJAID TrustyJAID left a comment

Working good after testing changes.

Loading

@Tobotimus Tobotimus merged commit b0ab6bd into Cog-Creators:V3/develop Feb 24, 2019
1 check passed
Loading
@aikaterna aikaterna deleted the patch-6 branch Feb 24, 2019
Tobotimus added a commit that referenced this issue Mar 7, 2019
Searching for a song and pressing the reaction to queue a song would not add the song to the queue if `[p]audioset maxlength` was off. This was an omission from #2465.
Tobotimus added a commit that referenced this issue Mar 10, 2019
- Seek can now seek to a specific position, formatted like 00:00:00 or 00:00. Using negative or positive ints still functions the same as previously and will seek ahead or behind by that value instead.
- This PR requires the `_time_convert` func added in #2465 and should be merged after that one.
@jack1142 jack1142 added this to the 3.1.0 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants