search

onaio / kujaku

Mapping and check-in library for Android using MapBox SDK
https://ona.io
BSD 2-Clause "Simplified" License
18 stars 11 forks source link

GPS Passive recording feature #275

Closed eotin closed 5 years ago

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 1328

Changes Missing Coverage Covered Lines Changed/Added Lines %
library/src/main/java/io/ona/kujaku/services/options/TrackingServiceHighAccuracyOptions.java 12 13 92.31%
library/src/main/java/io/ona/kujaku/services/options/TrackingServiceSaveBatteryOptions.java 12 13 92.31%
sample/src/main/java/io/ona/kujaku/sample/activities/WmtsActivity.java 0 1 0.0%
library/src/main/java/io/ona/kujaku/activities/MapActivity.java 0 2 0.0%
library/src/main/java/io/ona/kujaku/helpers/storage/TrackingStorage.java 29 31 93.55%
sample/src/main/java/io/ona/kujaku/sample/activities/BaseNavigationDrawerActivity.java 0 3 0.0%
library/src/main/java/io/ona/kujaku/exceptions/TrackingServiceNotInitializedException.java 0 4 0.0%
library/src/main/java/io/ona/kujaku/utils/KujakuMultiplePermissionListener.java 0 5 0.0%
library/src/main/java/io/ona/kujaku/helpers/storage/BaseStorage.java 69 84 82.14%
library/src/main/java/io/ona/kujaku/helpers/storage/MapBoxStyleStorage.java 14 39 35.9%
<!-- Total: 466 748 62.3% -->
Files with Coverage Reduction New Missed Lines %
library/src/main/java/io/ona/kujaku/views/KujakuMapView.java 5 26.64%
<!-- Total: 5 -->
Totals Coverage Status
Change from base Build 1318: 14.3%
Covered Lines: 2182
Relevant Lines: 5826
💛 - Coveralls
craigappl commented 5 years ago

@Kigamba or @githengi I just got confirmation from @eotin that this PR is ready for review. Please review this PR.

ekigamba commented 5 years ago

@eotin So I have presented the feature to the team and they though that the following are important:

eotin commented 5 years ago

@eotin So I have presented the feature to the team and they though that the following are important:

  • Have you tested on android 9 with restrictions on foreground service?

No, we do not have devices on Android 9, but i will check the new restrictions on Foreground service

  • Can you create an interface for the location provider so that someone can use either fusedAPI client or GPS?

For now we do not want ot use the fused location API as the GPS is adequate here

  • Can we have the route drawing in the view?

I can try to in the sample activity, but it is more the host responsability.

  • Can we make the record icon configurable?

How did you manage the configuration with the others icons ?

ekigamba commented 5 years ago

@eotin

Can you create an interface for the location provider so that someone can use either fusedAPI client or GPS? The reason is that you do not want to force everyone to use GPS which consumes a lot of power compared to Fused Location API which handles this much more efficiently since the tracking is going to happen for long hours

Can we have the route drawing in the view? I can try to in the sample activity, but it is more the host responsability.

The team thinks that it is an important component of the PR and can be easily done using the ArrowLine layer instead of being done by the host application

Can we make the record icon configurable?

Enable the developer to pass an drawable configuration (drawable id's) that is going to be used for both states that show recording and recording stopped

bripatand commented 5 years ago

@eotin

Can you create an interface for the location provider so that someone can use either fusedAPI client or GPS? The reason is that you do not want to force everyone to use GPS which consumes a lot of power compared to Fused Location API which handles this much more efficiently since the tracking is going to happen for long hours Briandp: from our testing, the Fuse Location API provide inaccurate points and in an unpredictable manner especially if you are not connected to a network. Most of time the resulting line or polygon is highly irregular.

Can we have the route drawing in the view? I can try to in the sample activity, but it is more the host responsability.

The team thinks that it is an important component of the PR and can be easily done using the ArrowLine layer instead of being done by the host application Briandp: we believe relying on the "recording" API call to "display" the collected points on the screen is bad design, confusing and encapsulate too much behaviour in one API call (pattern "separation of concerns"). OK to use ArrowLine in the host to show direction thought.

Can we make the record icon configurable?

Enable the developer to pass an drawable configuration (drawable id's) that is going to be used for both states that show recording and recording stopped Briandp: Sounds good but why only there? We've tried to be consistent. For instance the focusOnMyLocation Icon is not configurable and hardcoded in the library. And there are a few others. So shouldn't we apply the same logic everywhere in Kujaku where icons are used.

ekigamba commented 5 years ago

@bripatand

Briandp: we believe relying on the "recording" API call to "display" the collected points on the screen is bad design, confusing and encapsulate too much behaviour in one API call (pattern "separation of concerns"). OK to use ArrowLine in the host to show direction thought.

Very true. I also understand separation of concerns and did not mean that literally. We are putting a lot of effort to make sure all features are configurable and not default so the the client application can use whichever part it wants and in whichever way.

Briandp: Sounds good but why only there? We've tried to be consistent. For instance the focusOnMyLocation Icon is not configurable and hardcoded in the library. And there are a few others. So shouldn't we apply the same logic everywhere in Kujaku where icons are used.

I don't refute that and sorry 🙏 if there was a misunderstanding. You guys are part of the small GeoWidget community and we(Ona and I) are very thankful for your contributions to the repository. The aim from the start of the project has been to create a library that can be used by the multiple host applications that are currently using it for various purposes, and in the near future. That is why you were able to convince us on using an actual View. The general aim is to provide the configurability aspect whenever required and makes sense.

  1. The API has mostly been designed to either be used with a UI or without for features that would have a UI. That is why we have IKujakuMapView and IKujakuMapViewLowLevel

  2. For features, configurability would be required due to different designs and requirements. I extended this discussion and the design provided would end up being different for different host applications. That's the reason I brought it up. The icon specifically would end up being different, the position and even whether it should be shown when the recording is started programmatically.

  3. The addPoint is one of the features that offers such configurability in the icon to show with two options due to the nature of the feature and usability on different applications

  4. The arrow line layer and boundary layer also offer such configurability to change such UI elements.

    From the discussions on the feature and how important this feature is:

  5. Whether the icon is shown

  6. The position of the icon

  7. The actual icon shown

need to be configurable.

@bripatand I hope this makes sense?

bripatand commented 5 years ago

@ekigamba: Ok for making the following configurable: Whether the icon is shown The position of the icon The actual icon shown

ekigamba commented 5 years ago

@bripatand Sorry, I forgot to reply to this

Briandp: from our testing, the Fuse Location API provide inaccurate points and in an unpredictable manner especially if you are not connected to a network. Most of time the resulting line or polygon is highly irregular.

It does provide inaccurate points sometimes and it aggregates all. The use of the tracking feature means that is will be on for long hours as the user performs duties in the field. The team thought that it would be good to provide the option to use a different provider not necessarily completely change to GPS. GPS is still the most accurate but it might have some cons that might not fit the situation as opposed to FusedLocation API which includes all these providers to provide an OK location while not using too much energy.

I also tested the feature while commuting(3 times) and it worked excellent once. One of the cases was a bug which caused the app to crash when I tried to check my route on arrival. For the other case, the only points that were recorded was when I started the journey and when I arrived. My assumption is that it lost GPS connection and therefore only got the points when I was stationery. On the other hand, I used Google Maps for directions twice still commuting in the similar manner and it was pretty accurate. I never lost connection, thus it would be nice to have other options. I believe FusedLocationAPI.

**

  1. The route taken was the same
  2. The area is urban, Kenya's capital city
  3. I also had my internet connection off on all cases

Only the google maps situation is different and circumstances much more harsh **

@bripatand What are your thoughts?

bripatand commented 5 years ago

@ekigamba: as you have seen, the FusedLocationAPI is unpredictable as you cannot garanty to get a point every X meter using this API. You tested it 3 times and you got 3 different behaviour. And if you don't have any connectivity, the FusedLocationAPI is going to use the GPS anyway, just adding an unpredictability... Note we have 2 modes in the current API, one that keep the GPS on all the time which gives the best results in term of accuracy and another one which relies on the GPS to tell us when the phone has moved by X meters but it is much less accurate. If we want the ability to use the FusedLocationAPI, we would need to make significant changes to the code and provide clear instruction on when to use GPS and when to use Fuse. We honestly don't know when we would recommend to use the Fuse if your objective is to collect a "route" you can use. Note as well that Google apps (Map etc...) have root privililege and can do some actions that no other app can, typically bypassing the power saving feature enforced by Android when needed... Finally, you cannot really lose connection to the GPS unless you are inside or potentially in New York surrounded by tall buildings. There are always at least 3 GPS satellites above your head at any time anywhere on earth (may be not at the poles). It might take several minutes to connect to them if the GPS has been off for a long time but once you get the first fix/point, it means it is connected to at least 3 of them and from that moment getting a point is immediate. But if you think it is important, you can raise it to the group for adding it in the next release.

ekigamba commented 5 years ago

@bripatand

Cool, I understand why you are against it. Would it be possible to just abstract the specific location provider used just as we have done before in the KujakuMapView, so that all one would have to do is implement their own provider and pass it over?

There is a location client interface already https://github.com/onaio/kujaku/blob/master/library/src/main/java/io/ona/kujaku/interfaces/ILocationClient.java and all that would be required is to have this as the client in the service and have a GPS client or rather AndroidLocationClient implementing this and being the default LocationClient unless another one is specified.

Would this be OK by you guys?

bripatand commented 5 years ago

@bripatand

Cool, I understand why you are against it. Would it be possible to just abstract the specific location provider used just as we have done before in the KujakuMapView, so that all one would have to do is implement their own provider and pass it over?

There is a location client interface already https://github.com/onaio/kujaku/blob/master/library/src/main/java/io/ona/kujaku/interfaces/ILocationClient.java and all that would be required is to have this as the client in the service and have a GPS client or rather AndroidLocationClient implementing this and being the default LocationClient unless another one is specified.

Would this be OK by you guys?

We should test it first to see if we can get the FuseAPI to provide a good enough sequence of points. Our experience of this API is that it can be unpredictable. We should also discuss with the geo spatial widget group. But for a future release potentially.

ekigamba commented 5 years ago

We should test it first to see if we can get the FuseAPI to provide a good enough sequence of points. Our experience of this API is that it can be unpredictable. We should also discuss with the geo spatial widget group. But for a future release potentially.

That's OK, any notes on the issue can be added here for future reference https://github.com/onaio/kujaku/issues/283