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

Stop using WeakReference in BaseLocationClient #5624

Merged
merged 8 commits into from
Jul 18, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jun 2, 2023

Closes #5208

What has been done to verify that this works as intended?

I've tested the changes manually.

Why is this the best possible solution? Were any other approaches considered?

The only interesting thing here is that after getting rid of using WeakReference I decided to combine setting listeners with start/stop methods see: d888d58 and 2246046 Those two were always called together so I thought that it made sense to combine them.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Please test everything related to using GPS which means geo widgets and background location tracking. Please also test carefully if after recreating the activity/getting back to the app tracking location works correctly since this is the biggest area of risk I guess.

Do we need any specific form for testing your changes? If so, please attach one.

Any form with geo widgets and background location tracking like the All widgets form and AuditTest.xml.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review June 2, 2023 16:24
@grzesiek2010 grzesiek2010 requested a review from seadowg June 2, 2023 16:24
@grzesiek2010 grzesiek2010 changed the title Collect 5208 Stop using WeakReference in BaseLocationClient Jun 2, 2023
@dbemke
Copy link

dbemke commented Jul 6, 2023

Something strange happened with maps. Mapbox is not available as a source of maps in settings and Google maps don't show the map (blank background, unable to zoom in/out)
maps

@seadowg
Copy link
Member

seadowg commented Jul 6, 2023

@dbemke are you using the build created by CI? I'm realising you'll need to manually build to test maps (other than OSM), as we don't include our Google Maps or Mapbox keys in the CI build (so neither will work).

@dbemke
Copy link

dbemke commented Jul 6, 2023

Yes, the CI build

@dbemke
Copy link

dbemke commented Jul 18, 2023

Tested with Success!

Verified on device with Android 10

Verified cases:

  • regression tests with Geo widgets and background location recording (audit logs)
  • Google Maps, OSM, Mapbox
  • don’t keep activities enabled and disabled
  • no background processes
  • minimizing the app, rotating the screen
  • location permissions enabled, disabled
  • quick checks on Android 8.1

@srujner
Copy link

srujner commented Jul 18, 2023

Tested with Success!

Verified on device with Android 13

@grzesiek2010 grzesiek2010 merged commit e02d656 into getodk:master Jul 18, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop using WeakReference in BaseLocationClient
4 participants