Repeating grp fix #504
Repeating grp fix #504
Conversation
bennsimon
commented
Oct 21, 2020
•
bennsimon
commented
- Updates the logic to update the repeating group count object
android-json-form-wizard/src/main/java/com/vijay/jsonwizard/task/AttachRepeatingGroupTask.java
Outdated
Show resolved
Hide resolved
| stepFields.put(repeatingGroupCountObj); | ||
| return repeatingGroupCountObj; | ||
| } else { | ||
| return null; |
There was a problem hiding this comment.
Do we need the else block? Is there ever an instance when stepJsonObject is null?
There was a problem hiding this comment.
yeah for malformed json form
There was a problem hiding this comment.
The method throws JSONException, so I'm thinking malformed JSON will be covered here? The calling method can then figure out how to handle the exception, no?
There was a problem hiding this comment.
Ok, but when is the stepJsonObject null?
There was a problem hiding this comment.
either way, you can simply return null without the else block, right?
There was a problem hiding this comment.
Ok, but when is the stepJsonObject null?
When the step does not exist ... for malformed json forms
either way, you can simply return null without the else block, right?
I will still need an if block
There was a problem hiding this comment.
yea, we can remove the else block though.
| //get the generated number of groups | ||
| final String generatedGroupsCount = jsonObject.optString(REPEATING_GROUPS_GENERATED_COUNT); | ||
| final String generatedGroupsCount = countFieldObject != null ? countFieldObject.optString(VALUE) : ""; |
There was a problem hiding this comment.
Based on https://github.com/OpenSRP/opensrp-client-native-form/pull/504/files#r512021105. Should countFieldObject ever be null?
There was a problem hiding this comment.
yeah for malformed json form
| referenceEditText.setOnFocusChangeListener(new View.OnFocusChangeListener() { | ||
| @Override | ||
| public void onFocusChange(View view, boolean hasFocus) { | ||
| if (!hasFocus) { | ||
| addOnDoneAction((TextView) view, doneButton, widgetArgs); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Any reason to remove this? This was recommended as a better user experience by Roger onaio/rdt-standard#319 (comment).
There was a problem hiding this comment.
Yeah, this feature breaks the normal behaviour of repeating group, it adds a bug ... it creates twice the number of groups needed.
There was a problem hiding this comment.
Ok, were you able to figure out why that happens?
There was a problem hiding this comment.
referenceEditText gains focus when cursor is on it, a number is entered ... then after referenceEditText loses focus when the done button is clicked it fires the addOnDoneAction from focusChange listener at the same time the done button clickListener does the same ... so it generates twice the number of groups.
There was a problem hiding this comment.
Cool. Would removing the addOnDoneAction from the done button work then?
There was a problem hiding this comment.
I meant that since onFocusChanged is triggered when the button is clicked (after entering text) we can simply use onFocusChanged and remove the onclicklistener from the done button.
There was a problem hiding this comment.
I dont think thats a good idea, looks like a hack ... i think one has to be disabled.
There was a problem hiding this comment.
It really isn't. The point of the feature (from Roger's comment) is to generate the repeating group when one navigates away from the reference edit text. Clicking on other views (including the done button) counts as navigating away.
There was a problem hiding this comment.
Why would you need the unclickable done button if you have a onFocusChangeListener on a referenceEditText? I dont get it but anyways ... This needs to be configurable since not all implementation expect that behaviour.
There was a problem hiding this comment.
tbh, it's only for backward compatibility, we don't need a button any more. The button being "truly" clickable or not doesn't really change the end-user experience so that's not a problem.
You can make it configurable, but any time someone wants both the button and focus change to be supported, the bug will show up again.
android-json-form-wizard/src/main/java/com/vijay/jsonwizard/widgets/RepeatingGroupFactory.java
Show resolved
Hide resolved
bennsimon
force-pushed
the
repeating_grp_fix
branch
from
f4ed03a to
535729d
Compare
