Adds data layer for location background sample. #227

codingjeremy · 2020-03-12T23:19:12Z

No description provided.

codingjeremy · 2020-03-13T03:09:10Z

Hope to get your final input on the permission + location + DB design for this simple sample.

I will probably move to coroutines + flow in next version.

nic0lette

Only a few minor things, but I'll let Florina + Sean comment

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

florina-muntenescu · 2020-03-13T19:03:24Z

...rc/main/java/com/google/android/gms/location/sample/locationupdatesbackgroundkotlin/Utils.kt

+    // Background permissions didn't exit prior to Q, so it's approved by default.
+    if (permission == Manifest.permission.ACCESS_BACKGROUND_LOCATION &&
+        android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.Q) {
+


new line not needed

suggestion: ktlint on source to find formatting issues.

ran ktlint and fixed a bunch of other stuff, but it says this is ok... ?

No idea, but I removed anyway. :)

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

florina-muntenescu · 2020-03-13T19:07:14Z

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

+ * Access point for database (MyLocation data) and location APIs (start/stop location tracking and
+ * checking tracking status).
+ */
+class LocationRepository private constructor(private val myLocationDatabase: MyLocationDatabase,


consider renaming MyLocationDatabase to just LocationDatabase
but here, it looks like you don't need the entire database, but just the dao, so you can just pass it as a parameter

Done.

For MyLocationDatabase, I wanted to imply it wasn't a database of the Android Location class, but maybe I am reading too much into it. I could rename them SimpleLocation or something. (They are basically simplified versions of Android's Location.)

IDK, what do you think?

florina-muntenescu · 2020-03-13T19:10:37Z

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

+    // Database related fields/methods:
+    private val locationDao = myLocationDatabase.locationDao()
+
+    private val executor = Executors.newSingleThreadExecutor()


because you're creating the executor here, it means that you're not able to unit test this class easily.
I suggest passing the executor as a constructor parameter as well. In tests, this can be replaced with an instant executor.

ah, good call, moved.

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

.../google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationDao.kt

florina-muntenescu · 2020-03-13T19:18:22Z

...le/android/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationDatabase.kt

+                    MyLocationDatabase::class.java,
+                    DATABASE_NAME
+                )
+                .build()


this should go on the previous line

florina-muntenescu · 2020-03-13T19:18:57Z

...ogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationEntity.kt

