Convert hierarchy to Fragment #6516

seadowg · 2024-11-18T15:52:07Z

Work towards #5420
Closes #6303

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

I've tried to keep changes to a minimal here with the main goal just being to get the majority of the hierarchy code into a Fragment. There's only two structural changes to highlight really:

I ended up rewriting the hierarchy's options menu so that it uses the new MenuProvider API. I felt like this made more sense than rewriting it to use the deprecated Fragment methods.
The new Fragment uses a new FormHierarchyViewModel to manage state. This will most likely get merged into FormEntryViewModel, but I think that'll make more sense once form entry has also been moved to a Fragment.

I also added some new tests for code that I found had none.

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 shouldn't change any behaviour and the big risk here is that it either does or introduces bugs to flows using the form hierarchy. A general test of using the hierarchy (during form entry and viewing) would be good here.

This also changes the jump/summary icon to the new one!

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 · 2024-11-22T16:17:43Z

@getodk/testers marking this ready for testing before review as it'd be good to get a first pass before I clean things up in case there are some fundamental problems here I've missed.

dbemke · 2024-12-16T10:36:35Z

There used to be 2 icons: to go to the hierarchy view and to go "up" in the hierarchy (the second screenshot). Now there's only one icon and it feels a bit confusing in groups and repeats cause the new icon doesn't suggest where the user is moved after tapping it. Is it expected?

seadowg · 2024-12-16T11:33:21Z

There used to be 2 icons: to go to the hierarchy view and to go "up" in the hierarchy (the second screenshot). Now there's only one icon and it feels a bit confusing in groups and repeats cause the new icon doesn't suggest where the user is moved after tapping it. Is it expected?

I think this was the intention of #6303, but please correct me if I'm wrong @alyblenkin.

alyblenkin · 2024-12-16T16:34:25Z

I think this was the intention of #6303, but please correct me if I'm wrong @alyblenkin.

This is a good catch! The goal was just to update the east arrow icon, not the up icon as well. I think the up arrow is still a helpful visual indicator that you're going back up to the table of contents.

seadowg · 2024-12-16T16:47:35Z

This is a good catch! The goal was just to update the east arrow icon, not the up icon as well. I think the up arrow is still a helpful visual indicator that you're going back up to the table of contents.

That makes sense. Just to be sure though before I unwind anything, the Figma link shows the hierarchy with the list icon in the top right:

I'm guessing that's a mistake?

alyblenkin · 2024-12-16T17:05:51Z

Sorry I think the group folder icon in the first mockup is a mistake. However, now I'm second guessing myself because we discussed this a while back. @lognaturel correct me if I'm wrong here, but our goal was just update the east arrow, not the up arrow icon as well.

alyblenkin · 2024-12-16T17:09:22Z

To clarify, I think the list icon would work on it's own if we didn't already have the up arrow, but I think it's a pretty ingrained pattern now. I think the list icon would suggest you go back to the top level given the current pattern where there is more of this concept of moving up or down, but you could also imagine tapping to keep going to the main table of contents. I don't think this is a stronger design choice, rather it could be another option. Not worth considering given we haven't had folks bring up any issues.

lognaturel · 2024-12-16T17:20:16Z

Feels right to me to leave the up arrow as it is. I think navigating up a level feels relatively clear. I don't think the table of contents icon works well there because it would suggest you go all the way back to the top level.

alyblenkin · 2024-12-16T18:59:02Z

@seadowg - sorry that you have to unwind things there! I've update the design.

seadowg · 2024-12-17T10:01:44Z

sorry that you have to unwind things there! I've update the design.

No worries! I just wanted to double check.

seadowg · 2024-12-17T15:22:42Z

@getodk/testers that should be good to go again!

dbemke · 2024-12-20T13:37:21Z

Tested with success!

Verified on devices with Android 10, 14

Verified cases:

the new icon in the hierarchy
groups and repeats
the hierarchy view in new forms, drafts, Ready to send, Sent
the hierarchy view after recovering a savepoint
moving backwards enabled and disabled
RTL
light and dark mode
don't keep activities enabled and disabled

...ct_app/src/androidTest/java/org/odk/collect/android/feature/formentry/FormHierarchyTest.java

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

collect_app/src/main/res/layout/form_hierarchy_layout.xml

collect_app/src/main/AndroidManifest.xml

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

grzesiek2010 · 2025-01-07T11:57:18Z

icons/src/main/res/drawable/ic_baseline_list_24.xml

+    android:viewportWidth="24"
+    android:viewportHeight="24">
+    <path
+        android:fillColor="?colorSurface"


Why not colorOnSurface?

I'm pretty sure they're flipped from what you'd think here: the tint is colorOnSurface and the path is colorSurface. Everything looks correct right?

But why not to use colorOnSurface in both as we do in every other icon? What's the point of using colorSurface here?

