Menu buttons for FormHierarchy #2763

cooperka · 2018-12-17T16:11:07Z

Implements menu buttons discussed in getodk/roadmap#19

Add repeat child (visible on the "repeat picker" screen)
Delete repeat child (visible when viewing a repeat child)
Go up (visible on any screen other than the beginning)

Add/delete functionality already existed (swipe forward to add; long-press on question text to delete) -- this change just makes it more accessible.

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).

Navigating in and out of nested repeat groups, adding children, deleting 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?

Adding these buttons to the menu in FormHierarchy seems to make the most sense because that's where you get an overview of the form. FormHierarchy allows you to view children of repeat groups, and now on the same screen you can add new children. You can also delete the child currently being viewed.

Other approaches could involve showing menus elsewhere in the app, such as FormEntry, but that seems less intuitive.

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 ability to add/delete repeat instances is now easier and more obvious. The new menu buttons should make repeat group navigation more efficient. Users may not expect to see the new buttons, but no old behavior has been changed.

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.

Not that I can find. This page has screenshots of FormHierarchy but no repeat groups; the new buttons would not be visible in any of the screenshots.

Known issues being addressed

"Go up" button displays on the first screen in some forms
Deleting the last indexed (bottom) item in a repeat group causes a jump all the way to the first page in some forms
Repeats with jr:count shouldn't have buttons to add/delete
Deleting the last remaining item in a repeat group causes a navigation into the preceding repeat group, if there are 2 in a row, rather than navigating "up" in the hierarchy

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

zestyping

Hi—I have a few questions about this. Some of it is due to not having a deep understanding of how repeat groups are structured and how the FormController navigates around the form—sorry!—though I think it wouldn't hurt to spell this out a little more in comments, to help others who aren't intimately familiar with this avoid breaking it by accident; the logic looks quite intricate. I hope my comments are helpful!

