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

Dependency updates for v2024.3 #6252

Merged
merged 19 commits into from
Jul 25, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jul 5, 2024

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

As usual, I've updated the dependencies for the next release. Nothing special to discuss here.

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?

Usually, we don't test dependency updates but this time there are some changes that need testing in my opinion:

  1. Myanmar calendar - I wanted to update it at the beginning of the last cycle but I found some bugs that I reported (see Problems with month names in 1.0.8.RELEASE chanmratekoko/mmcalendar#4). The problems should be fixed now but we can't be sure there are no more similar issues. Please test it like it was a new question type - carefully.
  2. Mapbox has been upgraded so it needs to be briefly tested for regressions.
  3. Do you remember the issue with recording locations we had after updating some dependencies last time? (see Location in GeoPoint questions is not always updated if read multiple times #6027). Please make sure it does not occur this time.

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

No.

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

@grzesiek2010 grzesiek2010 force-pushed the dependency_updates_v2024.3 branch 29 times, most recently from 1a049c9 to 7f488eb Compare July 9, 2024 15:06
@grzesiek2010 grzesiek2010 force-pushed the dependency_updates_v2024.3 branch from 9412925 to 3603afc Compare July 9, 2024 21:18
@grzesiek2010 grzesiek2010 marked this pull request as ready for review July 10, 2024 10:57
@grzesiek2010 grzesiek2010 requested a review from seadowg July 10, 2024 10:57
@@ -230,7 +230,7 @@ jobs:
command: ./gradlew assembleSelfSignedRelease

- run:
name: Check APK size isn't larger than 11.5MB
name: Check APK size isn't larger than 11.7MB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Mapbox responsible for this?

Copy link
Member Author

@grzesiek2010 grzesiek2010 Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but I'm not sure if it's only because of Mapbox, the difference (0.2MB) wasn't big enough to make me do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth double-checking so we know before merging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just checked and it's not only Mapbox however to check which other dependencies affected the build size I would need to revert the updates one by one and build apks. It's also possible that it's not just one but a few so I think it's not worth it.

ktlint_standard_binary-expression-wrapping = disabled
ktlint_standard_condition-wrapping = disabled
ktlint_standard_function-literal = disabled
ktlint_standard_backing-property-naming = disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like function-literal and backing-property-naming are nice improvements, actually. Could we fix them instead of disabling? Agreed the other feel unneeded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't analyze which of those rules are worth enabling. I was thinking that maybe it would be a good task for me while you are off in September? It would be something easier to review for @lognaturel.

@@ -26,10 +26,13 @@
<exclude name="JUnitUseExpected" /> <!-- Favor using fail explicitly to test correct exception -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to both inspecting these changes as most code will be Kotlin going forward anyway.

@grzesiek2010 grzesiek2010 requested a review from seadowg July 15, 2024 12:38
@grzesiek2010 grzesiek2010 added this to the v2024.3 milestone Jul 15, 2024
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grzesiek2010 grzesiek2010 force-pushed the dependency_updates_v2024.3 branch from a17eefc to 3603afc Compare July 15, 2024 14:50
@grzesiek2010 grzesiek2010 requested a review from seadowg July 15, 2024 14:53
@seadowg
Copy link
Member

seadowg commented Jul 15, 2024

@grzesiek2010 probably makes sense to let testing happen before merging here!

@dbemke
Copy link

dbemke commented Jul 16, 2024

ANRs occur while opening maps on all devices. Most likely it's just Google maps. The issue doesn't occur on the master version apk build with and without tokens (Google maps or OSM open first).
Steps to reproduce:

  1. Install the app and download the demo project.
  2. Open any map

@srujner
Copy link

srujner commented Jul 16, 2024

Performance in the Myanmar calendar is very poor.
In other calendars the date change is smooth, in Myanmar it visibly stutters.

XRecorder_16072024_104633.mp4

@grzesiek2010
Copy link
Member Author

Performance in the Myanmar calendar is very poor.
In other calendars the date change is smooth, in Myanmar it visibly stutters.

I've reverted that change in the Myanmar question type. It seems to be a problem in the newest version of the library which I've reported here: chanmratekoko/mmcalendar#4 (comment). In this case, there is no need to test it.

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Jul 22, 2024

ANRs occur while opening maps on all devices. Most likely it's just Google maps. The issue doesn't occur on the master version apk build with and without tokens (Google maps or OSM open first).

I've downgraded the dependency that was responsible for that to the version we used in v2024.2 and everything should be fine now. Upgrading it so that it works without ANRs will require some more changes so I will file a separate issue for that.

@dbemke
Copy link

dbemke commented Jul 25, 2024

Tested with Success!

Verified on a device with Android 10

Verified Cases:

  • Mapbox
  • regressions checks in geowidgets, forms with maps with all sources of maps
  • regression checks in offline layers
  • All widgets form
  • various regression checks in different forms

@WKobus
Copy link

WKobus commented Jul 25, 2024

Tested with Success!

Verified on a device with Android 14

@grzesiek2010 grzesiek2010 merged commit 5857287 into getodk:master Jul 25, 2024
6 checks passed
@grzesiek2010 grzesiek2010 self-assigned this Jul 25, 2024
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.

5 participants