Good question! It looks like we just don't need tint. I'd always get confused by this, but the tint color is "blended" in with the others so here the path was being rendered with colorSurface but then it would just be "blended" (just overridden in this case) with colorOnSurface. If I remove tint and set the path to colorOnSurface it seems to work as expected.

If I remove tint and set the path to colorOnSurface it seems to work as expected.

Yes, but when you change themes it might not work as expected see this pr #5537
Maybe now as removed changing the theme in the app it is no longer needed but I'm not sure. If you want to remove it please make sure it is not needed or maybe a better idea is to keep it and file an issue to investigate if we still need it.

..._app/src/main/java/org/odk/collect/android/formentry/repeats/DeleteRepeatDialogFragment.java

collect_app/src/main/java/org/odk/collect/android/formhierarchy/FormHierarchyFragment.java

grzesiek2010

#6516 (comment)

seadowg force-pushed the view-only branch 2 times, most recently from c0b1039 to a44178c Compare November 21, 2024 11:46

seadowg added the needs testing label Nov 22, 2024

seadowg added 9 commits December 11, 2024 11:25

Add tests for view only behaviour

a60d775

Remove ViewOnlyHierarchyActivity

c6754ea

Spike out empty form hierarchy Fragment

76040da

Create fragemnt for hierarchy

94785c9

Update test Activity references

e169db5

Make sure FormHiearchyActivity is still available for tests

4e4c01b

Move form hierarchy state to viewmodel

be59efb

Update test target

c8c10ed

Delete FormHierarchyActivity

3a9bc6f

seadowg force-pushed the view-only branch 2 times, most recently from a01cd5a to a2fa91f Compare December 11, 2024 12:06

Replace hierarchy icon

546336a

seadowg force-pushed the view-only branch from a2fa91f to 546336a Compare December 17, 2024 14:29

dbemke added behavior verified and removed needs testing labels Dec 20, 2024

seadowg marked this pull request as ready for review December 20, 2024 14:52

seadowg requested a review from grzesiek2010 December 20, 2024 14:52

grzesiek2010 requested changes Jan 7, 2025

View reviewed changes

seadowg added 7 commits January 8, 2025 08:59

Make test names consistent

0153f6b

Remove redundant code

156a1ce

Update comment references

5507ef7

Remove callback approach from DeleteRepeatDialogFragment

c1c459f

Extract constant for fragment request

5873c0e

Isolate elements to display logic

2324306

Rework DeleteRepeatDialogFragment tests

3e1de99

seadowg requested a review from grzesiek2010 January 8, 2025 10:47

grzesiek2010 requested changes Jan 8, 2025

View reviewed changes

Fix color declaration

9aec982

seadowg force-pushed the view-only branch from bebf8af to 9aec982 Compare January 8, 2025 16:02

seadowg requested a review from grzesiek2010 January 8, 2025 16:02

grzesiek2010 approved these changes Jan 8, 2025

View reviewed changes

grzesiek2010 merged commit 467c317 into getodk:master Jan 8, 2025
6 checks passed

seadowg deleted the view-only branch January 9, 2025 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert hierarchy to Fragment #6516

Convert hierarchy to Fragment #6516

seadowg commented Nov 18, 2024 •

edited

Loading

seadowg commented Nov 22, 2024

dbemke commented Dec 16, 2024

seadowg commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

seadowg commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

alyblenkin commented Dec 16, 2024 •

edited

Loading

lognaturel commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

seadowg commented Dec 17, 2024

seadowg commented Dec 17, 2024

dbemke commented Dec 20, 2024

grzesiek2010 Jan 7, 2025

seadowg Jan 8, 2025

grzesiek2010 Jan 8, 2025

seadowg Jan 8, 2025

grzesiek2010 Jan 8, 2025

grzesiek2010 left a comment

Convert hierarchy to Fragment #6516

Convert hierarchy to Fragment #6516

Conversation

seadowg commented Nov 18, 2024 • edited Loading

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?

Before submitting this PR, please make sure you have:

seadowg commented Nov 22, 2024

dbemke commented Dec 16, 2024

seadowg commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

seadowg commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

alyblenkin commented Dec 16, 2024 • edited Loading

lognaturel commented Dec 16, 2024

alyblenkin commented Dec 16, 2024

seadowg commented Dec 17, 2024

seadowg commented Dec 17, 2024

dbemke commented Dec 20, 2024

grzesiek2010 Jan 7, 2025

Choose a reason for hiding this comment

seadowg Jan 8, 2025

Choose a reason for hiding this comment

grzesiek2010 Jan 8, 2025

Choose a reason for hiding this comment

seadowg Jan 8, 2025

Choose a reason for hiding this comment

grzesiek2010 Jan 8, 2025

Choose a reason for hiding this comment

grzesiek2010 left a comment

Choose a reason for hiding this comment

seadowg commented Nov 18, 2024 •

edited

Loading

alyblenkin commented Dec 16, 2024 •

edited

Loading