In the first screenshot you posted, the individual repeated instances are shown as "Alice (1)" and "Bob (2)", and in the second, the repeated group title is "Friends (1)". Which bits of code are responsible for formatting these labels? I think these labels are a bit confusing as is (see also my comments on #2762). In particular, since "Alice (1)" and "Friends (1)" look so alike, the label doesn't make it clear whether the name identifies a repeat group title or an instance of a repeated group; I think the label should use distinct formatting to make this obvious.

If we feel good about going with my suggestion in #2762, these labels should be formatted similarly, e.g. "1. Alice", "2. Bob", and "Friends > 1".

zestyping · 2019-01-05T02:38:44Z

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

@@ -212,8 +212,14 @@ private void updateOptionsMenu() {
        // Not ready yet. Menu will be updated automatically once it's been prepared.
        if (optionsMenu == null) return;

+        FormController formController = Collect.getInstance().getFormController();
+        boolean isAtBeginning = formController.isCurrentQuestionFirstInForm() && !shouldShowRepeatGroupPicker();


What's the condition that this is trying to express? I'm puzzled that this is defined with the exclusion !shouldShowRepeatGroupPicker() and then is only used once below, in an expression that then includes shouldShowPicker, making said exclusion moot.

Are you intending for the Up button to be visible only for the second and subsequent repeated items in a repeat group? If so, why?

I might have misunderstood how isCurrentQuestionFirstInForm() works for repeat groups, so maybe it would help if you describe the intended condition in prose first, thanks!

The logic here is cleaned up quite a bit after the commit you're commenting on, and the latest code hopefully makes more sense.

In short, you're only at the top/beginning of the hierarchy if you're both at the first question and not showing the repeat picker. The "up" button should be displayed on every screen of the hierarchy except at the beginning, because then there's nothing to go "up" to.

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

collect_app/src/main/java/org/odk/collect/android/utilities/DialogUtils.java

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

zestyping · 2019-01-09T00:07:20Z

collect_app/src/main/java/org/odk/collect/android/utilities/DialogUtils.java

+        String name = formController.getLastRepeatedGroupName();
+        int repeatcount = formController.getLastRepeatedGroupRepeatCount();
+        if (repeatcount != -1) {
+            name += " (" + (repeatcount + 1) + ")";


In the example where the repeat section is "Friends" and each instance of the repeated group is a friend, what does this show? Suppose there are a total of three friends (Alice, Bob, Charlie) and the currently shown friend is Bob. Will it show "Friends (3)" because there are three friends, or "Friends (2)" because Bob is #2, or "Bob (2)"?

Friends (2):

But this is old logic, and I think it should be improved as you described in #2762 (comment). There are probably numerous other places in the code where this type of string is built... we shoud do a quick audit if we want to update it.

cooperka · 2019-01-10T17:08:44Z

@zestyping I believe I've addressed all comments now.

I pushed 4 new commits at d73ad5b, and I'm now rebasing on master since it's been a while.

mmarciniak90 · 2019-01-15T10:21:41Z

@cooperka I noticed that buttons are not visible in the view g1 (1) but are visible for view g1 (2) in the form fewRepeatGroups.xml.txt

Similar situation for form:
nested-indexed-repeat.xml.txt

@cooperka please take look on this
@opendatakit-bot unlabel "needs testing"

cooperka · 2019-01-16T20:32:41Z

Thanks for the info @mmarciniak90 -- I fixed it. Previously it would think it was at the very beginning when it was at the first question inside of the first group.

@opendatakit-bot label "needs testing"

zestyping · 2019-01-16T21:39:55Z

Thanks so much for all the responses and making these edits, @cooperka! I'm sorry I didn't respond right away—I've just discovered that my Github notifications are misconfigured, so I'm fixing that and intend to be more responsive in the future.

mmarciniak90 · 2019-01-17T11:09:32Z

@cooperka
I checked form: nested-indexed-repeat.xml.txt

If user does not add any repeat group, Go Up button is visible and disappears when is clicked

If repeat group is added but user opens jump screen from question outside the repeat group, Go Up button is visible and disappears when is clicked

The same situation is for nested-repeats-complex form.

@opendatakit-bot unlabel "needs testing"

grzesiek2010 · 2019-01-21T21:31:50Z

@cooperka once you fix the bug pointed out by @mmarciniak90 please fix conflicts as well.

cooperka · 2019-01-22T04:50:38Z

@mmarciniak90 that bug should be resolved now. And @grzesiek2010 it's been rebased.

There are a lot of tricky edge cases here; I tested in as many forms as I could think of and it seems to finally be working well. One remaining issue is that if there are 2 repeat groups in a row, and you delete the last remaining item from the second repeat, it will send you to the last index of the first repeat instead of going back a level as expected, i.e. if the structure is:

Top-level
  Sub-group
    Friends
      A
      B
    Enemies
      C

When viewing Enemies > C -> Click delete -> Sends you to Friends > B when it should have sent you to Sub-group.

This is a narrow edge case, and it's not even that much of a usability issue, but @grzesiek2010 I'd appreciate your help addressing this if you think this is a blocker to merge.

@opendatakit-bot label "needs testing"

kkrawczyk123 · 2019-01-22T15:17:29Z

@cooperka, It looks like issues pointed by @mmarciniak90 have been fixed which is great! I have used the same forms as she did and I have noticed a crash when I tried to add a new nested group from the hierarchy view when used nested-indexed-repeat.xml form. I am attaching gif to help you with reproducing:

(yes I clicked on + button on the last displayed screen as it is nearly visible on the gif xd )

@opendatakit-bot unlabel "needs testing"

cooperka · 2019-01-22T15:52:56Z

I've fixed the crash itself but this case will need to be handled differently. Repeats with jr:count shouldn't have buttons to add/delete.

@grzesiek2010 do you know how we might determine if a repeat has an explicit count? I tried using getCaptionPrompt().getFormElement().getAdditionalAttribute("jr", "count") (and variations) but it always returns null... I'll post in Slack.

cooperka · 2019-01-22T19:46:34Z

@kkrawczyk123 forms that use specific numbers of repeat items are now supported. The add/delete buttons should not be visible in these types of repeat groups. Can you please check again?

@opendatakit-bot label "needs testing"

kkrawczyk123 · 2019-01-23T09:21:58Z

@cooperka I am not sure if I fully understand the approach that you followed. I can see some sense in disabling deleting within the nested group when we are giving the number of the repeats by ourselves according to getodk/javarosa#61 but if it's about adding the new items of the nested group I think that should be allowed and I don't understand why you disabled it. I am able to add the group when I am back to the form anyway. cc @yanokwa @grzesiek2010

grzesiek2010 · 2019-01-23T09:24:23Z

I agree. The delete option is redundant in case of repeat groups with repeat-count number but adding new groups should be possible since we can do that on the form level.

cooperka · 2019-01-23T14:15:03Z

I don't follow -- if, in the nested-indexed-repeat form, friend A has declared they have exactly 2 pets, are you suggesting you should be able to add a third? This seems to be disallowed, how are you able to do so? This is what I see when I'm in the form:

How many pets does A have?
1. Answer 2
2. Swipe to next question
What is pet updating autoplay to play select choices. adding text highlighting #1's name?
1. Swipe to next question
What is pet Feature/google sheets #2's name?
1. Swipe to next question
Add another "" group? [friend, not pet]

You can not add new pets, so I hid the "add" button for pets. There's still an "add" button for friends.

grzesiek2010 · 2019-01-23T14:52:03Z

Ahhh sorry I misunderstood if we use repeat-count we can add more groups only editing that value there is no prompt to add a new group at the end so the approach is ok @kkrawczyk123 please continue testing.

grzesiek2010 · 2019-01-23T15:04:40Z

@cooperka please resolve conflicts

It's confusing to show it then

For consistency with the 'jump' button in FormEntry.

Avoid setting it to null, simplify logic.

Previously would think it was at the beginning when it was at the first question inside of the first group.

Otherwise the index could be changed when you visit a question then go back to the hierarchy, showing the arrow on the first screen when it shouldn't.

Previously, deleting the last indexed item in the list (i.e. bottom one of multiple) wouldn't trigger the repeat picker on some forms. But now the last remaining item (i.e. only one left) will leave the user on a blank screen. To be fixed next.

Now that it's possible to see the repeat picker in this state, due to the last commit.

cooperka · 2019-01-23T16:34:25Z

@grzesiek2010 rebased and ready 👍

yanokwa added this to the v1.19 milestone Jan 8, 2019

zestyping reviewed Jan 9, 2019

View reviewed changes

cooperka mentioned this pull request Jan 9, 2019

Clean up form 'breadcrumb' paths #2762

Merged

3 tasks

cooperka force-pushed the form-hierarchy-buttons branch from d73ad5b to 94fe7fd Compare January 10, 2019 17:08

yanokwa added the needs testing label Jan 15, 2019

getodk-bot removed the needs testing label Jan 15, 2019

getodk-bot added the needs testing label Jan 16, 2019

cooperka mentioned this pull request Jan 17, 2019

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

Open

getodk-bot removed the needs testing label Jan 17, 2019

cooperka force-pushed the form-hierarchy-buttons branch 2 times, most recently from 56ce3fb to ab6f02e Compare January 22, 2019 04:35

getodk-bot added the needs testing label Jan 22, 2019

getodk-bot removed the needs testing label Jan 22, 2019

getodk-bot added the needs testing label Jan 22, 2019

cooperka force-pushed the form-hierarchy-buttons branch 2 times, most recently from b3d1fa4 to 1e8ec72 Compare January 22, 2019 23:04

getodk-bot added the behavior verified label Jan 23, 2019

cooperka added 23 commits January 23, 2019 11:12

Create stub options menu for FormHierarchy

1bd5b54

Add 'add' button

986f1ab

Add 'go up' button

e494465

Factor out static createDeleteRepeatConfirmDialog util

0c82a13

Add 'delete' button

57d2b3c

Use ?iconColor for new icon fills

f3a3713

Never show add/delete buttons on ViewOnlyFormHierarchy

8e6ba71

Don't show delete button while picker is active

25a6c43

It's confusing to show it then

Make sure 'go up' icon is always second from the edge

7dcbd7d

For consistency with the 'jump' button in FormEntry.

Make 'up' arrow thicker like the 'jump' arrow

4b65181

Clean up form_hierarchy_menu formatting

02c1647

Fix a few PMD rule violations

dc963a4

Fix unintentionally overloaded method name

6951d48

PR fix: Clean up menu button toggling logic

c48fbde

PR fix: Clarify getRepeatPromptIndex logic

2b93931

PR fix: Rename showDeleteRepeatConfirmDialog

c6cb175

PR fix: Clean up screenIndex == null logic

6b6fd8d

Avoid setting it to null, simplify logic.

PR fix: More accurately detect beginning of form

4b77e0a

Previously would think it was at the beginning when it was at the first question inside of the first group.

Use screenIndex instead of currIndex to detect beginning

f9da285

Otherwise the index could be changed when you visit a question then go back to the hierarchy, showing the arrow on the first screen when it shouldn't.

Handle deleting last remaining repeat group item

7955b3d

Now that it's possible to see the repeat picker in this state, due to the last commit.

Fix crash when trying to get ref to end of form

35cd0b1

Check isGroupSizeLocked before showing add/delete buttons

2182652

cooperka force-pushed the form-hierarchy-buttons branch from 1e8ec72 to 2182652 Compare January 23, 2019 16:33

yanokwa approved these changes Jan 23, 2019

View reviewed changes

yanokwa merged commit e74548b into getodk:master Jan 23, 2019

cooperka deleted the form-hierarchy-buttons branch January 23, 2019 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu buttons for FormHierarchy #2763

Menu buttons for FormHierarchy #2763

cooperka commented Dec 17, 2018 •

edited

Loading

zestyping left a comment •

edited

Loading

zestyping Jan 5, 2019

cooperka Jan 9, 2019 •

edited

Loading

zestyping Jan 9, 2019

cooperka Jan 9, 2019

cooperka commented Jan 10, 2019

mmarciniak90 commented Jan 15, 2019

cooperka commented Jan 16, 2019

zestyping commented Jan 16, 2019

mmarciniak90 commented Jan 17, 2019

grzesiek2010 commented Jan 21, 2019

cooperka commented Jan 22, 2019

kkrawczyk123 commented Jan 22, 2019

cooperka commented Jan 22, 2019 •

edited

Loading

cooperka commented Jan 22, 2019

kkrawczyk123 commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

cooperka commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

cooperka commented Jan 23, 2019

Menu buttons for FormHierarchy #2763

Menu buttons for FormHierarchy #2763

Conversation

cooperka commented Dec 17, 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.

Known issues being addressed

Before submitting this PR, please make sure you have:

zestyping left a comment • edited Loading

Choose a reason for hiding this comment

zestyping Jan 5, 2019

Choose a reason for hiding this comment

cooperka Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

zestyping Jan 9, 2019

Choose a reason for hiding this comment

cooperka Jan 9, 2019

Choose a reason for hiding this comment

cooperka commented Jan 10, 2019

mmarciniak90 commented Jan 15, 2019

cooperka commented Jan 16, 2019

zestyping commented Jan 16, 2019

mmarciniak90 commented Jan 17, 2019

grzesiek2010 commented Jan 21, 2019

cooperka commented Jan 22, 2019

kkrawczyk123 commented Jan 22, 2019

cooperka commented Jan 22, 2019 • edited Loading

cooperka commented Jan 22, 2019

kkrawczyk123 commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

cooperka commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

grzesiek2010 commented Jan 23, 2019

cooperka commented Jan 23, 2019

cooperka commented Dec 17, 2018 •

edited

Loading

zestyping left a comment •

edited

Loading

cooperka Jan 9, 2019 •

edited

Loading

cooperka commented Jan 22, 2019 •

edited

Loading