+ * {@link android.location.Location} class).
+ */
+@Entity(tableName = "my_location_table")
+data class MyLocationEntity (@PrimaryKey val id:UUID = UUID.randomUUID(),


Could be called LocationEntity
Put the id on the next line and re-format the file

florina-muntenescu · 2020-03-13T19:20:49Z

...ogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationEntity.kt

+                       ) {
+
+    override fun toString(): String {
+


nit: remove extra new line

...roid/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationTypeConverters.kt

objcode

A few comments, otherwise LGTM!

objcode · 2020-03-13T21:01:36Z

...rc/main/java/com/google/android/gms/location/sample/locationupdatesbackgroundkotlin/Utils.kt

+    // Background permissions didn't exit prior to Q, so it's approved by default.
+    if (permission == Manifest.permission.ACCESS_BACKGROUND_LOCATION &&
+        android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.Q) {
+


suggestion: ktlint on source to find formatting issues.

objcode · 2020-03-13T21:02:23Z

...rc/main/java/com/google/android/gms/location/sample/locationupdatesbackgroundkotlin/Utils.kt

+) {
+    val provideRationale = shouldShowRequestPermissionRationale(permission)
+
+    // If the user denied a previous request, but didn't check "Don't ask again", we provide


Move comment to KDoc (and add kdoc for other public method)

ktlint said that is ok... so IDK? :-D

Moved comment up to KDoc.

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

objcode · 2020-03-13T21:08:32Z

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

+ */
+class MyLocationManager private constructor(private val context: Context) {
+
+    val trackingLocation: MutableLiveData<Boolean> by lazy {


+1 remove this lazy since the LiveData should do no work when there are no observers.

Same note about exposing LiveData.

Also, add kdoc for public API.

objcode · 2020-03-13T21:40:38Z

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

+            fusedLocationClient.requestLocationUpdates(locationRequest, locationUpdatePendingIntent)
+
+        } catch (e: SecurityException) {
+            trackingLocation.value = false


Same note about main-thread only .value =

objcode · 2020-03-13T21:40:46Z

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

+
+    fun stopLocationUpdates() {
+        Log.d(TAG, "stopLocationUpdates()")
+        trackingLocation.value = false


Same note about main-thread only .value =

Added to method call since it's only call from Fragment. LMK if that isn't what you meant.

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt

…d strings).

codingjeremy · 2020-03-17T01:10:47Z

PTAL, I think I addressed everything. Thank you again for reviewing!

codingjeremy

Resolved all issues, PTAL.

.../google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/db/MyLocationDao.kt

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

codingjeremy

Resolved all issues, PTAL.

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt

objcode

LGTM nice one!

codingjeremy · 2020-03-19T17:49:49Z

@objcode - Thanks!

Talked to Florina yesterday and she approved.

@nic0lette - I will do another followup PR if you have any additional changes.

Adds Location data layer files and permission extension funs.

fdfa4b0

codingjeremy changed the base branch from master to LocationUpdateUI March 13, 2020 03:06

codingjeremy changed the base branch from LocationUpdateUI to master March 13, 2020 03:06

codingjeremy requested review from florina-muntenescu, nic0lette and objcode March 13, 2020 03:08

nic0lette reviewed Mar 13, 2020

View reviewed changes

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt Outdated Show resolved Hide resolved

...oogle/android/gms/location/sample/locationupdatesbackgroundkotlin/data/LocationRepository.kt Outdated Show resolved Hide resolved

florina-muntenescu reviewed Mar 13, 2020

View reviewed changes

objcode reviewed Mar 13, 2020

View reviewed changes

Addresses PR feedback (name changes, style changes, and removes unuse…

3c7720c

…d strings).

codingjeremy commented Mar 17, 2020

View reviewed changes

...google/android/gms/location/sample/locationupdatesbackgroundkotlin/data/MyLocationManager.kt Show resolved Hide resolved

codingjeremy requested review from nic0lette, objcode and florina-muntenescu March 17, 2020 18:43

objcode approved these changes Mar 19, 2020

View reviewed changes

codingjeremy merged commit 2d141ce into master Mar 19, 2020

codingjeremy deleted the add-data-layer branch March 19, 2020 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds data layer for location background sample. #227

Adds data layer for location background sample. #227

codingjeremy commented Mar 12, 2020

codingjeremy commented Mar 13, 2020

nic0lette left a comment

florina-muntenescu Mar 13, 2020

objcode Mar 13, 2020

codingjeremy Mar 17, 2020

florina-muntenescu Mar 13, 2020

codingjeremy Mar 17, 2020

florina-muntenescu Mar 13, 2020

codingjeremy Mar 17, 2020

florina-muntenescu Mar 13, 2020

codingjeremy Mar 17, 2020

florina-muntenescu Mar 13, 2020

codingjeremy Mar 17, 2020

florina-muntenescu Mar 13, 2020

codingjeremy Mar 17, 2020

objcode left a comment

objcode Mar 13, 2020

objcode Mar 13, 2020

codingjeremy Mar 17, 2020

objcode Mar 13, 2020

objcode Mar 13, 2020

objcode Mar 13, 2020

codingjeremy Mar 17, 2020

codingjeremy commented Mar 17, 2020

codingjeremy left a comment

codingjeremy left a comment

objcode left a comment

codingjeremy commented Mar 19, 2020

Adds data layer for location background sample. #227

Adds data layer for location background sample. #227

Conversation

codingjeremy commented Mar 12, 2020

codingjeremy commented Mar 13, 2020

nic0lette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjeremy commented Mar 17, 2020

codingjeremy left a comment

Choose a reason for hiding this comment

codingjeremy left a comment

Choose a reason for hiding this comment

objcode left a comment

Choose a reason for hiding this comment

codingjeremy commented Mar 19, 2020