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

Fix: [Emscripten] open links in browser #8655

Merged
merged 1 commit into from Feb 8, 2021

Conversation

embeddedt
Copy link
Contributor

Problem

The "Visit website" buttons were not functional on Emscripten since it doesn't support xdg-open.

Description

This PR adds Emscripten-specific logic that opens the website using standard JS APIs.

Limitations

None that I can really think of.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Nice! Completely forgot that we support opening links :D

src/os/unix/unix.cpp Outdated Show resolved Hide resolved
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

Regarding commit message: Feature is something that is on top of our changelog, telling the world as one of the first things: WE NOW HAVE THIS. Not sure this can be consider that big of a deal to announce it like that ;)

Next we have Add , to tell we added functionality that doesn't have to be shouted from the roof-tops .. that would fit, but ..

Next we have Fix to indicate it fixes something. Which feels more appropriate for this; as it was broken, and now it is fixed. But the difference between Add and Fix can be a bit thin in these cases .. you could say Add: [Emscripten]: add OSOpenBrowser() support, or Fix: [Emscripten] Link now open properly when clicked. Depends a bit if you look at it from the user perspective or from the developer perspective, I guess. Personally, I try to look from the user, but in the end, that is up to you :)

@embeddedt embeddedt changed the title Feature: [Emscripten] Add OSOpenBrowser() support Fix: [Emscripten] open links in browser Feb 8, 2021
@embeddedt
Copy link
Contributor Author

@TrueBrain Should be ready for (re)review now.

@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 17:31 Inactive
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Most excellent :) Some minor coding-style bla, but otherwise looks really good.

The comment is really well written, thank you for that :)

os/emscripten/pre.js Show resolved Hide resolved
os/emscripten/pre.js Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 17:50 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 17:51 Inactive
@embeddedt
Copy link
Contributor Author

@TrueBrain Sorry for the coding style mistakes; I haven't had a chance to read through the guide yet and configure my editor. I should probably do that. 😉

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

@TrueBrain Sorry for the coding style mistakes; I haven't had a chance to read through the guide yet and configure my editor. I should probably do that. 😉

No worries; I always feel a bit like a nag to complain about it, but I also know it is the fastest way to learn any coding style a project has: by being corrected :D So I hope you don't mind, one more I missed first time around, and in your latest fix something went a bit wonky :P

Tnx a lot for this :D

@@ -28,6 +28,10 @@
#include <SDL.h>
#endif

#ifdef __EMSCRIPTEN__
#include <emscripten.h>
Copy link
Member

Choose a reason for hiding this comment

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

I totally missed this, but otherwise my fellow devs will hurt me: please add a tab between # and include, as seen in the blob just below this.
Currently not all code is converted yet, but new code should fix this :)

if (leftButtonDown) {
document.addEventListener("mouseup", openWindow);
} else
openWindow();
Copy link
Member

@TrueBrain TrueBrain Feb 8, 2021

Choose a reason for hiding this comment

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

You forgot the else case :)

Also, please checkout out your spaces here, it seems to vary a lot :P Seems this file has 4 spaces for indentation, so it would be good to apply this to all new code here. But mostly, the if has a different indentation than the else :D

Edit: owh, it mixes tabs and spaces. Seems this js file was done with spaces .. which is my bad :P But my editor does that by default .. better to stick with that for now :)

Copy link
Contributor Author

@embeddedt embeddedt Feb 8, 2021

Choose a reason for hiding this comment

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

The weirdness is probably VSCode choking on the mixed indentation style between this and other files; I'll hand-edit it to be consistent.

@embeddedt
Copy link
Contributor Author

embeddedt commented Feb 8, 2021

Hopefully the last round this time. (Thank you for the corrections by the way.)

@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 18:01 Inactive
document.addEventListener("mousedown", e => {
if (e.button == 0) {
leftButtonDown = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

my OCD helps me: this one is intended wrong :P :P

@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 18:02 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 February 8, 2021 18:03 Inactive
@TrueBrain TrueBrain merged commit 6c8f222 into OpenTTD:master Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants