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

@embeddedt embeddedt commented Feb 8, 2021

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

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
@embeddedt embeddedt force-pushed the emscripten_open_browser branch from 96266f4 to 0a774e9 Compare Feb 8, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 8, 2021

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 force-pushed the emscripten_open_browser branch from 0a774e9 to c4b02a3 Compare Feb 8, 2021
@embeddedt embeddedt changed the title Feature: [Emscripten] Add OSOpenBrowser() support Fix: [Emscripten] open links in browser Feb 8, 2021
@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented Feb 8, 2021

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

@TrueBrain TrueBrain added the preview label Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 Feb 8, 2021 Inactive
Copy link
Member

@TrueBrain TrueBrain left a comment

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
@embeddedt embeddedt force-pushed the emscripten_open_browser branch from c4b02a3 to 529ad43 Compare Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 Feb 8, 2021 Inactive
@embeddedt embeddedt force-pushed the emscripten_open_browser branch from 529ad43 to 69e2fc4 Compare Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 Feb 8, 2021 Inactive
@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented Feb 8, 2021

@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

@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

@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.

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 embeddedt force-pushed the emscripten_open_browser branch from 69e2fc4 to 1a97edc Compare Feb 8, 2021
@embeddedt
Copy link
Contributor Author

@embeddedt 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 Feb 8, 2021 Inactive
document.addEventListener("mousedown", e => {
if (e.button == 0) {
leftButtonDown = true;
}
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.

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

@embeddedt embeddedt force-pushed the emscripten_open_browser branch from 875e371 to a4b11c6 Compare Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8655 Feb 8, 2021 Inactive
@TrueBrain TrueBrain merged commit 6c8f222 into OpenTTD:master Feb 8, 2021
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants