"Repeat group picker" UI for FormHierarchy #2740

cooperka · 2018-11-21T16:14:26Z

Implements the core behavior described in getodk/roadmap#19. The new buttons will be added in a separate upcoming PR.

Based on branch #2730.

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

Extensive manual testing with:

nested-repeats-complex (XML / XLS) (top-level "friends" and "enemies" groups; nested "friends/pets" group)
nested-repeats-complex-jr (XML) (same as above, but "enemies" has no jr-template tag, so it shows a blank child by default)

Navigating in and out of nested repeat groups, adding children, editing names, using the back button, jumping back and forth between the FormEditor and FormHierarchy views.

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

We considered overhauling FormHierarchyActivity to navigate using a more conventional "back stack" instead of using refreshView, but that requires all method calls to be stateless to avoid corrupting the state of earlier activities, which FormController currently doesn't support. This simpler solution works within the existing event-based form parsing framework.

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?

This changes the navigational UI so that instead of expanding a dropdown showing the children of a repeatable group, those children are shown on their own on an intermediary screen (sometimes referred to as the "picker"). This intermediary screen will be used in an upcoming PR to display an "add" button to add new items to the repeat group. Overall these changes aim to improve the speed and intuitiveness of navigation.

The behavior changes should be isolated to FormHierarchyActivity, but this activity interacts with the global state via FormController, so regressions can be avoided by ensuring that the state is not mutated in some new unexpected way.

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

Linked above.

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

I was unable to find any existing documentation that needs to be changed (namely here which has screenshots of FormHierarchy but no expandable repeat groups).

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

This doesn't yet render anything new.

This ensures a consistent form reference. For example, without this change, if the user is on a question then opens the jump screen, the index will be of the question and not of the repeat group.

Currently no text to display, but at least render the UI.

Remove unused imports; add braces; remove unused drawables.

yanokwa · 2018-12-04T01:30:18Z

@zestyping I'd appreciate a careful review of this PR if you have a moment!

zestyping · 2018-12-04T19:16:42Z

@yanokwa Thanks for looping me in! I'm in Dar Es Salaam wrangling map data for a hospital at the moment, so I might not get to this before the call on Friday. I can try to have a look at it on the weekend, if that's not too late.

grzesiek2010 · 2018-12-06T13:25:09Z

I know keeping arrow icons doesn't make sense now as this pr removes expanding/collapsing but maybe we should add another icon to indicate it's a group we can open. This pr adds just a text Repeatable Group but to be honest such a group looks exactly the same like normal question (it could be a text question with Repeatable Group as an answer)

Before	This branch

what do you think?

Maybe one from those:
https://material.io/tools/icons/?icon=repeat&style=baseline
https://material.io/tools/icons/?icon=group_work&style=baseline
https://material.io/tools/icons/?icon=more&style=baseline
https://material.io/tools/icons/?icon=group&style=baseline

cooperka · 2018-12-06T14:16:08Z

@grzesiek2010 I completely agree, and we actually have that in mind for an upcoming PR. I didn't want to put too many separate changes in one PR but we do want to add icons to each type of question to visually represent the question/item type (e.g. repeatable group, date, text, image, etc.). Would you be okay with holding off on that for now?

collect_app/src/main/java/org/odk/collect/android/logic/HierarchyElement.java

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java

grzesiek2010 · 2018-12-07T14:03:43Z

Would you be okay with holding off on that for now?

I would prefer having that one with this pr. I don't know how long it will take to review/accept and merge another pr. We can release a new version in the middle so it would be better to add a group icon in this pr. It's not a big deal to do that so if you can please do.

yanokwa · 2018-12-10T01:58:21Z

I think keeping the text is a good idea. More information about what a thing is is helpful.

~~The breadcrumb feels a little weird because you click on a thing I don't see that thing in the header.~~

~~current behavior~~
~~screenshot~~

~~desired behavior~~
~~screenshot~~

~~Is that easy to add to the PR? If so, great. If not, I can file an issue to follow up with it.~~

Update: Looks like you've already thought of that at cooperka#4

Finally, do you have the XLS that generated the XML, might be helpful for the testers.

cooperka · 2018-12-10T03:36:33Z

Good eye, yes the breadcrumbs will be significantly cleaned up in that upcoming PR. I also just updated the description to include the XLS.

grzesiek2010 · 2018-12-10T10:29:19Z

@cooperka thanks!

It looks good to me. @zestyping please take a look when you are free.

grzesiek2010 · 2018-12-11T13:21:17Z

@mmarciniak90 @kkrawczyk123
whoever is going to test this pr please remember that we use the edited hierarchy view in two places:

filling new forms
reviewing sent forms

Everything should work as before apart from repeatable groups which will be displayed in a new way.
Please test form with many questions/groups to be sure the list is scrolled to an appropriate place once you open it.
And also repeatable groups that contain ordinary groups inside.

lognaturel · 2018-12-11T22:46:43Z

I'll admit I'm a bit disappointed to see the stateful approach remain but I also agree that this is a pragmatic approach. The commit history is really clean. I've only done a quick read through but for what it's worth, I don't have any concerns.

cooperka · 2018-12-12T02:35:53Z

I agree @lognaturel, I still hope we can get there eventually. The alternative stateless/back-stack approach is a much bigger project than we originally anticipated 😞 It seems to me like something that needs up-front commitment to see it through all the way (one piece at a time) if it's going to be attempted, otherwise we'll create the n+1 problem for form navigation. Hopefully we or someone else will be able to get our hands dirty in the near future.

kkrawczyk123 · 2018-12-13T13:09:19Z

@cooperka, I really like the new hierarchy view as it looks very pretty but I have noticed some issues:

When I open the general hierarchy view and open one of groups Friends/Enemies and rotate the screen I am thrown back to the general hierarchy view and I have to open Friends/Enemies group one again. The same situation occurs with the nested group: when I rotate the screen after opening Pets group I am thrown to the screen where I can open Pets group again.
When I add more group members then fit to the device's screen (No matter if Friends/Enemies/Pets) the scroll initially shows in the middle of the displayed page like on attached screenshot:

and I have to scroll up to see the page from the first group.

@opendatakit-bot unlabel "needs testing"

cooperka · 2018-12-13T14:10:36Z

Thank you @kkrawczyk123! I'll address those and get back to you.

Manually restoreInstanceState because onRestoreInstanceState happens too late after onCreate.

This logic was obsolete; probably related to old expand/collapse behavior. setAdapter is also already called by refreshView, so no need for it here. Standardize on 'break' instead of 'return' for consistency.

cooperka · 2018-12-13T19:18:28Z

@kkrawczyk123 can you try again please? I just made a couple fixes (tested and verified locally).

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java

kkrawczyk123 · 2018-12-17T14:01:34Z

Tested with success!!
Confirmed that issues have been fixed.
Verified on Androids: 4.1, 4.2, 4.4, 5.1, 6.0, 7.0 ad 8.1.

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

yanokwa · 2018-12-17T14:29:56Z

@cooperka Looking forward to the other PRs that build on this one!

cooperka force-pushed the form-hierarchy-picker branch 2 times, most recently from 0510030 to b3aa5dc Compare November 21, 2018 17:13

cooperka added 18 commits November 30, 2018 11:08

Factor out jumpToHierarchyStartIndex

3c2d4ca

Call refreshView instead of manual expand/collapse

cae805a

This doesn't yet render anything new.

Set repeatGroupPickerRef in order to render 'picker' view

c3034ad

Handle 'go up' when in the picker view

bea5530

Clarify currentIndex docs

edd3ef7

Always step out to the closest screen event

b08e644

This ensures a consistent form reference. For example, without this change, if the user is on a question then opens the jump screen, the index will be of the question and not of the repeat group.

Toggle the picker if coming from an inner repeat group

b7174e0

Also show groupPath for the root picker

a3b8e3f

Currently no text to display, but at least render the UI.

Safe repeatGroupPickerIndex instead of repeatGroupPickerRef

a434aa3

Show repeat picker 'label' in groupPath

07a0f29

Compare unindexed ref to support nested repeat groups

9524be3

Replace dropdown icon with 'Repeatable Group' label

20427ee

Remove now-obsolete code from HierarchyElement

b7a4391

Factor out shouldShowRepeatGroupPicker()

f29259e

Add clarifying comments in goUpLevel()

1585e20

Fix checks

c3b6646

Remove unused imports; add braces; remove unused drawables.

Use 'instance' instead of 'child' when possible

f69b591

Clean up old comments

77ac22d

cooperka force-pushed the form-hierarchy-picker branch from 8480bb0 to 77ac22d Compare November 30, 2018 16:08

lognaturel mentioned this pull request Dec 6, 2018

BYO nginx getodk/central#17

Closed

grzesiek2010 reviewed Dec 7, 2018

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/logic/HierarchyElement.java Outdated Show resolved Hide resolved

grzesiek2010 reviewed Dec 7, 2018

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java Outdated Show resolved Hide resolved

grzesiek2010 reviewed Dec 7, 2018

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java Outdated Show resolved Hide resolved

Add repeat icon to repeatable groups in hierarchy

3683821

yanokwa added the needs testing label Dec 11, 2018

yanokwa approved these changes Dec 12, 2018

View reviewed changes

grzesiek2010 approved these changes Dec 12, 2018

View reviewed changes

grzesiek2010 mentioned this pull request Dec 13, 2018

[WIP] Form hierarchy redesign #1271

Closed

5 tasks

getodk-bot removed the needs testing label Dec 13, 2018

cooperka added 2 commits December 13, 2018 13:54

Remember repeatGroupPickerIndex on screen rotation

ac693e6

Manually restoreInstanceState because onRestoreInstanceState happens too late after onCreate.

Don't try to scroll anywhere when entering/exiting the picker

8412441

This logic was obsolete; probably related to old expand/collapse behavior. setAdapter is also already called by refreshView, so no need for it here. Standardize on 'break' instead of 'return' for consistency.

grzesiek2010 reviewed Dec 13, 2018

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java Outdated Show resolved Hide resolved

grzesiek2010 reviewed Dec 13, 2018

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java Outdated Show resolved Hide resolved

Fix restoreInstanceState protected => private

a728d13

grzesiek2010 added the needs testing label Dec 14, 2018

Reverse restoreInstanceState condition

1bb55e9

getodk-bot added behavior verified and removed needs testing labels Dec 17, 2018

yanokwa merged commit 43a3ba1 into getodk:master Dec 17, 2018

cooperka mentioned this pull request Jan 17, 2019

[Demo] All combined APK for TCC cooperka/collect#6

Open

shobhitagarwal1612 mentioned this pull request Feb 7, 2019

Collect: More efficient repeat group navigation getodk/roadmap#19

Closed

lognaturel mentioned this pull request Aug 30, 2019

Repeat group UI redesign #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Repeat group picker" UI for FormHierarchy #2740

"Repeat group picker" UI for FormHierarchy #2740

cooperka commented Nov 21, 2018 •

edited

Loading

yanokwa commented Dec 4, 2018

zestyping commented Dec 4, 2018

grzesiek2010 commented Dec 6, 2018 •

edited

Loading

cooperka commented Dec 6, 2018 •

edited

Loading

grzesiek2010 commented Dec 7, 2018

yanokwa commented Dec 10, 2018 •

edited

Loading

cooperka commented Dec 10, 2018

grzesiek2010 commented Dec 10, 2018

grzesiek2010 commented Dec 11, 2018 •

edited

Loading

lognaturel commented Dec 11, 2018

cooperka commented Dec 12, 2018

kkrawczyk123 commented Dec 13, 2018

cooperka commented Dec 13, 2018

cooperka commented Dec 13, 2018

kkrawczyk123 commented Dec 17, 2018

yanokwa commented Dec 17, 2018

"Repeat group picker" UI for FormHierarchy #2740

"Repeat group picker" UI for FormHierarchy #2740

Conversation

cooperka commented Nov 21, 2018 • edited Loading

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

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:

yanokwa commented Dec 4, 2018

zestyping commented Dec 4, 2018

grzesiek2010 commented Dec 6, 2018 • edited Loading

cooperka commented Dec 6, 2018 • edited Loading

grzesiek2010 commented Dec 7, 2018

yanokwa commented Dec 10, 2018 • edited Loading

cooperka commented Dec 10, 2018

grzesiek2010 commented Dec 10, 2018

grzesiek2010 commented Dec 11, 2018 • edited Loading

lognaturel commented Dec 11, 2018

cooperka commented Dec 12, 2018

kkrawczyk123 commented Dec 13, 2018

cooperka commented Dec 13, 2018

cooperka commented Dec 13, 2018

kkrawczyk123 commented Dec 17, 2018

yanokwa commented Dec 17, 2018

cooperka commented Nov 21, 2018 •

edited

Loading

grzesiek2010 commented Dec 6, 2018 •

edited

Loading

cooperka commented Dec 6, 2018 •

edited

Loading

yanokwa commented Dec 10, 2018 •

edited

Loading

grzesiek2010 commented Dec 11, 2018 •

edited

Loading