Skip to content

[3.x] Add explicit URL property in LinkButton node#63855

Closed
AndreaTerenz wants to merge 3 commits intogodotengine:3.xfrom
AndreaTerenz:url-link-button
Closed

[3.x] Add explicit URL property in LinkButton node#63855
AndreaTerenz wants to merge 3 commits intogodotengine:3.xfrom
AndreaTerenz:url-link-button

Conversation

@AndreaTerenz
Copy link

@AndreaTerenz AndreaTerenz commented Aug 3, 2022

This PR adds a url property to LinkButton that points to a resource (most likely a webpage) to be opened when the button is clicked. Such value will be used as the visible text of the button when the text property isn't set.

The idea is to have something simpler than a RichTextLabel, which requires figuring out the BBCode syntax for a link and manually writing a script to actually open the URL when clicking. At the same time, LinkButtons right now (in Godot 3.x at least) are simply flat buttons with some underlined text which isn't actually treated as a link to anything (the dev is supposed to again manually write a script that opens the url when the button is pressed).

This PR aims at clarifying this system, where RichTextLabels may be used for complex text with multiple links (which could need to be treated separately in code) and where LinkButtons are a simple and quick way of displaying a single URL to (e.g.) one's GitHub profile.

@AndreaTerenz AndreaTerenz requested review from a team as code owners August 3, 2022 00:14
@AndreaTerenz AndreaTerenz changed the title Add explicit URL property in link button Add explicit URL property in LinkButton node Aug 3, 2022
@Calinou
Copy link
Member

Calinou commented Aug 3, 2022

Note that there's already a pull request implementing this feature in master: #30675

@Calinou Calinou added this to the 3.x milestone Aug 3, 2022
@AndreaTerenz
Copy link
Author

@Calinou I admit I didn't see it, but it seems to me that that PR opens the link by having the button listen to its own pressed signal and the connection is made with the G4.0 API (aka with callables). This would clearly not work in G3.x.
My solution, on the other hand, doesn't use signals at all and instead directly overrides the BaseButton::pressed() method - which already "listens" to the pressed signal and is compatible with G4.0. I think this solution could be copy-pasted into G4, but I didn't wanna create duplicate PRs for the same code.

@YuriSizov
Copy link
Contributor

I didn't wanna create duplicate PRs for the same code.

As a rule, we first accept features to the master branch, and only then can they be backported/cherrypicked to the maintenance branches. We now have 3 PRs that add this feature, 2 for master (#30675, #59810), and this one for 3.x. So it's better that we focus the discussion on one of them, stabilize the implementation, and then decide on backporting. Feel free to cross-review #30675, your approach does indeed seem to be better, regarding the method override.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
xl_text = tr((text.is_empty()) ? url : text);
xl_text = text.is_empty() ? url : tr(text);

Probably should be like this, there's no reason to translate URL.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point, will do. I also noticed that the is_empty method doesn't exist for Strings in G3 (but I saw it in the G4 related issue?), so I really need to make a few corrections anyway

Copy link
Member

Choose a reason for hiding this comment

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

In 3.x it's called empty().

Copy link
Author

Choose a reason for hiding this comment

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

@KoBeWi Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@bruvzg Actually, tr(...) doesn't return a String but a StringName, so the ternary operator doesn't work:

if (text.empty()) {
	xl_text = url;
}
else {
	xl_text = tr(text);
}

@akien-mga akien-mga changed the title Add explicit URL property in LinkButton node [3.x] Add explicit URL property in LinkButton node Apr 25, 2024
@AndreaTerenz AndreaTerenz closed this by deleting the head repository Jul 4, 2025
@KoBeWi KoBeWi added the archived label Jul 4, 2025
@KoBeWi KoBeWi removed this from the 3.x milestone Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants