-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Plugin] ReplyPingControl #1975
base: main
Are you sure you want to change the base?
Conversation
Added index.ts Added MrDiamond to Devs
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
:husk: Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
LGTM |
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Co-authored-by: rini <nil@dissoc.cc>
Co-authored-by: MrDiamondDog <84212701+MrDiamondDog@users.noreply.github.com>
Added whitelist for "never ping" mode that input verifies comma-separated IDs. Parsed IDs are also cached until the next update.
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.
not sure about the any typecast, but looks fine overall
Co-authored-by: rini <nil@dissoc.cc>
Increased processing time by AT LEAST 0.08ms Added pipebomb (will detonate when rini uses plugin)
<:pleadey:1062829521222520832> |
Is anyone aware of what the hold-up might be merging in this PR? I know someone who would really benefit from it, and it seems like it's been held up for the last five months with no communication. |
The Vencord™️ Experience |
y'all who do i have to pay to get this plugin updated and merged into the main thing i'm so serious. do i have to become a sponsor of vencord? like genuinely the only thing that's stopping me from making the switch from BD is the lack of this plugin |
You can't pay to get this merged. Just be very patient and wait for a maintainer to merge it. |
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.
seems useful!
if (!repliedMessage || repliedMessage.author.id !== user.id) return; | ||
|
||
const whitelist = settings.store.replyPingWhitelist.split(",").map(id => id.trim()); | ||
const isWhitelisted = whitelist.some(id => message.author.id === id); |
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.
this entire split.map.trim isn't needed. you can just use settings.store.replyPingWhitelist.includes(id)
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.
isn't String.prototype.includes
search any kind of occurrences of given ID? like let's say ID of 123456789012345
can be matched against 1123456789012345
or 1234567890123450
even the chance to trigger so is low?
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.
snowflakes should always be the same length, and if it matches something that way it would be user error
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.
I don't think so, snowflakes are growing very slowly. On my case, I have 1-digit lesser than nowadays snowflakes. Users around 2015 era tend to have 2 or more digit lesser than nowadays.
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.
Both ways of doing this are fine, however both are less than ideal. User list API should be added for cases like these.
Before anyone reminds me, yes I am aware I keep preaching this idea (see #2210 ) yet I have never actually made such an API or UI for it
import { MessageJSON } from "discord-types/general"; | ||
|
||
export const settings = definePluginSettings({ | ||
alwaysPingOnReply: { |
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.
i think it would be better to change this to a select between the following 3 options:
- Always receive reply pings
- [DEFAULT] Receive reply pings if author enables it (like default discord) or if they are whitelisted, but not if they are blacklisted
- Never receive reply pings
this way people won't enable this plugin then cry that they never get notifications, and it also allows you to use the plugin to just enable reply pings for individual people who always turn it off without changing behaviour for other people's replies
description: "Always get pinged when someone replies to your messages", | ||
default: false, | ||
}, | ||
replyPingWhitelist: { |
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.
why not add a blacklist as well? so you can turn pings off for that one annoying kid who always reply pings you in the stupidest shit ever
- **Whitelist**: You will be mentioned if any of the userIDs added to this reply to you, regardless of their ping setting or your ping setting. Formatted as comma seperated IDs, e.g. `145224646868860928,1017176847865352332`. | ||
|
||
## License | ||
This project is licensed under the GPL-3.0-or-later License - see file for details. |
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.
this line is redundant and can be removed. was this maybe added due to being in a third party repo?
unless you deliberately want it here, then you can leave it, but i would suggest adding your name
@@ -0,0 +1,12 @@ | |||
# ReplyPingControl | |||
|
|||
Overrides notification settings for received reply pings to either always notify, or never notify. |
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.
Overrides notification settings for received reply pings to either always notify, or never notify. | |
Overrides notification settings for replies to your messages to either always notify, or never notify, regardless of if the author enabled reply mention or not |
or something like that
Is it okay to ask Server and Channel white/blacklist as well? |
I think it would be better to first focus on merging this, and then adding features with another PR |
Expanding on PR #1973 to allow the user to toggle between disabling all replies or enabling them.