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

Added check to see if URLs start with "http://", add it if they don't. #952

Merged
merged 6 commits into from Mar 22, 2014

Conversation

Projects
None yet
2 participants
@zedutchgandalf

zedutchgandalf commented Mar 21, 2014

Also added support for dynamically redirecting (instead of always redirecting to the first site in the list).

Added check to see if URLs start with "http://", add it if they don't…
…. Added support for dynamically redirecting.
@@ -341,7 +347,8 @@ class FlxPreloader extends NMEPreloader
#if flash
private function goToMyURL(?e:MouseEvent):Void
{
Lib.getURL(new URLRequest(allowedURLs[0]));
var prefix:String = allowedURLs[myURLIndex] == FlxPreloader.LOCAL ? "" : ~/^https?:\/\//.match(allowedURLs[myURLIndex]) ? "" : "http://";

This comment has been minimized.

@Gama11

Gama11 Mar 22, 2014

Member

I'm not normally against ternaries, but this line is too complicated for that to still be readable (double ternary here even). Also, it would be nice to have a comment here explaining the regex.

@zedutchgandalf

This comment has been minimized.

zedutchgandalf commented Mar 22, 2014

@Gama11 is something like this better?

@Gama11

This comment has been minimized.

Member

Gama11 commented Mar 22, 2014

Yeah, I think that's a lot more readable.

Not sure about myURLIndex. Maybe something like siteLockURLIndex would be more descriptive? I can see why you went with myURL, because of goToMyURL(), but that function isn't public API (that's also why the comment " Used in goToMyURL()" is not too helpful).

@zedutchgandalf

This comment has been minimized.

zedutchgandalf commented Mar 22, 2014

Kind of off-topic, but what is this "Travis CI build"? And why is it failing?

@Gama11

This comment has been minimized.

Member

Gama11 commented Mar 22, 2014

It's a service for open-source projects for continous testing. In this case, it builds every demo and does a few basic unit tests for every commit, just to check everything works.

The reason why it's currently failing is unrelated to your pull request.

@zedutchgandalf

This comment has been minimized.

zedutchgandalf commented Mar 22, 2014

I added the same functionality to FlxG.openURL().

@Gama11

This comment has been minimized.

Member

Gama11 commented Mar 22, 2014

Not a bad idea, but in that case, the preloader should call openURL() instead of having the same code there twice, creating a redundancy. It should also be documented in the description for the function.

@zedutchgandalf

This comment has been minimized.

zedutchgandalf commented Mar 22, 2014

Is it a redundancy, though?
FlxPreloader.goToMyURL() can direct to FlxPreloader.LOCAL, while FlxG.openURL() is mainly used to open actual URL's.
So it would only be possible to use FlxG.openURL in the case where goToMyURL doesn't try to go to LOCAL, which would result in a kind of strange construction for goToMyURL, according to me...

@Gama11

This comment has been minimized.

Member

Gama11 commented Mar 22, 2014

I wasn't saying to remove goToMyURL. Except for the local part, the logic in the two functions is almost identical though - the http-logic is redundant.

@zedutchgandalf

This comment has been minimized.

zedutchgandalf commented Mar 22, 2014

The preloader now uses FlxG.openURL() for actual URL's

Gama11 added a commit that referenced this pull request Mar 22, 2014

Merge pull request #952 from zedutchgandalf/dev
Added check to see if URLs start with "http://", add it if they don't.

@Gama11 Gama11 merged commit 92c82a5 into HaxeFlixel:dev Mar 22, 2014

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment