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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audio PR part 1.5 #3047

Closed
wants to merge 67 commits into from
Closed

Audio PR part 1.5 #3047

wants to merge 67 commits into from

Conversation

Drapersniper
Copy link
Contributor

@Drapersniper Drapersniper commented Oct 11, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

NO LONGER A DRAFT, but this one has all the goodies from #2950 minus the new Menu implementation.

  1. Play + Bump Combo Command (#2522)
  2. Don't shuffle bumped tracks (Added an extra argument to [p]shuffle that allows the person with permissions to say whether bumped tracks should be shuffled or kept at the top of the queue, off by default)
  3. Expand local track support to all file formats (Added support for AAC, AU, M3U, M4A, OPUS, RA, TS, WAV, WMA, 3GP, FLV M4V, MKV, MP4, MOV, WEBM, WMV)
  4. Cache DJ setting value to reduce api calls (more significant on Mongo)
  5. Rework UX to standardize messages (Summary of what message is about is the title, and a more descriptive explanation is added to embed description/footer)
  6. Add cooldowns to playlist save, eq save and summon, also add resets in error scenarios.
  7. Add a global variant of the existing whitelist/blacklist thats thats priority over the server level whitelist/blacklist

@aikaterna love you please don't kill me 馃憖

No need to assign to anyone just as of yet but in theory everything here work since #2950 is fully functional, but ... since this hasn't been tested it I may have messed up some things, once I've taken it through i spin i'll flag it as ready for review.

Update

This PR is ready to be reviewed

Drapersniper and others added 8 commits Aug 8, 2019
`[p]bankset maxbal` can be used to set the maximum bank balance

Signed-off-by: Guy <guyreis96@gmail.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
@Drapersniper Drapersniper requested a review from Twentysix26 as a code owner Oct 11, 2019
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
@aikaterna aikaterna self-assigned this Oct 11, 2019
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
@Flame442 Flame442 added Type: Enhancement Type: Feature Type: Fix labels Oct 11, 2019
Drapersniper and others added 6 commits Oct 11, 2019
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Draper <27962761+Drapersniper@users.noreply.github.com>
Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
@aikaterna
Copy link
Member

@aikaterna aikaterna commented Nov 23, 2019

This is a partial review, I intend to look at and test more parts of this PR.
Sorry for the line numbers mentioned in this text, my browser is not letting me display the larger diffs to comment on specific parts. If you have questions about any of these, please ask me in Discord.


Shuffle docstring needs to be title case on Bumped and removed an s, added "this"
"""Toggle shuffle.
Bumped tracks default to True.
Set this to False if you wish to avoid bumped songs being shuffled.
"""


Use [p]local play
Select a folder that has files in it
The listing footer says "search results" instead of 鈥渓ocal folders鈥 or 鈥渓ocal files鈥


Please display Shuffle-bump settings (true/false) in the response to the user
Also not sure why this was added as a bool to shuffle when it is stored as a setting, and could be a setting. I could be convinced otherwise on this though and it's fine as is with True or False as the arg, just seems awkward to me.


Queue footer:

Page 1/16 | 154 tracks, 11:08:30 remaining |

Auto-Play: | Shuffle: | Repeat:

Remove extra pipe on top line after "remaining" if the footer will always be 2 lines. Also please remove the extra line return inbetween the two lines.


The removal of "\localtracks" from queueing local tracks with play commands (including bumpplay), which was made in the Lord of Audio PR, should be mentioned in release notes (please correct me if this was mentioned somewhere, I remember us talking about it previously, but I just want the users to be aware)

Example command: [p]bumpplay ENM\501 - Inside The Machine.mp3
Was expecting: [p]bumpplay localtracks\ENM\501 - Inside The Machine.mp3


Extra space before "Raises" in docstring in playlists.py under deleting a playlist


Was it Black that forced "0 to disable" to a 2nd line on the docstrings for audioset emptypause, audioset emptydisconnect, and audioset vote? I feel like that should be shown to users by default and not have them have to run help specifically on that command to know that. Also audioset maxlength has this descriptor for "0 to disable" and it was not touched... if you were trying to standardize this change.


The formatting for the track display for audioset status needs to be changed: see https://i.imgur.com/zwldslS.png


Why is there an await ctx.embed_colour() here? (Line 2034)


description=_("You must be in the voice channel pause or resume.") (Line 2615)
Should be "You must be in the voice channel to pause or resume."

@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Nov 24, 2019

Morning forward will work on a fork of audio

Michael H added 2 commits Nov 26, 2019
 - Is in direct conflict with goals stated in #2804
 - Features this was intended to enable can be enabled in other more
 appropriate ways later on
@Drapersniper Drapersniper reopened this Nov 26, 2019
@aikaterna
Copy link
Member

@aikaterna aikaterna commented Dec 12, 2019

[p]local play shows 鈥渪 local folders鈥 on the folder page, and shows 鈥渪 local folders鈥 on the track listing page as well. Was expecting 鈥渪 local tracks鈥 on the track listing embed. Local search returns the correct footer now, 鈥渪 local tracks鈥

Using shuffle with no subcommand displays the help for shuffle and also toggles the setting, instead of just toggling it.

Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: Drapersniper <27962761+drapersniper@users.noreply.github.com>
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone Dec 19, 2019
鈥rdBot into audio-misc-pt1

锟 Conflicts:
锟	redbot/cogs/audio/audio.py
锟	redbot/cogs/audio/utils.py
鈥nd/Red-DiscordBot into audio-misc-pt1

锟 Conflicts:
锟	redbot/cogs/audio/audio.py
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Dec 21, 2019

Do not merge this one ... Merge #3205

@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Dec 22, 2019

Will setup a CPR for all of this so itsd easier to follow ... less likely to fuck up)

@jack1142 jack1142 added the Type: Bug label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Type: Enhancement Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants