Dependency updates for v2024.4 #6505

grzesiek2010 · 2024-11-12T19:32:21Z

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

As usual, I've updated the dependencies for the next release. However, this time, there are some significant changes in addition to the regular updates:

Switched to Gradle Version Catalog: Dependency management now uses the Gradle Version Catalog, which is the default approach for managing dependencies in new projects. It offers several advantages over the previous buildSrc module, such as faster project builds when dependencies are changed and notifications about new available versions. For details, see this commit.
Migrated to FusedLocationProviderClient: This migration was necessary to update the play-services-location library. For more information, see this commit.
Reviewed and Fixed Some PMD Rules: Over the last few dependency updates, we had disabled some new PMD rules. After a review, I re-enabled a few of them. However, others were left disabled as they either don’t make much sense, would require extensive edits across hundreds of classes, or are faulty and don’t work as intended.

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?

It's often challenging to determine what should be tested when multiple dependencies are updated. However, in this case, I can highlight one area: location recording. As mentioned earlier, we switched to a new location provider, which could be a potential source of bugs. In fact, this is our second attempt at the migration, as the first one failed due to this issue: #6027. Hopefully, everything will go smoothly this time.

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

A form with a few geopints might be helpful for testing reading locations:
geo5.xlsx

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:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
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

seadowg

Loving the change to version catalog! I used it recently on a different app and thought it was so much nicer, but definitely a pain to convert to. Thanks for taking the time to do that.

seadowg · 2024-11-21T11:25:52Z

gradle/libs.versions.toml

+[plugins]
+androidApplication = { id = "com.android.application" }


Is there a way to declare the plugin versions here so we don't still need these declared in the root build.gradle?

Yes, I even attempted to do that, but since it was quite complex (requiring a lot of additional changes) and some of the functions I would need to use are still marked as unstable, I decided to hold off for now.

seadowg · 2024-11-21T11:29:08Z

