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

[TIMOB-8746] Android: Geolocation e.coords.longitude is returning undefined. #2055

Merged
merged 6 commits into from Apr 25, 2012
Merged

[TIMOB-8746] Android: Geolocation e.coords.longitude is returning undefined. #2055

merged 6 commits into from Apr 25, 2012

Conversation

vishalduggal
Copy link
Contributor

Associated ticket in JIRA

Verified that neither IOS nor MobileWeb have co-ordinates in the locationEvent when success is false. Updated docs to reflect this.
Updated error event to match the definition in the docs.

@@ -45,7 +45,6 @@

public class TiLocation implements Handler.Callback
{
public static final int ERR_POSITION_UNAVAILABLE = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be kept in place. the error this represents stems from a failure when interacting with the native LocationManager which is wrapped by TiLocation. Since the error is not generated by interaction with LocationProviderProxy, moving the error constant to LocationProviderProxy creates a disconnect.

e.put(TiC.ERROR_PROPERTY_CODE, code);
e.put(TiC.ERROR_PROPERTY_MESSAGE, msg);
d.put(TiC.EVENT_PROPERTY_ERROR, e);
HashMap<String, Object> d = new HashMap<String, Object>(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good change but it was a mistake when we originally put this method inside TiConvert since this is used only by Geolocation. If possible, I think this should be moved to GeolocationModule.java along with the existing buildLocationEvent() method and events should be built in a more generic way.

For example:
-success property should exist in all events for Geolocation events and ideally be set as part of a "buildBaseEvent" or "buildGeolocationEvent" perhaps that is then invoked by the "buildLocationEvent" and "buildErrorEvent" calls
-likewise, provider should always be present

I know this is arguably scope creep, but since the fix involves changing how the event is built we can (and should IMO) take this opportunity to update the mechanism to something that will cause less problems in the future by maintaining better encapsulation, code re-use and event consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have moved the method to the geolocation module but am uncomfortable about the scope creep. I'll file a separate ticket as an API enhancement which we will pick up in a later sprint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JIRA isue filed TIMOB-8882

@rusticphilosopher
Copy link
Contributor

FR in progress

@rusticphilosopher
Copy link
Contributor

Code reviewed and FR passed. docgen and validate also passed. Accepted

rusticphilosopher pushed a commit that referenced this pull request Apr 25, 2012
[TIMOB-8746] Android: Geolocation e.coords.longitude is returning undefined.
@rusticphilosopher rusticphilosopher merged commit 555b1e6 into tidev:master Apr 25, 2012
@vishalduggal vishalduggal deleted the timob-8746 branch October 27, 2014 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants