Legacy support & Minimessage configuration options #80
Conversation
There was a problem hiding this comment.
https://docs.advntr.dev/minimessage/api.html#builder
The docs talked about a builder where we can specify exactly which tags we want it to be able to resolve. It sounds like we might be able to use that for the minimessage configuration options part. Could be good to use that if minimessage keeps expanding its api so its easy for me to keep GPF with all the latest features, but I also couldn't really figure out if it could actually be used in the way we want here. Feel free to look into it if you want but its also fine to ignore I think.
| } | ||
| matcher = pattern.matcher(message); | ||
|
|
||
| if (!requiredChanges) { |
There was a problem hiding this comment.
I can't tell what this variable is trying to do in this one. If I don't have permission for a click action I tried to use, it removes my click component, but leaves all the other click components in the message?
There was a problem hiding this comment.
The problem was, when you find something in the pattern, but have the permission to use it, there is no need for change. But if there is no need for change, the while loop will keep iterating over and over until the server runs out of memory.
There was a problem hiding this comment.
But we could make the permission check before the actually pattern initialization. However it could become annoying with the click and hover components. What do you think about this?
There was a problem hiding this comment.
Yeah, I think it makes sense for perm check before pattern initialization. What makes it annoying with click and hover components? That there are multiple types of them? Thats fine, just split it up more
if (!sender.hasPermission("gpflags.messages.hover.show_text")) {
// replace all "<hover:show_text>" with "\<hover:show_text>"
}
if (!sender.hasPermission("gpflags.messages.hover.show_item")) {
// replace all "<hover:show_item>" with "\<hover:show_item>"
}
...As you see in my example above, I also chose to escape out the tag rather than repalcing it with "".
I think if you choose to replace it with "", it could result in some cases where for example <re<blue>d> would turn into <red> which we want to avoid.
There was a problem hiding this comment.
Dont get this, Im sorry.
Do you want to put a \ before the hover:.... stuff? I think this would still setup the component itself. Also, we are having a while loop that repeats until there is no component left. So i think thats not bad at all atm.
All i could imagine to reduce iteration time:
if(!sender.hasPermission("gpflags.messages.hover.show_text") || !sender.hasPermission("gpflags.messages.hover.show_item") || !sender.hasPermission("gpflags.messages.hover.show_entity")) {
pattern = Pattern.compile("<hover:.*?>");
matcher = pattern.matcher(message);
while (matcher.find()) {
boolean requiredChanges = false;
String hoverComponent = message.substring(matcher.start(), matcher.end());
if (hoverComponent.contains("show_text") && !sender.hasPermission("gpflags.messages.hover.show_text")) {
message = message.replace(hoverComponent, "");
requiredChanges = true;
}
if (hoverComponent.contains("show_item") && !sender.hasPermission("gpflags.messages.hover.show_item")) {
message = message.replace(hoverComponent, "");
requiredChanges = true;
}
if (hoverComponent.contains("show_entity") && !sender.hasPermission("gpflags.messages.hover.show_entity")) {
message = message.replace(hoverComponent, "");
requiredChanges = true;
}
matcher = pattern.matcher(message);
if (!requiredChanges) {
break;
}
}
}There was a problem hiding this comment.
Do you want to put a \ before the hover:.... stuff? I think this would still setup the component itself.
Yes that's exactly what Im saying. You can use the minimessage viewer at https://webui.advntr.dev/ to see that this does make the tag show without it serving as a tag.
There was a problem hiding this comment.
Also, don't worry much about reducing iteration time here. This won't get called so often to where it's too big a consideration so I'd much rather it be easy to read and maintain. I still think three individual loops for the hover would be the best, and similar for the other parts
Im sure this is powerful and its worth looking into it someday. However, Im not capable atm to hook into it so i propose to keep it with the method in ChatUtil yet. It might be something for a v2 feature and should not take too much time to refactor it once on it. From my side, the requested changes shall be done now. Feel free to request new request if you are missing anything else related to the existing code. |
| } | ||
| matcher = pattern.matcher(message); | ||
|
|
||
| if (!requiredChanges) { |
There was a problem hiding this comment.
Yeah, I think it makes sense for perm check before pattern initialization. What makes it annoying with click and hover components? That there are multiple types of them? Thats fine, just split it up more
if (!sender.hasPermission("gpflags.messages.hover.show_text")) {
// replace all "<hover:show_text>" with "\<hover:show_text>"
}
if (!sender.hasPermission("gpflags.messages.hover.show_item")) {
// replace all "<hover:show_item>" with "\<hover:show_item>"
}
...As you see in my example above, I also chose to escape out the tag rather than repalcing it with "".
I think if you choose to replace it with "", it could result in some cases where for example <re<blue>d> would turn into <red> which we want to avoid.
| } | ||
|
|
||
| // Check for bold, italic, strikethrough, underline, obfuscated | ||
| pattern = Pattern.compile("<(bold|italic|strikethrough|underlined|obfuscated)>"); |
There was a problem hiding this comment.
minimessage tags are case insensitive so this wont catch stuff that are like <BOLD> or <bOLD>
| } | ||
|
|
||
| // Check for hex-colors | ||
| message = message.replace("<#", "<color:#"); |
There was a problem hiding this comment.
If someone's using <# not as a tag, we wouldn't really want to convert this. Cant really imagine someone would do that but it's short enough a string to where I could see it coming up.
| while (matcher.find()) { | ||
| String link = pseudoMessage.substring(matcher.start(), matcher.end()); | ||
| pseudoMessage = pseudoMessage.replace(link, ""); | ||
| if(message.contains("<click:open_url:" + link + ">")) continue; // Skip if already a click-component |
There was a problem hiding this comment.
What if there are two of the same links in the message? This'll only convert the first one
There was a problem hiding this comment.
Not anymore, same iteration as everywhere until there is no link available.
There was a problem hiding this comment.
I added some comments. The more I look and think about this, the more I think using the Minimessage Builder API is going to be the best way around this. There's just so many edge cases that keep coming up and it feels like as soon as I'd release this, we'd find so many more being reported.
In that case, lets make a compromise: Since our server requires this change now, I will use this version I created locally and make tests if it works out. If I get some more time for hooking into the Builder, I will provide you a solution with it instead. Until then, I put this PR on draft. Cant tell a specific time where I look into it now. |
Related to #78:
Click-Components
gpflags.messages.click.suggest_command- Allows the suggest_command value of click-componentgpflags.messages.click.run_command- Allows the run_command value of click-componentgpflags.messages.click.run_command- Allows the run_command value of click-componentHover-Components
gpflags.messages.hover.show_text- Allows the show_textvalue of hover-componentgpflags.messages.hover.show_item- Allows the show_itemvalue of hover-componentgpflags.messages.hover.show_entity- Allows the show_entityvalue of hover-componentOthers
gpflags.messages.hex-colors- Allows the use of hex-colors in minimessagesgpflags.messages.colors- Allows the use of default colors (former legacy codes) in minimessagesgpflags.messages.style.bold- Allows usage of bold messagesgpflags.messages.style.underlined- Allows usage of underlinedmessagesgpflags.messages.style.italic- Allows usage of italicmessagesgpflags.messages.style.strikethrough- Allows usage of strikethrough messagesgpflags.messages.style.obfuscated- Allows usage of obfuscated messagesgpflags.messages.links: Allow using legacy links without<click:open_url:...>