async/build.gradle.kts

 }

 apply(from = "../config/quality.gradle")

 android {
-    compileSdk = Versions.android_compile_sdk
+    compileSdk = libs.versions.compileSdk.get().toInt()


I'm thinking it might make sense to still declare the compile/target/mid SDKs in as constants in Gradle (example below). It doesn't seem like version catalog lets us define integer values. They're also more like shared configuration than library versions.

buildscript { ext { minSdkVersion = 21 targetSdkVersion = 34 compileSdkVersion = 34 } ...

In kotlin DSL this would be like this:

compileSdk = rootProject.extra["compileSdkVersion"] as Int

so it would also require casting to Int. Given that I don't think this approach would be any better.

Oh, wow. Kotlin makes that much worse - in Groovy it'd be rootProject.ext.minSdkVersion. Let's leave it as-is then.

seadowg · 2024-11-21T11:38:04Z

location/src/main/java/org/odk/collect/location/GoogleFusedLocationClient.kt

+ * Should be used whenever Google Play Services is present. In general, use
+ * [LocationClientProvider] to retrieve a configured [LocationClient].
+ */
+class GoogleFusedLocationClient(


Did any changes get made to GoogleFusedLocationClient or its test? Because the Kotlin version happens in the same commit, the diff doesn't give me any insight into that.

Ah, yes, there are some changes because the old provider used different listeners, so the file structure has changed slightly. The file isn't large, so hopefully comparing it with the old one shouldn't be difficult.

seadowg · 2024-11-26T13:04:24Z

@getodk/testers I think it's worth testing location tracking (background, maps etc) before merging this as we've made changes to how we interact with location services.

dbemke · 2024-11-26T13:28:57Z

@grzesiek2010 Could you send us (in Slack) the apk with access to different maps sources?

dbemke · 2025-01-07T16:13:40Z

The first map opening in Geotrace or Geoshape feels a bit slower than it used to be. It seems that it happens after installing the app and opening the map for the first time. For example, on Android 10 (Redmi 9t) in OSM it takes 10 seconds before the maps zooms in / finishes searching for location. It works well but seems a bit slower.

grzesiek2010 · 2025-01-09T11:07:10Z

Hmm I've just tested it and it took 3s (fresh install) for all providers (Google, Mapbox, OSM). @srujner @WKobus are you also experiencing such differences between versions?

WKobus · 2025-01-09T12:15:27Z

I've tried it now on different providers and I had same results every time it took 2-3s, however I remember that during testing this PR I had a situation when it took ~10 sec to load, but I can't reproduce it anymore.

srujner · 2025-01-09T12:32:15Z

@grzesiek2010 Same with me, during testing I encountered this issue once or twice but now I have not been able to reproduce it anymore.

srujner · 2025-01-09T12:37:16Z

Tested with Success!

Verified on device with Android: 5.1, 8.1, 12, 14

Verified cases:

• Collecting Location in background: ; 
• Viewing Submissions on map; 
• Regression checks for GeoPoint, GeoTrace, GeoShape widgets; 
• Select Map forms 
• Offline Layers; 
• Google Maps, Mapbox, OSM; 
• Collecting Location in background; 
• View Submissions on map; 
• Screen rotation, minimize app, using backward button, 
• Forms from issue

dbemke · 2025-01-09T12:38:43Z

Tested with Success!

Verified on device with Android: 8.1, 10, 14

WKobus · 2025-01-09T12:39:49Z

Tested with Success!

Verified on device with Android: 11, 15

grzesiek2010 · 2025-01-09T13:03:49Z

I've tried it now on different providers and I had same results every time it took 2-3s, however I remember that during testing this PR I had a situation when it took ~10 sec to load, but I can't reproduce it anymore.

Same with me, during testing I encountered this issue once or twice but now I have not been able to reproduce it anymore.

So, it might have been random. Sometimes a device needs more time to read the location, and it doesn’t necessarily have to be related to these changes. Let’s keep an eye on it.

grzesiek2010 force-pushed the dependency_updates_for_v2024.4 branch 30 times, most recently from 1bafc27 to 2068f65 Compare November 14, 2024 18:03

grzesiek2010 added 3 commits November 19, 2024 12:26

Unblock the UseIndexOfChar rule

ae7924a

Unblock the ConsecutiveAppendsShouldReuse rule

5a6f82c

Unblock the GuardLogStatement rule

26b5e18

grzesiek2010 force-pushed the dependency_updates_for_v2024.4 branch from 94f66ef to 0a16698 Compare November 19, 2024 11:38

grzesiek2010 added 6 commits November 19, 2024 18:44

Migrated to gradle version catalog

58c6056

Define plugins with gradle version catalog

d462ec0

Remove redundant targetSDK from a submodule

38355de

Migrated to FusedLocationProviderClient

ed47b9c

Fixed downloading robolectric artifacts

07c0d68

More dependency updates

948a9f1

grzesiek2010 force-pushed the dependency_updates_for_v2024.4 branch from 0a16698 to 948a9f1 Compare November 19, 2024 18:42

grzesiek2010 marked this pull request as ready for review November 19, 2024 19:10

grzesiek2010 requested a review from seadowg November 19, 2024 19:10

seadowg requested changes Nov 21, 2024

View reviewed changes

grzesiek2010 requested a review from seadowg November 21, 2024 19:05

seadowg approved these changes Nov 26, 2024

View reviewed changes

seadowg added the needs testing label Nov 26, 2024

seadowg merged commit c143f9f into getodk:master Dec 10, 2024
6 checks passed

WKobus added behavior verified and removed needs testing labels Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependency updates for v2024.4 #6505

Dependency updates for v2024.4 #6505

grzesiek2010 commented Nov 12, 2024 •

edited

Loading

seadowg left a comment

seadowg Nov 21, 2024

grzesiek2010 Nov 21, 2024

seadowg Nov 21, 2024

grzesiek2010 Nov 21, 2024

seadowg Nov 26, 2024

seadowg Nov 21, 2024

grzesiek2010 Nov 21, 2024

seadowg commented Nov 26, 2024

dbemke commented Nov 26, 2024

dbemke commented Jan 7, 2025

grzesiek2010 commented Jan 9, 2025

WKobus commented Jan 9, 2025

srujner commented Jan 9, 2025

srujner commented Jan 9, 2025

dbemke commented Jan 9, 2025

WKobus commented Jan 9, 2025

grzesiek2010 commented Jan 9, 2025

		[plugins]
		androidApplication = { id = "com.android.application" }

Dependency updates for v2024.4 #6505

Dependency updates for v2024.4 #6505

Conversation

grzesiek2010 commented Nov 12, 2024 • edited Loading

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

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?

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

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

Before submitting this PR, please make sure you have:

seadowg left a comment

Choose a reason for hiding this comment

seadowg Nov 21, 2024

Choose a reason for hiding this comment

grzesiek2010 Nov 21, 2024

Choose a reason for hiding this comment

seadowg Nov 21, 2024

Choose a reason for hiding this comment

grzesiek2010 Nov 21, 2024

Choose a reason for hiding this comment

seadowg Nov 26, 2024

Choose a reason for hiding this comment

seadowg Nov 21, 2024

Choose a reason for hiding this comment

grzesiek2010 Nov 21, 2024

Choose a reason for hiding this comment

seadowg commented Nov 26, 2024

dbemke commented Nov 26, 2024

dbemke commented Jan 7, 2025

grzesiek2010 commented Jan 9, 2025

WKobus commented Jan 9, 2025

srujner commented Jan 9, 2025

srujner commented Jan 9, 2025

dbemke commented Jan 9, 2025

WKobus commented Jan 9, 2025

grzesiek2010 commented Jan 9, 2025

grzesiek2010 commented Nov 12, 2024 •

edited

Loading