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

Fix instances count on main menu #6535

Merged
merged 15 commits into from
Feb 7, 2025
Merged

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Nov 28, 2024

Closes #6531

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

The biggest change I had to make here was to modify InstancesDataServices so that the instances counts were qualified by project. As part of this, I ended up introducing a more formalized way to create/work with data services in the form of a DataService abstract class (in Data.kt).

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?

The changes made are pretty widespread, so I think a general check of downloading, updating, saving and sending forms would be good here.

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 seadowg force-pushed the instances-count branch 3 times, most recently from 2620b99 to 4c0bce3 Compare December 5, 2024 11:33
@seadowg seadowg marked this pull request as ready for review January 21, 2025 14:16
@seadowg seadowg requested a review from grzesiek2010 January 21, 2025 14:16
private val currentProjectViewModel = CurrentProjectViewModel(
projectsDataService
)
private val currentProjectViewModel by lazy { CurrentProjectViewModel(projectsDataService) }
Copy link
Member

Choose a reason for hiding this comment

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

Does it make any sense to use by lazy in tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't construct a CurrentProjectViewModel until after the JUnit rules have run (so that InstantTaskExecutorRule has done its job). I agree that the lazy is a bit weird though, so I'll just replace it with construction in the tests themselves.

return DataDelegate(data)
}

protected fun <T> data(key: String, default: T, updater: () -> T): DataDelegate<T> {
Copy link
Member

Choose a reason for hiding this comment

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

It's used just in one place I think we can get rid of it and use qualifiedData instead. Then we could also remove attachData and move the code directly to qualifiedData.

Copy link
Member Author

@seadowg seadowg Feb 7, 2025

Choose a reason for hiding this comment

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

Right this is a little weird. The reason for the two is so that the updater type can be more accurate: for data we don't want there to be a qualifier passed to it. This kind of falls apart though because you could still then pass a qualifier to Data#flow. It feels like it's half done. I'll have a quick go at making the types stricter throughout.

@seadowg seadowg requested a review from grzesiek2010 February 7, 2025 11:55
@grzesiek2010 grzesiek2010 merged commit 66f0967 into getodk:master Feb 7, 2025
6 checks passed
@seadowg seadowg deleted the instances-count branch February 7, 2025 13:41
@seadowg seadowg added the high priority Should be looked at before other PRs/issues label Feb 7, 2025
@dbemke
Copy link

dbemke commented Feb 10, 2025

Tested with Success!

Verified on a device with Android 10

Verified Cases:

  • issue # 6531 doesn't occur
  • regression checks in saving as draft,finalizing, sending form
  • regression checks in form new version, downloading/updating media

@WKobus
Copy link

WKobus commented Feb 10, 2025

Tested with Success!

Verified on a device with Android 15 and Android 12 by Szymon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
4 participants