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 a uri property to LinkButton #30675

Merged
merged 1 commit into from
Dec 17, 2022

Conversation

zaksnet
Copy link
Contributor

@zaksnet zaksnet commented Jul 18, 2019

This is useful when you want to add link buttons through code (simplifies things) and anyway without this linkButton is just a button with an underline. I tried connecting the pressed signal instead of launching the URL from the draw event but i couldn't figure it out, any guidance is appreciated. Also documented linkButton.xml

If this is going to get merged it would be nice to add:

  • StyleBoxes for the rest of the states (currently only for focused exists).
  • Text alignment.

I wouldn't bother otherwise since you can do all this with a button already, but since this exists, we could improve it.

EDIT: Changed keyword from Link to Url since its more proper in this case (link most of the times implies a website URL, while URL can be anything from websites, file paths to system paths or even UNC network paths). Also added launch_url to manually launch the Url. @akien-mga please review.

@zaksnet zaksnet requested a review from a team as a code owner July 18, 2019 11:37
@zaksnet zaksnet force-pushed the link-button-link-prop branch from 082053b to ec8fe74 Compare July 18, 2019 11:42
@akien-mga akien-mga added this to the 3.2 milestone Jul 19, 2019
@zaksnet zaksnet force-pushed the link-button-link-prop branch 3 times, most recently from 5dd6280 to 5d3ce87 Compare July 19, 2019 09:54
@zaksnet zaksnet force-pushed the link-button-link-prop branch 2 times, most recently from b158b59 to 7d5d358 Compare July 19, 2019 14:45
@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@aaronfranke
Copy link
Member

@zaksnet Is this still desired? If so, it needs to be rebased on the latest master branch.

@zaksnet zaksnet force-pushed the link-button-link-prop branch from 7d5d358 to bc3f5ab Compare March 4, 2021 16:12
@zaksnet zaksnet requested a review from a team as a code owner March 4, 2021 16:12
@zaksnet zaksnet force-pushed the link-button-link-prop branch from 4e23add to bc3f5ab Compare March 4, 2021 16:18
Calinou
Calinou previously requested changes Mar 4, 2021
@zaksnet zaksnet force-pushed the link-button-link-prop branch 6 times, most recently from 7ea3eca to 06d4016 Compare March 4, 2021 18:05
@zaksnet
Copy link
Contributor Author

zaksnet commented Mar 4, 2021

Alright this finaly seems to be working:
Link Button Demo
@Calinou !

Just as a reminder, all this PR does is adding the Url parameter for the LinkButton. This is convenient because all the user has to do is enter the url in the editor. Otherwise to get the same result you'd have to create a script and export a variable to the editor.

@zaksnet
Copy link
Contributor Author

zaksnet commented Mar 4, 2021

The errors in the editor after closing the project seem to be related to OS::get_singleton()->shell_open(url);.

ERROR: Condition "!process_map->has(p_pid)" is true. Returning: FAILED
   at: OS_Windows::kill (platform\windows\os_windows.cpp:489)

EDIT: NVM, i get them with any project when i open/close it a couple of times.

@zaksnet zaksnet force-pushed the link-button-link-prop branch from 06d4016 to 0292d47 Compare March 4, 2021 19:10
@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2021

This is useful when you want to add link buttons through code (simplifies things)

You mean like

var link = LinkButton.new()
link.url = "https://godotengine.org/"

instead of

var link = LinkButton.new()
link.connect("pressed", OS, "shell_open", ["https://godotengine.org/"])

?

btw by connecting pressed to open_url() you make it impossible to use the button for a different purpose.

@zaksnet zaksnet force-pushed the link-button-link-prop branch 7 times, most recently from 7442879 to 6033cb0 Compare July 28, 2022 18:17
@zaksnet
Copy link
Contributor Author

zaksnet commented Jul 29, 2022

@aaronfranke @Calinou This is ready for review

Copy link
Contributor

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

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

Just a documentation review as I crossed this PR.

@akien-mga
Copy link
Member

I'd suggest overriding the virtual pressed() method instead of using the signal, as done in #59810.

I'm also not convinced about the usefulness of the open_url() method. A button's function is to be pressed and that's when the URL should be opened. If users want to open the URL without pressing the button they don't really need the button in the first place, but they could just do OS.shell_open($LinkButton.url) if need be.

@zaksnet zaksnet force-pushed the link-button-link-prop branch from 6033cb0 to b1fe266 Compare August 7, 2022 08:47
@zaksnet zaksnet force-pushed the link-button-link-prop branch from b1fe266 to cfcc47d Compare August 24, 2022 09:11
@akien-mga akien-mga changed the title Add a url property to LinkButton Add a uri property to LinkButton Dec 17, 2022
@akien-mga akien-mga force-pushed the link-button-link-prop branch from cfcc47d to 3faaf98 Compare December 17, 2022 14:18
@akien-mga
Copy link
Member

I force pushed an update to solve merge conflicts, and do some further improvements to the docs, and clean up of unused methods added to the header.

I also changed the url property to uri, as I think it's the more correct term for this kind of identifier which may or may not have a protocol specified. E.g. AFAIK godotengine.org would be a URI while https://godotengine.org is a URL, and both should work here. Maybe @Faless can confirm.

I don't mind changing back to url if we feel like this is still the better name.

@akien-mga akien-mga requested review from Calinou, aaronfranke and HaSa1002 and removed request for HaSa1002 December 17, 2022 14:21
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga akien-mga force-pushed the link-button-link-prop branch from 3faaf98 to d73a9b5 Compare December 17, 2022 14:27
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested and it seems to work fine.

One thing I thought might be expected for some users would be to set the URI as tooltip for the LinkButton, but if this is implemented it might have to be opt-out (e.g. a uri_as_tooltip bool property). But let's wait first to see if some users actually request it.

@akien-mga akien-mga dismissed stale reviews from aaronfranke and Calinou December 17, 2022 19:57

Changes done.

@akien-mga akien-mga merged commit 10bc1d8 into godotengine:master Dec 17, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu
Copy link
Member

timothyqiu commented Dec 18, 2022

Cherry-picked for 3.6.

@timothyqiu timothyqiu removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 18, 2022
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.

10 participants