FormHierarchy cleanup #2730
FormHierarchy cleanup #2730
Conversation
Introduced at https://github.com/grzesiek2010/collect/blob/2ecb17048d6ef1f6779dce5276a7c7e623d218b7/collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java#L116 with the view-only mode. Would be useful if current index could be set when the view-only activity is opened but that doesn't seem possible since the activity is not launched from form filling.
The removed comment was incorrect -- startIndex is used when the back button is pressed.
The @nonnull annotations in HierarchyElement make this impossible. The null check was added in https://github.com/opendatakit/collect/blob/77f4f2eadc5302c9968e9cb332fb090018e9681f/src/org/odk/collect/android/activities/FormHierarchyActivity.java#L308 and I am pretty sure that it was needed only because of the .. entry added with an explicitly null index (https://github.com/opendatakit/collect/blob/77f4f2eadc5302c9968e9cb332fb090018e9681f/src/org/odk/collect/android/activities/FormHierarchyActivity.java#L186)
|
This cleanup is extremely helpful, thank you @lognaturel! Reading your commits is like reading a book 📖 |
|
LOL. A real page turner! I couldn't put it down! "Lyrical, incisive, and downright witty, Martin expertly leads us through a gauntlet of code smells and anti-patterns in one of the most engaging pull requests in recent memory." |
There was a problem hiding this comment.
Everything looks so clean and well defined. The commit messages were really helpful to quickly understand the changes without much effort.
I noticed a small bug with the help of the comments that should be easy to fix. Thanks for adding those comments everywhere @lognaturel!
| */ | ||
| @Override | ||
| public boolean onKeyDown(int keyCode, KeyEvent event) { | ||
| return super.onKeyDown(keyCode, event); |
There was a problem hiding this comment.
Calling the super method would call FormHierarchyActivity's onKeyDown(), resulting in logging of an audit event.
We can add a check for determining whether the activity is ViewOnlyFormHierarchyActivity or not by using local variable or using instanceOf.
Overriding onBackPressed() instead of onKeyDown() since we are only tracking the back button can further simplify the readability of code
There was a problem hiding this comment.
@shobhitagarwal1612 since lognaturel is out I made this fix here: cooperka@a6d1843
I can't push to her branch but I assume you can as a Member.
git remote add cooperka https://github.com/cooperka/collect.git
git fetch cooperka
git cherry-pick a6d1843| if (index == null) { | ||
| goUpLevel(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Removing this null check looks safe to me too.
| @@ -39,8 +39,6 @@ | |||
| */ | |||
| @Override | |||
| void configureButtons(FormController formController) { | |||
| formController.stepToOuterScreenEvent(); | |||
|
Thanks, all! |
@smoyth and @cooperka have let me know that they are interested in taking on #809 which @smoyth wrote a spec for, approved by the TSC. @shobhitagarwal1612 had previously built a prototype at #1271.
A big challenge is that the code is structured in unexpected ways. This PR performs some cleanup and documents the current structure (which will hopefully be changed soon).
Most commits are just moving things around or adding comments. I added commit details to describe my reasoning on bigger changes. In particular, 914ca44 and 2c5a701 may present some risk. @grzesiek2010, that first one was introduced in 029afa3 so maybe you have some memory of why.
What has been done to verify that this works as intended?
Filled out All Widgets and groups-repeats.xml.txt forms, jumped at different points in the form and particular into and from repeats. Sent the finalized forms and did the same from View Sent Form (view-only variant of the hierarchy).
Why is this the best possible solution? Were any other approaches considered?
A potentially slightly controversial refactor is in commits a563e99 to 9280d8d: I change the split between a hierarchy view that allows edits and one that doesn't. The prior split wasn't ideal because there was a lot of redundancy which made it hard to see what was different between the two. It also separated the really important click handler from the rest of the implementation. The approach I took was to introduce some methods which could be overridden to block all edit behavior. I think that makes it very clear what the view-only behavior is.
The possibly risky commits 914ca44 and 2c5a701 could be omitted. I included them because they really seemed like unnecessary code.
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?
There should be no changes for the user. The biggest sources of risk are:
Do we need any specific form for testing your changes? If so, please attach one.
No.
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:
./gradlew checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.