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

Add option to display webpage titles by right clicking in chat #13724

Closed
wants to merge 10 commits into from

Conversation

chmodsayshello
Copy link
Contributor

@chmodsayshello chmodsayshello commented Aug 9, 2023

This simple PR adds a setting that allows you to display the title of a webpage (like it would be displayed in your browser) by simply right-clicking them in chat. It does so by getting the website's HTML using cURL, and then extracting the title tag.

Why is this PR needed
While playing in multiplayer, players may come across links where it's not obvious where they lead you to. With this feature, you can find that out just with a single click, and without the need of opening a browser.

To do

This PR is a Ready for Review¹.
1: Since it compiled and works just fine I would mark this as "Ready for review" for now. However, due to this being my first PR to the minetest engine itself, I do expect tons of changes I'll have to implement before it's in a case where merging it is actually considered.

How to test

  • download the branch and compile a client
  • enable the newly added setting
  • hop into the game
  • send some chat messages that contain links
  • open chat and right-click those links
  • send a weblink leading to a site without title (like https://cplusplus.com/reference/regex/)
  • verify that this case is handled

grafik

@wsor4035 wsor4035 added Feature ✨ PRs that add or enhance a feature @ Client / Controls / Input labels Aug 9, 2023
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives players a simple way to (potentially unknowingly) reveal their IP addresses. However the setting is false by default, so this may not be an issue.

I think this would be taken better care of by server-side mods: Simply use register_on_chat_message, search messages for URLs and asynchronously request the pages using the HTTP API, then have the server send the title to everyone like ShadowBot in the Minetest IRC channel. This avoids (1) every client having to request the website just to show a title preview (2) players leaking their IP; the server's IP is known already.
However the UX may be considered slightly worse; titles would be sent by default rather than on-demand. It could also be implemented on-demand, but that would have to use chatcommands or formspec interactions.

src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
@chmodsayshello
Copy link
Contributor Author

chmodsayshello commented Aug 10, 2023

This gives players a simple way to (potentially unknowingly) reveal their IP addresses. However the setting is false by default, so this may not be an issue.

I think this would be taken better care of by server-side mods: Simply use register_on_chat_message, search messages for URLs and asynchronously request the pages using the HTTP API, then have the server send the title to everyone like ShadowBot in the Minetest IRC channel. This avoids (1) every client having to request the website just to show a title preview (2) players leaking their IP; the server's IP is known already.
However the UX may be considered slightly worse; titles would be sent by default rather than on-demand. It could also be implemented on-demand, but that would have to use chatcommands or formspec interactions.

While I agree that you can easily hand out your ip with this feature, like you said, you have to enable it by hand (like you said), AND the alternative would the be pressing ctrl and left click to open the link in the default broswer, which isn't any better in my opinion...

Regarding the server side mod, to add to all the drawbacks you've already mentioned, as a user you would have to go to each sever you play on as a user and ask the owner to add a mod, and client side mods are usually disabled...

src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Aug 10, 2023
return "";
}

bool GUIChatConsole::weblinkClickOpen(s32 col, s32 row)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that this PR does not add this code, all it did to it was changing it's name (which should be fine as it's private), and separate a part of it, as the feature added requires that as well. That somehow confused the git diff

@rubenwardy
Copy link
Member

rubenwardy added the [Supported by core dev] label

(Note that I added this label because sfan5 started reviewing, so I assumed he supports this)

@sfan5
Copy link
Member

sfan5 commented Aug 13, 2023

To be clear I am neutral to this feature, but if it is merged the code should be corrected.
I suppose it doesn't hurt though.

@rubenwardy rubenwardy removed the Concept approved Approved by a core dev: PRs welcomed! label Aug 13, 2023
@rubenwardy rubenwardy added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. label Sep 17, 2023
@grorp
Copy link
Member

grorp commented Oct 19, 2023

From IRC:

rubenwardy: Yeah I don't support that pr

rubenwardy: The implementation uses regex to parse html, which is bad, and the feature feels scope creepy

grorp: I agree. Also, the feature doesn't seem useful to me: You can just open the URL in your browser if you want to know the title. That also makes it more explicit that you reveal your IP.

sfan5: yeah, giving it an initial review was a mistake

https://irc.minetest.net/minetest-dev/2023-10-19#i_6123362

@Zughy Zughy closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Controls / Input Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants