OK: one last big push for this. I have a branch on my fork of cordova-android with the necessary native changes [1].
I have updated the manual tests for location in mobile-spec [2] so that it is easy to explicitly test the cordova plugin version vs. the version attached to navigator.geolocation (presumably the "native" version). I might also add a few more Qunit tests but there isn't much to add there, as it's difficult to assert things about the state of GPS visibility on a device, etc. SO: the iOS native bits seem to be doing OK following Shaz's/Becky's review. Android peeps: can you take a look? I did testing last night on my ICS phone and it worked well. I would like to see this get in today but I understand if we don't want to get it into master before we tag 1.6.0 :) [1] https://github.com/filmaj/incubator-cordova-android/tree/geoupdate [2] https://github.com/filmaj/incubator-cordova-mobile-spec/tree/geoupdate On 3/29/12 12:56 PM, "Filip Maj" <[email protected]> wrote: >Updated my branch with that change now. > >On 3/29/12 11:31 AM, "Filip Maj" <[email protected]> wrote: > >>Good catch Becky, thanks. I'll patch that shortly. >> >>On 3/29/12 11:02 AM, "Becky Gibson" <[email protected]> wrote: >> >>>I think there is an issue in the LocationManager: didUpdateToLocation: >>>fromLocation method. The NSFastEnumeration protocol for NSDictionary >>>iterates over the keys and based how watchCallbacks is created I think >>>you >>>want the object. >>> >>>Thus, I think this: >>> >>>for (NSString *callbackId in self.locationData.watchCallbacks) { >>> [self returnLocationInfo:callbackId]; >>> } >>> >>>should be: >>> >>>for (NSString *timerId in self.locationData.watchCallbacks) { >>> [self returnLocationInfo:[self.locationData.watchCallbacks >>>objectForKey: timerId ]]; >>> } >>> >>>-becky >>> >>>On Wed, Mar 28, 2012 at 9:37 PM, Shazron <[email protected]> wrote: >>> >>>> Never mind - saw that NetworkStatus commit was already in the mainline >>>>as >>>> well >>>> >>>> 2012/3/28 Shazron <[email protected]>: >>>> > Oh - when you merge it in, don't squash your commits in this case >>>> > unless you list all the changes in the squash. Good to know about >>>>the >>>> > "Network Status" change since I needed to know about this in the >>>> > updated guides I'm writing. >>>> > >>>> > 2012/3/28 Filip Maj <[email protected]>: >>>> >> Thanks for looking it over Shaz, much appreciated - the link helps >>>>too. >>>> I >>>> >> am slowly getting over my aversion of invoking functions using >>>>square >>>> >> brackets and accepting objective-c for what it is. >>>> >> >>>> >> On 3/28/12 4:26 PM, "Shazron" <[email protected]> wrote: >>>> >> >>>> >>>Looks good Fil - good to get the iOS only cruft out of there. >>>> >>> >>>> >>>Minor point but there's a way to hide the private vars in the >>>> >>>implementation file since users won't need to really see them in >>>>the >>>> >>>public headers, through class extensions: >>>> >>> >>>> >>>>http://stackoverflow.com/questions/5826345/how-to-declare-instance-vari >>>>a >>>>b >>>>l >>>> >>>es-and-methods-not-visible-or-usable-outside-of-t >>>> >>>Not that prevents access, just visibility, and we are open source >>>>after >>>> >>>all... >>>> >>> >>>> >>>2012/3/28 Filip Maj <[email protected]>: >>>> >>>> I've sent a pull request to both apache/cordova-js and >>>> >>>>apache/cordova-ios >>>> >>>> for the geo stuff (side note: github pull requests might be >>>>broken >>>> again >>>> >>>> :/) >>>> >>>> >>>> >>>> https://github.com/apache/incubator-cordova-ios/pull/8 >>>> >>>> >>>> >>>> >>>> >>>> The above gets the native iOS geolocation plugin lined up to the >>>>new >>>> >>>> interface imposed by cordova-js. >>>> >>>> >>>> >>>> The JS file is bundled in the branch so you don't need to rebuild >>>> >>>> cordova-js to see these improvements. >>>> >>>> >>>> >>>> Shaz and anyone else keen on the iOS platform: can you guys take >>>>a >>>> look? >>>> >>>> My tests on my end look good but would love another set of eyes. >>>> >>>> >>>> >>>> On 3/27/12 5:00 PM, "Filip Maj" <[email protected]> wrote: >>>> >>>> >>>> >>>>>So we'll keep it around (even though we won't override the >>>> >>>>>implementation >>>> >>>>>that we get for free in the WebView), and thus we'll need to >>>>update >>>> the >>>> >>>>>native implementation. Gotcha. I will update the JIRA issue as >>>>such >>>> >>>>>then. >>>> >>>>> >>>> >>>>>On 3/27/12 4:57 PM, "Joe Bowser" <[email protected]> wrote: >>>> >>>>> >>>> >>>>>>We should axe it/move it to its own plugins repo. Someone may >>>>want >>>> it, >>>> >>>>>>but >>>> >>>>>>I don't want to make promises for it. >>>> >>>>>> >>>> >>>>>>On Tue, Mar 27, 2012 at 4:55 PM, Filip Maj <[email protected]> >>>>wrote: >>>> >>>>>> >>>> >>>>>>> I am going to assume then that will be merged in and we'll be >>>> making >>>> >>>>>>>the >>>> >>>>>>> necessary native tweaks across the platforms we want to >>>>support. >>>> >>>>>>> >>>> >>>>>>> ANDROID peeps: should we axe native geo code, or should we >>>>keep >>>>it >>>> >>>>>>>around >>>> >>>>>>> and thus update the implementation to follow this new >>>>approach? >>>> >>>>>>> >>>> >>>>>>> On 3/27/12 3:33 PM, "Shazron" <[email protected]> wrote: >>>> >>>>>>> >>>> >>>>>>> >Apple should be following W3C, although I don't know if they >>>>fixed >>>> >>>>>>> >this bug yet, it's still unresolved for me in Radar: >>>> >>>>>>> >http://openradar.appspot.com/radar?id=1160403 but based on >>>>what >>>> my >>>> >>>>>>> >test on 5.1, they've fixed it. >>>> >>>>>>> > >>>> >>>>>>> >Sure ping me on email/jabber let's set up a time. >>>> >>>>>>> > >>>> >>>>>>> >On Tue, Mar 27, 2012 at 3:07 PM, Filip Maj <[email protected]> >>>>wrote: >>>> >>>>>>> >> Assuming that the native WebView implementations across >>>>whatever >>>> >>>>>>> >>platforms >>>> >>>>>>> >> adhere to the W3C Geo spec, then these native changes would >>>>line >>>> >>>>>>>up >>>> >>>>>>>our >>>> >>>>>>> >> implementation with what users are expecting in their >>>>browser. >>>> >>>>>>> >> >>>> >>>>>>> >> I can help with tweaking the implementation on iOS, but >>>>would >>>> love >>>> >>>>>>>if >>>> >>>>>>> >>you >>>> >>>>>>> >> could once-over it, Shaz, and perhaps jump on a quick >>>>remote >>>> hack >>>> >>>>>>>sesh >>>> >>>>>>> >> with me for 15-20 mins to make sure we are looking good. >>>> >>>>>>> >> >>>> >>>>>>> >> On 3/27/12 2:46 PM, "Shazron" <[email protected]> wrote: >>>> >>>>>>> >> >>>> >>>>>>> >>>Thanks Fil - I'm all for fixing geolocation in iOS. There's >>>> >>>>>>>several >>>> >>>>>>> >>>jira issues for it, and I've been attempting to fix it as >>>>best I >>>> >>>>>>>can, >>>> >>>>>>> >>>but users are still reporting problems with it since it >>>>doesn't >>>> >>>>>>>match >>>> >>>>>>> >>>the native implementation of UIWebView. >>>> >>>>>>> >>> >>>> >>>>>>> >>>On Mon, Mar 26, 2012 at 4:00 PM, Filip Maj <[email protected]> >>>> wrote: >>>> >>>>>>> >>>> Hey all, >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> The past week or so I've been working on revamping the >>>> >>>>>>>geolocation >>>> >>>>>>> >>>>tests according to what is laid out by the W3C API [1]. >>>>Been >>>> >>>>>>>tracking >>>> >>>>>>> >>>>progress and whatnot in a JIRA issue [2]. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Good news: I've got the tests implemented plus cordova-js >>>> >>>>>>>passing >>>> >>>>>>>said >>>> >>>>>>> >>>>tests (compare view to see diff available @ [3]). >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Bad news: we've been doing it wrong in our native >>>> >>>>>>>implementations >>>> >>>>>>>forŠ >>>> >>>>>>> >>>>well, ever, it seems. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Moving forward would like to hear suggestions from >>>>everyone. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Breaking down what we didn't do in the past that the spec >>>> >>>>>>>mandates: >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> * Properly implementing a timeout. It is one of the >>>> available >>>> >>>>>>> >>>>options that you can pass into getCurrentPosition / >>>> >>>>>>>watchPosition. >>>> >>>>>>> >>>>However, we've been using it to date as essentially a >>>> "frequency" >>>> >>>>>>> >>>>parameter for watchPosition, I.e. "give me position >>>>updates >>>> every >>>> >>>>>>> >>>><options.timeout> milliseconds". This is wrong. According >>>>to >>>> the >>>> >>>>>>>spec, >>>> >>>>>>> >>>>the timeout option defines how long after invoking a >>>> >>>>>>>watch/getCurrent >>>> >>>>>>> >>>>the error callback should wait before it fires with a >>>>TIMEOUT >>>> >>>>>>> >>>>PositionError object. >>>> >>>>>>> >>>> * There is no control over how often watchPosition >>>>should >>>> >>>>>>>fire >>>> >>>>>>> >>>>success callbacks. Instead, the spec says: "In step 5.2.2 >>>>of >>>> the >>>> >>>>>>>watch >>>> >>>>>>> >>>>process, the successCallback is only invoked when a new >>>> position >>>> >>>>>>>is >>>> >>>>>>> >>>>obtained and this position differs signifficantly from the >>>> >>>>>>>previously >>>> >>>>>>> >>>>reported position. The definition of what consitutes a >>>> >>>>>>>significant >>>> >>>>>>> >>>>difference is left to the implementation." >>>> >>>>>>> >>>> * I've also added tests + control of comparing the >>>> >>>>>>>"maximumAge" >>>> >>>>>>> >>>>parameter on the JS side, and keeping a reference to the >>>>last >>>> >>>>>>> >>>>successful >>>> >>>>>>> >>>>position retrieved from the native framework and comparing >>>>its >>>> >>>>>>> >>>>timestamp >>>> >>>>>>> >>>>together with maximumAge. This should implement proper >>>>caching >>>> of >>>> >>>>>>> >>>>positioning on the WebView side and hopefully save some >>>>native >>>> >>>>>>>round >>>> >>>>>>> >>>>trips. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> All of this means the the API on the native side for >>>> geolocation >>>> >>>>>>>will >>>> >>>>>>> >>>>change (sorry iOS!). Basically we have three actions that >>>>the >>>> >>>>>>> >>>>Geolocation plugin should listen for: >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> * getLocation, which takes as parameters >>>>enableHighAccuracy >>>> >>>>>>> >>>>(boolean) and maximumAge (int as milliseconds). >>>> >>>>>>> >>>> * addWatch, parameter: only the usual callbackID >>>>required. >>>> >>>>>>> >>>> * clearWatch, parameter: only the usual callbackID >>>> required. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> getLocation should require very little changing (other >>>>than >>>> not >>>> >>>>>>> >>>>needing >>>> >>>>>>> >>>>the timeout parameter anymore, since that is handled on >>>>the >>>>JS >>>> >>>>>>>side >>>> >>>>>>>in >>>> >>>>>>> >>>>my patch). >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> addWatch should keep a list of callback Ids, and, as soon >>>>as >>>> we >>>> >>>>>>>have >>>> >>>>>>> >>>>one watch started, the native framework should start >>>>watching >>>> the >>>> >>>>>>> >>>>position for a "significant position difference". Once >>>>that >>>> >>>>>>>happens, it >>>> >>>>>>> >>>>should fire the success callback(s) for all stored watch >>>> callback >>>> >>>>>>>Ids. >>>> >>>>>>> >>>>If there is an issue retrieving position, it should fire >>>>the >>>> >>>>>>>error >>>> >>>>>>> >>>>callback(s) for all stored watch callback Ids. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> I commented out a bunch of iOS-specific code that already >>>>did >>>> a >>>> >>>>>>> >>>>"distance filter" type of approach to position >>>>acquisition, >>>>but >>>> >>>>>>>was >>>> >>>>>>> >>>>only >>>> >>>>>>> >>>>available if you provided undocumented parameters in the >>>> options >>>> >>>>>>> >>>>object. >>>> >>>>>>> >>>>Not sure about how feasible a distance filter is in >>>>Android, or >>>> >>>>>>>Windows >>>> >>>>>>> >>>>Phone, or our other supported platforms. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> One final point of discussion worth bringing up about >>>>this >>>> >>>>>>>issue. >>>> >>>>>>> >>>>BlackBerry and Android use the default implementation of >>>> >>>>>>>geolocation >>>> >>>>>>> >>>>abilities in their respective WEbViews. Because of this I >>>>would >>>> >>>>>>>mandate >>>> >>>>>>> >>>>removal of any Geolocation java code from the Android + >>>> >>>>>>>BlackBerry >>>> >>>>>>> >>>>implementations. Saves some bytes. Originally we had the >>>> Android >>>> >>>>>>>plugin >>>> >>>>>>> >>>>classes in there for support for devices before 2.0. Since >>>>we >>>> are >>>> >>>>>>>only >>>> >>>>>>> >>>>supporting 2.0 and above, this is no longer needed. Are >>>>there >>>> any >>>> >>>>>>> >>>>issues >>>> >>>>>>> >>>>with this? >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Appreciate you guys looking this over. >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> Cheers, >>>> >>>>>>> >>>> Fil >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> [1] >>>> http://dev.w3.org/geo/api/spec-source.html#api_description >>>> >>>>>>> >>>> [2] https://issues.apache.org/jira/browse/CB-359 >>>> >>>>>>> >>>> [3] >>>> >>>>>>> >>>> >>>> >>>>>>> >>>> >>>>>>> >>>> https://github.com/filmaj/incubator-cordova-js/compare/master...geotes >>>> >>>>>>>t >>>> >>>>>>> >>>>s >>>> >>>>>>> >> >>>> >>>>>>> >>>> >>>>>>> >>>> >>>>> >>>> >>>> >>>> >> >>>> >> >
