Timob 7565 - geolocation refactor #1622
Conversation
…ion rule and another bug fix for a rare edge case
| /** | ||
| * GeolocationModule exposes all common methods and properties relating to geolocation behavior | ||
| * associated with Ti.Geolocation to the Titanium developer. Only cross platform API points should | ||
| * be exposed through this class as Android only API points or types should be put in a Android module |
There was a problem hiding this comment.
better "Android-only". I had to read that twice.
|
Currently updating behavior mode switching logic according to new developments. |
|
Updated implementation, Jira ticket with test case and Geolocation spec linked from ticket. |
|
|
||
|
|
||
| /** | ||
| * Constructor |
There was a problem hiding this comment.
I'm not sure the description is really telling us anything we don't already know -- maybe overkill :)
There was a problem hiding this comment.
Trying to be consistent. Every method should have a javadoc even if it's a placeholder IMO.
There was a problem hiding this comment.
But why? If there isn't a description then you're not adding any value to the Javadoc
There was a problem hiding this comment.
Why? If there isn't an actual description then what value are you adding to the Javadoc?
| * @see org.appcelerator.kroll.KrollModule#propertyChanged(java.lang.String, java.lang.Object, java.lang.Object, org.appcelerator.kroll.KrollProxy) | ||
| */ | ||
| @Override | ||
| public void propertyChanged(String key, Object oldValue, Object newValue, KrollProxy proxy) |
There was a problem hiding this comment.
This method is a little large -- do you think it would be better to break each property it's own method?
|
Code review complete, please address comments. There are a few other general comments I had:
|
Conflicts: android/titanium/src/java/org/appcelerator/titanium/TiC.java
| protected static final int MSG_ENABLE_LOCATION_PROVIDERS = KrollModule.MSG_LAST_ID + 100; | ||
| protected static final int MSG_LAST_ID = MSG_ENABLE_LOCATION_PROVIDERS; | ||
|
|
||
| private final String TAG = "GeolocationModule"; |
There was a problem hiding this comment.
Looks like these were converted to instance a little overzealously .. this block of privates should probably be static (they are ALL_CAPS)
|
Code review #2 complete, please address comments. General notes:
|
|
Code review accepted |
|
Pull Request Accepted: Tested with v8/rhino on LG Slate 3.1 and Droid 2.2.2 with attached TC in bug and KS |
|
Functional review accepted. Ran the test case in TIMOB-7565 on v8 and Rhino, and also verified the KS Geo test in v8 and Rhino on a Galaxy Nexus / 4.0.2. Some notes:
|
Timob 7565 - geolocation refactor
https://jira.appcelerator.org/browse/TIMOB-7565
also resolves:
https://jira.appcelerator.org/browse/TIMOB-7456
https://jira.appcelerator.org/browse/TIMOB-4779
Total refactor of Geolocation module (minus compass).
Testing instructions in ticket: https://jira.appcelerator.org/browse/TIMOB-7565
This is fairly localized but still a major change and requires KS geo pass, testing with app in ticket and ideally drillbit to safe on BOTH runtimes before this should be merged. This testing really needs to occur on a variety of devices.
** it might be easier to look at the raw files for the most part rather than the diff