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

Add discord roles blacklist and aliases for chat format #5157

Merged
merged 8 commits into from
Aug 9, 2023
Merged

Add discord roles blacklist and aliases for chat format #5157

merged 8 commits into from
Aug 9, 2023

Conversation

x86-39
Copy link
Contributor

@x86-39 x86-39 commented Nov 16, 2022

Information

This PR partially closes #5155

Details

Proposed feature:
Add options to only select certain Discord roles and to add aliases for roles.

This is parallel to the DiscordChatChannelRoleAliases and DiscordChatChannelRolesSelection options in DiscordSRV.

This can be used to ignore certain roles, by only selecting roles relating to Minecraft to be processed by the plugin.

The aliases allow for mapping names in Discord to in Minecraft. For example "Admins" can be mapped to "Admin". This can also be used to hardcode which colour a role should be in Minecraft by setting the alias to include a colour code for example: "§9Admin"

Environments tested:

OS: Fedora 36

Java version: 17.0.5

  • Most recent Paper version (1.XX.Y, git-Paper-BUILD)
  • CraftBukkit/Spigot/Paper 1.12.2
  • CraftBukkit 1.8.8
    Don't see how this could differ on other server versions.

Demonstration:

# List of roles which are used for the top role placeholders. If this is empty, all roles will be allowed.
discord-roles-selection:
  - "Admins"
  - "Members"

# Aliases for Discord roles before they are sent to Minecraft.
discord-roles-aliases:
  Admins: "§9Admin"
  Members: "Member"

The Founder role gets ignored and the Admins role gets translated to §9Admin
image
image

@x86-39 x86-39 changed the title Add roles selection option Add roles selection option for Discord Nov 16, 2022
Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

Both these features are fine, just a few things;

  • discord-roles-selection should be made into a blacklist rather than a whitelist, be renamed to discord-role-blacklist, and the config should have the examples commented out (like how world-aliases are in the main essentials config).

  • discord-roles-aliases should support both discord role names and ids and have their examples commented out as well.

@x86-39
Copy link
Contributor Author

x86-39 commented Nov 19, 2022

I feel like it being a whitelist is still desired in many cases, perhaps an option to turn the whitelist into a blacklist? This is how DiscordSRV does it too.

Will add it to support IDs and comment it out in the config tomorrow

@JRoy
Copy link
Member

JRoy commented Nov 19, 2022

I feel like it being a whitelist is still desired in many cases, perhaps an option to turn the whitelist into a blacklist? This is how DiscordSRV does it too.

I think a majority of users will only have a couple of roles they don't want used as top role, such as a default role, rather than a bunch of unwanted top roles

@x86-39
Copy link
Contributor Author

x86-39 commented Nov 19, 2022

Maybe a blacklist by default (renaming it to blacklist too) with an option for it to be a whitelist?

@JRoy
Copy link
Member

JRoy commented Nov 19, 2022

Maybe a blacklist by default (renaming it to blacklist too) with an option for it to be a whitelist?

I think that would become confusing, do you have an example of a common use case for a whitelist would be better over a blacklist?

@x86-39
Copy link
Contributor Author

x86-39 commented Nov 19, 2022

We have staff ranks inside Minecraft which are roles present in Discord, but the Discord has more roles than that. When messages go to the Minecraft server, we only care about those staff roles going to Minecraft. Having a whitelist of just the few staff ranks is a lot easier then.

@x86-39
Copy link
Contributor Author

x86-39 commented Dec 11, 2022

@JRoy Any input on this? If you agree with the whitelist suggestion I can implement it and finalise this PR

@JRoy
Copy link
Member

JRoy commented Dec 11, 2022

@JRoy Any input on this? If you agree with the whitelist suggestion I can implement it and finalise this PR

Sorry, though I had replied, still convinced that a blacklist would be more useful by default. You can add a boolean config option invert-discord-role-blacklist which is not in the config by default (and has false as its default value) in order to satisfy your usecase.

@x86-39
Copy link
Contributor Author

x86-39 commented Dec 11, 2022

Yeah a blacklist by default is fine, I'll add a boolean thank you. Will modify the PR when I have the chance

@x86-39
Copy link
Contributor Author

x86-39 commented Mar 21, 2023

Hey @JRoy, apologies I completely forgot about this pull request.

I just made the requested changes and updated the branch.

I also updated the other pull requests I have open:
#5159
#5158

@pop4959 pop4959 added type: enhancement Features and feature requests. module: discord Issues or PRs for the EssentialsDiscord module labels May 5, 2023
JRoy
JRoy previously approved these changes Aug 9, 2023
@JRoy JRoy changed the title Add roles selection option for Discord Add discord roles blacklist and aliases for chat format Aug 9, 2023
@JRoy JRoy enabled auto-merge (squash) August 9, 2023 01:34
@JRoy JRoy merged commit e3e52db into EssentialsX:2.x Aug 9, 2023
Starmism pushed a commit to valence-smp/Essentials that referenced this pull request Jan 8, 2025
…5157)

Co-authored-by: Josh Roy <10731363+JRoy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: discord Issues or PRs for the EssentialsDiscord module type: enhancement Features and feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants