Adding ofxAndroidLaunchBrowser(String url) to ofxAndroidUtils

[thiagohersan]

This wasn't discussed as an issue... Sorry. I needed it, so it got implemented, and now I'm submitting it.

Similar to ofLaunchBrowser(string url) in ofUtils and ofxiPhoneLaunchBrowser(string url) in ofxiPhoneExtras.

Tested on an Android 2.3.4 device with SDK r21, NDK r8c and API 17.

I have a very simple example that I used for testing, that I can also submit with this PR.

[bilderbuchi]

just as a btw, shouldn't ofLaunchBrowser call/wrap ofxiPhoneLaunchBrowser and ofxAndroidLaunchBrowser, depending on platform? it already has different implementations for the different desktop platforms...

[thiagohersan]

that's also what I originally thought, just add another #ifdef to ofLaunchBrowser(...) in ofUtils.

but, I think Android and iPhone code is being kept separately in their own addons for now.

I wonder if that is always going to be the case, or if the plan is to someday pull their code into the core....

[bilderbuchi]

sure it is smart to keep the code where it is, but in the #ifdef you could just call those functions...

[thiagohersan]

true. true.

I can do that in a different branch and PR ...

[arturoc]

there's other functions like this that are called from the core, si it's ok to add it. also you can add it in this PR if it's easier for you

[thiagohersan]

yeah, I remembered that ofSoundStream and ofSoundPlayer already have Android/iOS stuff #defined in there...

I'll add to this PR then. It will be easier to just check for "http://" in the url string inside ofLaunchBrowser.

[thiagohersan]

ok. I added Android/iOS support to ofLaunchBrowser() in ofUtils.cpp.

I made some modifications to the code that checks for "http" or "https" in the string url of ofLaunchBrowser() because Android wants those in lower-case. Let me know if I should revert back to Poco for string comparisons.

Tested on Android 2.3.4 and macosx 10.8 (to make sure I didn't break the desktop version).

[bilderbuchi]

thank you! 2 things:

• I know it's there, and +1 for following established style, but I think there's no need for those framing //------ around the ifdefs. maybe best to remove them in the whole function, instead of introducing new ones.
• regarding the http-check: are you saying that the poco compare fails on android, as it was before? Because if Android (alone) needs some specific implementation, it would be better to keep that implementation in ofxAndroidLaunchBroswer()

also, did you check on iOS, too? Otherwise we should get someone else (maybe @julapy?) to confirm that it works on the iphone.

[thiagohersan]

• I'll remove the //----- .
• the way it is now ofLaunchBrowser() does a case-insensitive compare using Poco::icompare(). For Android the check has to be case-sensitive (the native android code barfs on "Http" or "HTTP"), so I used ofToLower() on the url before comparing using the standard string compare function. I think Android is the only one that needs to have "http" in lower case, but it's not a special case in the code since it doesn't hurt the others if they get a url that's all lower case. If that's too specific I can tuck this in somewhere else and revert to using Poco::icompare().
• I don't have an iOS device to test this, so... it'd be great if someone else could test it.

[thiagohersan]

I'll revert the check in ofUtils.cpp to Poco::icompare(), then somewhere in the android code I'll add a special check for lowering the case of only http/https (probably in the java side).

[arturoc]

ok, i think it would be ok to do it on the c++ side though, even if most browsers support HTTP the correct is http

[bakercp]

I added the Poco::icompare() lines to that code a few months back to allow https and I think that @thiagohersan's ofToLower() approach + string::compare makes a lot of sense for now. No need to replicate it, when it really should always be http (browsers are essentially doing the same ofToLower operation). In the future, ofToLower and other oF string utility functions will be transitioning to UTF8 safe versions, but we can make those changes to this code when the time comes.

One small addition -- I'd suggest a comment next to ofToLower that simply mentions // Some platforms, such as Android require lower case urls to make sure we don't forget.

[bilderbuchi]

@arturoc cautioned in #1680 that

the problem with using toLower is that some urls might have capital letters, not the domain but the directories can be case sensitive so we should preserve the casing. one possibility is to lower only the first characters in an auxiliary variable and do the comparison on that, then change http/https to lower if it's capital

[bakercp]

Good point @arturoc and @bilderbuchi -- ofToLower should only be applied to the prefix after it has been confirmed to be http:// or https://

[thiagohersan]

what do you guys think of this?

tested on Android 2.3.4 and macosx 10.8.

[bakercp]

Looks like a good solution to me.

[bilderbuchi]

the same.

[arturoc]

thanks thiago

[roymacdonald]

@thiagohersan @arturoc this addition causes the compiler to throw a ton of errors when trying to compile for IOS.
Esentially al the errors refer to this additon, like this one:

In file included from /Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS5.1.sdk/System/Library/Frameworks/Foundation.framework/Headers/Foundation.h:8:
In file included from /Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS5.1.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIAccelerometer.h:8:
In file included from /Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS5.1.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIKit.h:9:
In file included from /Volumes/TODO/Users/roy/openFrameworksDev/libs/openFrameworksCompiled/project/ios/../../../../addons/ofxiPhone/src/utils/ofxiPhoneExtras.h:39:
In file included from /Volumes/TODO/Users/roy/openFrameworksDev/libs/openFrameworksCompiled/project/ios/../../../openFrameworks/utils/ofUtils.cpp:43:
/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS5.1.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSObjCRuntime.h:311:1: error: expected unqualified-id [1]
@Class NSString, Protocol;

I'll try to fix this so this function. But for the mean time I thik that this should be reverted. At least for iOS.

PS: Here in the github comments system, How can I wrap a block of text so it's rendered like code?

[arturoc]

just put 4 spaces before each of the code lines

going to take a look

[bilderbuchi]

PS: Here in the github comments system, How can I wrap a block of text so it's rendered like code?

or put 3 "`" above and below the code block. or press the link in the comment box which says "GitHub Flavored Markdown" which is the helpfile for the syntax.