estroBiologist / pluralchum

PluralKit integration for BetterDiscord
MIT License
41 stars 11 forks source link

Servername enabled causes inconsistent spacing between name and systemtag #15

Closed vmorrisonwood closed 10 months ago

vmorrisonwood commented 1 year ago

Disclaimer: I know servername is buggy! But it's a feature I rely on, and I previously used @Draconizations patched version when running PluralChum to get them working, and they worked perfectly.

In the newest build, servernames do display, but when the setting is enabled, it causes inconsistent spacing between the proxy name and it's systemtag, regardless of whether its a servername or not.

image

First line is a member with a servername enabled - space between the name and » Ouro Second line is a member with no servername enabled - space between the name and » Ouro Third and fourth lines are the same members, with the servername setting turned off.

image

First line is a member with a servername enabled - no space between the name and » Ouro Second line is a member with no servername enabled - space between the name and » Ouro

As you can see, it inconsistently applies - the member Starburst always has the space correctly applied, but Tanz does not. Kalashnikov doesn't have the space correctly applied, but Master does. There should not be any excess whitespace in the names, but this is hard to check as you can't as reliably prod the API for servername info.

An ideal implementation for servernames (which is what Draconizations originally applied) would just be to have PluralChum respect the webhook's name and simply colour it, without prodding the API for any other info. This would remove alternate system colour functionality, but we personally don't use this, so it'd be a worthwhile trade-off.

Hope this makes sense! Sorry for any confusing explanation, but hopefully I conveyed it well.

Draconizations commented 1 year ago

I can definitely open a PR for adding exactly what my patch does, except hidden behind a setting, if there's interest in that!

Although I will warn you that it also includes an extremely cursed regex that fixes any emoji broken by discord removing a non-printing characters from names. Not sure if I should even bother including that because it checks for any emoji that needs that character... manually... so it's 6000+ characters long.

estroBiologist commented 1 year ago

hm. i'm honestly not stoked about the notion of sacrificing system tag colour functionality? the current servername implementation is already a bodge, and i don't feel great about adding more on top of that when it was never a real solution in the first place.

at the end of the day, i think the real fix for problems like this (as well as other PC limitations when it comes to server-specific overrides) is to just bite the bullet and store the user's PK token. it's something i've been hesitant to do since the beginning because of the security concerns, but it's not like a hypothetical malicious BD plugin couldn't do worse stuff regardless.

i dunno. can't promise i'd have the time and energy to implement that change either way, and it's still a lot of responsibility to store those tokens, so i think for now it's just something we'll continue to have to live with. and while i appreciate the PR offer, i think i'm just not a big fan of the trade-off that patch makes. sorry, y'all

vmorrisonwood commented 1 year ago

I see, thank you for the response. I will probably continue to use patched versions in that case then as we use servernames liberally. While the system tag colour function is nice and thematically appropriate, we find it's more visually disruptive for our own use of PK, hence why the trade-off doesn't bother us. We have it disabled either way, as we prefer the uniform application of just one colour, haha. But I understand your viewpoint! Thank you for creating PluralChum either way, it's genuinely the only reason we have BD installed in the first place.

viiuan commented 1 year ago

Having a similar issue where this can cut off certain displaynames, even those that aren't servernames.