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

[WIP] Add URL button #8139

Closed
wants to merge 3 commits into from
Closed

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented Jan 27, 2019

Adds a formspec element to open URLs in a browser.

Note that this is only enabled in the mainmenu (currently)

This cannot be used whilst in-game, due to the obvious issues. I'd like to add support for this eventually, with a confirmation dialog showing the requested URL. But this would should not be done in this PR

Usecase

The use case in mind is allowing a button to open up the ContentDB pages of packages.

To implement reviews and comments, I don't want to have to add login support to the client. Instead, I'd prefer to have a button to open up ContentDB in a web browser.

To do

  • Ensure security
    • Is m_client always non-null in-game?
    • Is the regex safe? I haven't checked whether it disallows spaces and such
  • Make it obvious that the button opens a link - Internet icon?
  • OS support - eg: Android, mac-OS

@rubenwardy rubenwardy changed the title [WIPAdd URL button [WIP] Add URL button Jan 27, 2019
@rubenwardy rubenwardy added WIP The PR is still being worked on by its author and not ready yet. Feature ✨ PRs that add or enhance a feature labels Jan 27, 2019
src/porting.cpp Outdated
#ifdef _WIN32
return system((std::string("open ") + url).c_str()) == 0;
#else
return system((std::string("xdg-open ") + url).c_str()) == 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really not happy with this method of opening URLs, but I couldn't find a better way

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try this on android? IIRC you will need to use JNI/java for this but I haven't tested it. Maybe xdg-open works there, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, this doesn't support Android yet. I need to find out how to call Java functions from this part of the code, so I can then use intents to open the browser

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be easy enough, you will just need too add another JNI call in porting_android.cpp

src/porting.cpp Outdated Show resolved Hide resolved
@rubenwardy
Copy link
Member Author

rubenwardy commented Jan 27, 2019

Alternatively:

core.open_url(url)

player:open_url(url)

This would make it easier to make sure the it's never possible for button_url to be used in-game before such a feature is explicitly supported

The abuse of such a method in-game worries me, but worth discussing

@Fixer-007
Copy link
Contributor

And maybe ability to open a changelog page for new minetest update...

@@ -3629,6 +3685,14 @@ bool GUIFormSpecMenu::OnEvent(const SEvent& event)
return true;
}

if (!s.url.empty()) {
if (m_client) {
errorstream << "Unable to use button_url in game!" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

it sounds a little bit hacky to have those m_client tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to detect if we're in the menu or not? Probably to use the core.open_url approach instead of a button

#ifdef _WIN32
return system((std::string("open '") + url + "'").c_str()) == 0;
#else
return system((std::string("xdg-open '") + url + "'").c_str()) == 0;
Copy link
Member

Choose a reason for hiding this comment

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

doesn't work on android

Copy link
Member Author

Choose a reason for hiding this comment

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

as has been said

@@ -697,6 +699,21 @@ int mt_snprintf(char *buf, const size_t buf_size, const char *fmt, ...)
return c;
}

bool openURL(std::string url)
Copy link
Member

Choose a reason for hiding this comment

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

const std::string &

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature WIP The PR is still being worked on by its author and not ready yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants