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

moreUserTags: attempt to rewrite #3170

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft

Conversation

henmalib
Copy link
Contributor

@henmalib henmalib commented Jan 27, 2025

Reason:
image

While discord hasn't pushed to stable i'm trying to rewrite patches, so they work both on canary and stable

Currently I've got tags working in messages:
image

  • Messages
  • Threads
  • Member list
  • User profile
  • Ensure stability
  • Maybe refactoring

@Vendicated
Copy link
Owner

by rewrite I meant make patches less dependant on one another. for example, implement this without adding new members to Discord's enums

@Vendicated
Copy link
Owner

right now, the plugin targets so many different modules and if even one single patch breaks, everything else stops working and either causes crashes or makes everyone have APP tag or stuff like that

more robust means that the plugin either works or fully breaks (silently), not constantly this in between

@henmalib
Copy link
Contributor Author

by rewrite I meant make patches less dependant on one another. for example, implement this without adding new members to Discord's enums

Well, I'll look what i can do about it
Currently I'm saving tagObj to use it later without dependency, but without a patch for creating tags discord doesn't resolve it as a number ( maybe i just too eepy rn, will look later how to avoid it )

right now, the plugin targets so many different modules and if even one single patch breaks, everything else stops working and either causes crashes or makes everyone have APP tag or stuff like that

more robust means that the plugin either works or fully breaks (silently), not constantly this in between

That's how i see it:

  • fewer patches
  • patches are easier to read/modify
  • patches shouldn't depend on one another
  • patches shouldn't depend on things that might change easily (like current one for messages, I've already tried to make it better than before, but yeah, it needs improvement )

@henmalib
Copy link
Contributor Author

Got it working without adding/changing discord's tags object, gonna eep now

image

@Vendicated
Copy link
Owner

nini

src/plugins/moreUserTags/index.tsx Outdated Show resolved Hide resolved
src/plugins/moreUserTags/index.tsx Outdated Show resolved Hide resolved
@henmalib
Copy link
Contributor Author

While reading through the deleted file I've got a question:

Should i support data attributes for theming? If I'm not wrong you can just grab content in css for same results

Tags inside of profile were reliant on 2-3 patches, I'll try to reduce, but if I wouldn't find a way to do this: should i just drop support for it? Seeing tags from member list/message isn't that hard

@henmalib
Copy link
Contributor Author

Okay, so OP tags are working out of the box, i have no idea why original one had a patch with a comment "show OP tags correctly"
image

Also I've added member list
image

I've left a comment how it currently patches discord code and I'm not sure if it's gonna be safe enough ( kinda feel weird because of the assignment inside parenthesis).
If someone can give a feedback about current state of patches it will be appreciated

@henmalib
Copy link
Contributor Author

Tags in profile are also now here
image

It doesn't work with bots and requires 2 patches... I'm not happy about it

short explanation why:
Render DOM looks like:

server_profile(user,channelId):
	top_part(user)

Username, pronouns, tags are rendered inside top_part and for tags we need channelId, so the only way i know is prop it down from server_profile

It works and if patch with server_profile dies or doesn't exists at all - we will just get 0 tags in profiles ( bot tags will be present ).

Original plugin has 4 patches for profiles, including bot ones, so idk how to proceed with it

2 patches with "safe fail" seems reasonable, but still not perfect
adding 3rd patch just for tags for bots seems a lot
dropping profile support altogether - easier to maintain, lose some functionality ( are many people need tags there? )

@henmalib
Copy link
Contributor Author

Using MessageDecoratior API i can get almost the same results without using any patches ( thanks theCodeToad )

image

But we are losing tags in replies like here
image

And bots are... ugh, should find a way to remove default one
image

ngl, i feel like a Davin doing all these comments

@henmalib
Copy link
Contributor Author

henmalib commented Jan 29, 2025

lange rede kurzer sinn

It looks like i can't remove default tag without another patch, so what's the point of decorations if it still requires patching?

Also MessageDecorationProps.decorations seems to be undefined

@OIRNOIR
Copy link

OIRNOIR commented Jan 30, 2025

