Copies only the URL if there is a URL

[magnus-ISU]

Pretty self explanatory.

Click again to copy the entire message - this is told to the player when they click it.

[tierrif]

Hi, thanks for your contribution. To be fairly honest with you, I don't know if this is the best way to implement this feature. It'd be better if the link itself upon click were copied, and the rest of the message still copy-able if clicked elsewhere. I was pretty convinced chat-utils didn't affect this behavior (as Minecraft has this feature implemented), but if I'm wrong please let me know. Again, thanks.

[magnus-ISU]

I'm not sure, but the server I play on it does not work, to the great annoyance of several of the players for years. The reason is because of some weird configuration or chat plugin on the server side probably; but the whole point of a client mod is to get around stupid servers doing stupid things.

This might be slightly less usable in normal cases (though seriously, how often do you use the copy-a-general-chat-message feature? I copy links from chat daily. Also, how hard is it to double click a message?), but it is incredibly useful for me and several people I know.

[tierrif]

As someone who moderates a server, it's a very useful feature to copy chat messages, and such thing is done all the time. When it comes to URLs, the way you implemented it might be problematic if messages go around your link verification process and type very long messages with portions that simply start with "https://" (as an example). Given that, if you really want to add this feature, I suggest instead finding links in the chat receive Mixin, adding a separate click event and the way you'd find the link would be with a regex. If this is too confusing however, I don't mind implementing this myself, but it'll sure not be the highest of priorities, as I'd only implement it after the chat memory suggestion you made in the issue. I hope you understand.

[magnus-ISU]

Okay, reading through the entire chat receive mixin, I understand better how the mod works :P. However, I still don't understand how minecraft itself would allow you to do that, as there is a single text variable, and it has a single onClick action. I don't even understand how the mod lets you copy text with a URL, if it already has one.

I'm fine with maintaining a fork for a bit (ie. doing nothing and then running git merge when you add the chat memory feature :P) if you don't think this is useful.

If you do decide to implement the feature, you should consider how to handle multiple links in one chat message. My thinking for this was that

1. If you post more than 1 link in a minecraft chat message you're an idiot
2. You can still just copy the whole message and delete to the correct portions if you need to get the second link So I didn't worry about it. If you're implementing positional clicking, then you can actually handle this better.

I would probably use a regex like ([^ ]*://.*)|(www\.[^ ]*)|([^ ]*\.com[^ ]*)

[tierrif]

From what I experienced, Minecraft's URL click events bypass other click events implemented on Text styles. Unintentional, yet existing feature. Though if this no longer works (perhaps it slipped through a Minecraft update), I don't have any issues implementing this manually. But as said above, it would only be done later. As to your fork, feel free to do whatever you want with it. I don't mind at all what you do with the code, as long as you don't take over the mod itself, anything else is perfectly fine.

I was going to mention this, good thing you talked about positioned clicking. This whole click event mess is really stupid, I won't even call it anything else. I did not intend to make the mod this way, and I've been planning for a good while to handle click events manually through positioned clicking on the ChatHud itself - at a lower level in the client. However, this is obviously more effort, so to make the mod functional I found this as a "temporary" solution, which obviously has some limitations such as the one you found here. This mod was clearly not intended to the public but since I'm seeing a tiny bit more of demand than I expected, I might give it a little more work as time goes on. And if it is for the public, then I will have to consider all scenarios. This would include multiple links in a message, a scenario that sounds rare, yet possible, and in my honest opinion nothing should ever be excluded. The user always has a specific use we don't know about. When it comes to the regex, that's more like it.

[retrixe]

I think we should eventually switch out the mixin for something like #6 was trying to do, injecting in the method where chat packets are parsed instead of injecting where chat messages are added to the HUD, since entire messages are currently handled line-by-line(?) and not as a whole, which would be an issue for long URLs.

We can use Adventure to parse the message, look for URLs, check if there is a click event attached, if not then add one (since some servers will actually do this correctly and we don't want to interfere with that). Adventure makes this fairly trivial to do using TextReplacementConfig.

Example code from a server plugin to see how TextReplacementConfig can be used to insert URL click events:

Pattern linkPattern = Pattern.compile(Config.LINK_REGEX);
return LegacyComponentSerializer.legacySection().deserialize(translate)
        // Add click events to URLs.
        .replaceText(TextReplacementConfig.builder().match(linkPattern).replacement(builder1 -> {
            String urlString = !builder1.content().trim().matches("(?i)^https?://.*")
                    ? "http://" + builder1.content().trim() : builder1.content().trim();
            return builder1.clickEvent(
                    net.kyori.adventure.text.event.ClickEvent.clickEvent(
                            net.kyori.adventure.text.event.ClickEvent.Action.OPEN_URL,
                            urlString
                    )
            );
        }).build());

I think attaching a URL click event is the ideal way to handle this, since the proposed solution in this PR is counter-intuitive when URL click events exist. I'm not very interested in writing this myself, but I'm willing to assist if you have any issues implementing this.

[tierrif]

Yes, the current implementation by injecting into the HUD handles lines individually, according to how the client splits them based on max length, and not on whether they're part of the same message or not, which is an issue at this higher level. As you said, injecting into the network handler is better, right when the message packet is received, which does handle messages just as they were received from the server. However, back when I tried this, the method would run twice, but after I finished some changes in a couple of recent updates, I saw that this was a threading issue, and injecting at RETURN instead of HEAD fixed this issue in methods that force a main thread to run a specific runnable for the received packet.

However, there are more issues when doing this with the anti-spam component. The anti-spam system deletes one, and only one line when a message is duplicated, which is sort of the only option I found (for now) to remove messages from the ChatHUD. As these are not distinguished by the HUD at all, as they got split beforehand (and hence was the issue when injecting there in the first place), doing this was tricky with long messages. The fix for this is possibly canceling the event in the network handler itself, if possible.

[retrixe]

URL click events are attached by the server and not by the client, as OP said, their server does not detect URLs in messages and attach URL click events appropriately.

@magnus-ISU if you are willing to submit a new PR following the approach I detailed here (https://github.com/tierrif/chat-utils/pull/10#issuecomment-1199930053) using Adventure and TextReplacementConfig (or something similar) to attach URL click events to URLs if one is not present already, then it would be welcome for merge. No need to wait on #6 or similar solution to land imho because all the other features of this mod are already broken because of it, and it makes no sense to block this feature waiting for that PR to land, especially with the new event system which hides these details from listeners.

[tierrif]

Closed due to inactivity