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

Fixed handling repeatable groups wrapped with regular ones #6105

Merged
merged 5 commits into from
May 25, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Apr 22, 2024

Closes #6015

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

We had at least two issues with this case when a repeatable group was wrapped with a regular one:

  1. The fact that the regular group in the hierarchy was visible as a removable one. I've described it in Crash in the Nested repeats for audit form while removing an empty inner repeat #6015 (comment)
  2. A wrong path was displayed in the hierarchy when navigating up.

I think I've fixed both issues using the appropriate form index.

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 pr is not big but I find it risky. We need to test the hierarchy view with nested groups (repeatable and regular ones) to make sure they are displayed in the correct way. @dbemke @srujner @WKobus please be creative and create your own forms ideally with multiple levels of nested groups.

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

The form I've attached in this comment: #6015 (comment)

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
  • 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 April 22, 2024 20:55
@seadowg
Copy link
Member

seadowg commented May 13, 2024

Given that @grzesiek2010 has flagged this as risky, I'll wait to merge until after testing.

@dbemke
Copy link

dbemke commented May 16, 2024

There is a difference between the way group/ repeats are shown in
3231nested-repeats-for-audit.xml.txt.
Which one is the expected one?
The light mode is the PR version, the dark mode is the master version.
A different icon:
iconsRepeats

Difference in the way outer > inner is shown:
2023vrPR

@grzesiek2010
Copy link
Member Author

To me the PR version works correctly and what you see on the master branch and older versions was wrong, for example the duplicate section Inner repeat 2 in the path (the second example) or the Inner repeat 2 > 1 displayed where you are on the outer (regular group) level (the first example).

@dbemke
Copy link

dbemke commented May 24, 2024

Tested with Success!

Verified on a device with Android 10

Verified Cases:

@WKobus
Copy link

WKobus commented May 24, 2024

Tested with Success!

Verified on a device with Android 14

@grzesiek2010 grzesiek2010 merged commit fcbddb6 into getodk:master May 25, 2024
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.

Crash in the Nested repeats for audit form while removing an empty inner repeat
4 participants