Will there be an option to set tag colors as the guild member color (similar to #2161)? I personally think user tags are informative but very ugly when members' tags use the bot colors because it makes it harder to differentiate between priviliged members and bots at first glance.

@henmalib
Copy link
Contributor Author

Will there be an option to set tag colors as the guild member color (similar to #2161)? I personally think user tags are informative but very ugly when members' tags use the bot colors because it makes it harder to differentiate between priviliged members and bots at first glance.

Your plugins add a bunch of new patches which caused this plugin to get deleted

I might try to recreate it using decorations, then they will be a lot more useful

@henmalib
Copy link
Contributor Author

I would love to hear answers to the following questions by someone (Vee or Nuckyz):

  • Should data attributes be supported like in an original plugin?
  • Tags inside of a profile require 3 patches ( for both bots and users ), which are going to be dependent on one another. Should I support it? If one of the patches breaks, default tag will be displayed ( none for users, app for bots )
  • If adding a plugin idea from @OIRNOIR, it's probably to use DecorationsAPI, but will require a patch to remove default tags. What seems more reasonable: changing tags directly or removing them?

@TheKodeToad
Copy link
Contributor

TheKodeToad commented Feb 2, 2025

And bots are... ugh, should find a way to remove default one image

My solution was just not to try and merge the bot and custom tag. I always found separating them with • to be somewhat weird - and this works just fine IMO:
image

(also allows styling the tag separately 🎉)

Of course you can't add everything with decorators though, my plugin is admittedly unfinished and will eventually need a few patches

I thought it might be appreciated to do a direct port of my plugin since it also has icon support and you can add as many custom tags as you want

image

but looks like this plugin is in a better/more mergeable state since I last checked

@henmalib
Copy link
Contributor Author

henmalib commented Feb 2, 2025

My solution was just not to try and merge the bot and custom tag. I always found separating them with • to be somewhat weird - and this works just fine IMO:

That might be a nice solution, but wouldn't it look weird with [APP] [WEBHOOK]?

Of course you can't add everything with decorators though
Well, the only thing i haven't seen an API for was for user profiles, messages/member list are quite easy

I'll probably change to decorations again tomorrow, try to implement a pr from OIRNIOR and will use splitting like you do

@lolsuffocate
Copy link
Contributor

Well, the only thing i haven't seen an API for was for user profiles, messages/member list are quite easy

I believe Nuckyz just PR'd an API for this #3197

@henmalib
Copy link
Contributor Author

henmalib commented Feb 2, 2025

Well, the only thing i haven't seen an API for was for user profiles, messages/member list are quite easy

I believe Nuckyz just PR'd an API for this #3197

Well, untill it gets merged there's no point of relying on it
But thanks for pointing this out. I'm pretty sure It will get merged soon, so yeah
Decorations might be the way to go

@TheKodeToad
Copy link
Contributor

That might be a nice solution, but wouldn't it look weird with [APP] [WEBHOOK]?

True, I just didn't implement webhook tags. That could become a separate plugin again or just have its own handling... because surely just changing a message is unlikely to break that much

@Nuckyz
Copy link
Collaborator

Nuckyz commented Feb 2, 2025

I've changed the decorators API a bit, so now if you want to use them you no longer need to manually add margin or align vertically in messages. If you want those changes just merge dev into your branch

For the NicknameIconsAPI, I will work a bit more in improving PlatformIndicators and then I plan to merge that into dev as well, shouldnt take much

@henmalib henmalib marked this pull request as ready for review February 3, 2025 16:07
@henmalib henmalib marked this pull request as draft February 3, 2025 16:07
@henmalib
Copy link
Contributor Author

henmalib commented Feb 3, 2025

Yop, decorations without trying to override original ones are good
The only thing that seems to be weird for me is that [ADMIN] comes before [APP], but it messages it goes in another direction

image
image

The only thing left is profiles, which currently requires 3 patches
( 1st is to make discord use our tags, 2nd to prop guild/channel to user profile, 3rd is to actually display it )

And it seems that #3197 has only userId, so... there is a choice
3 patches to have tags in 1 place or just drop user profile support. I don't see other solutions now

@TheKodeToad
Copy link
Contributor

TheKodeToad commented Feb 3, 2025

2nd to prop guild/channel to user profile

Just use SelectedChannelStore - it shouldn't be possible to open a profile for another channel I don't believe
Edit: just remembered the new modal sometimes shows guild profile and sometimes global profile. I suppose that could cause problems.

@henmalib
Copy link
Contributor Author

henmalib commented Feb 3, 2025

Just use SelectedChannelStore - it shouldn't be possible to open a profile for another channel I don't believe

Fire, I didn't knew that this existed or totally forgot about it. Now i should just wait till new API drops and this plugin going to be gluten patch free

Edit: just remembered the new modal sometimes shows guild profile and sometimes global profile. I suppose that could cause problems.

Eehh, it's still a draft and we have time to see how it behaves, but SelectedStoreChannel should still have the same channel?

@henmalib
Copy link
Contributor Author

henmalib commented Feb 3, 2025

Also profiles don't have any tags by default, so [APP] tags are missing there
image

I might just render 2 tags at the same time to stay consistent with every other field, but it is worth it?

@Nuckyz
Copy link
Collaborator

Nuckyz commented Feb 4, 2025

Also profiles don't have any tags by default, so [APP] tags are missing there image

I might just render 2 tags at the same time to stay consistent with every other field, but it is worth it?

It does for me
image

@Nuckyz
Copy link
Collaborator

Nuckyz commented Feb 4, 2025

Also please merge dev into this branch! Both APIs you used had changes as I mentioned, and I'm pretty sure your tags are going to be off-center because of the classes you are adding, and the fact the APIs do the centering for you now

@Nuckyz
Copy link
Collaborator

Nuckyz commented Feb 4, 2025

I will also try to make it configurable so you can place your tags before Discord ones in messages

@henmalib henmalib changed the base branch from main to dev February 4, 2025 04:34
@henmalib
Copy link
Contributor Author

henmalib commented Feb 4, 2025

It does for me

I forgot that i'm currently replacing the old one

Also please merge dev into this branch! Both APIs you used had changes as I mentioned, and I'm pretty sure your tags are going to be off-center because of the classes you are adding, and the fact the APIs do the centering for you now
Messages were off-center, I've removed both classes and rem-thingy, they are still off-centered, classes that i've used were from discord itself and that's how they used it

By default now:
image
Manually adding bottom .1rem to the component:
image

Member list seems fine by default anyway ( i haven't changed any classes )
image

@henmalib
Copy link
Contributor Author

henmalib commented Feb 4, 2025

So discord usesRemSizies in messages, so should I
image

Manual padding by .1rem from the bottom is not perfect, but without it it looks really off-set

@Nuckyz
Copy link
Collaborator

Nuckyz commented Feb 4, 2025

They are different sizes, you should look how discord sets the size of the ones in messages and replicate that

@henmalib
Copy link
Contributor Author

henmalib commented Feb 4, 2025

They are different sizes, you should look how discord sets the size of the ones in messages and replicate that

That's exactly how I'm doing it now
useRemSizes sets the height to be .975rem
Without it: 16px

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants