Expose item child nodes in SelectChoice #660
Conversation
| : formDef.getMainInstance().resolveReference(item)); | ||
|
|
||
| choice.setIndex(i); | ||
|
|
||
| if (copyMode) { |
There was a problem hiding this comment.
This copyMode stuff isn't tested or documented. I think we should remove all of it in the near-ish future. For now I wanted to at least group it together so it's out of the way.
| /** | ||
| * The node that this choice represents. Not serialized. | ||
| */ | ||
| private TreeElement item; |
There was a problem hiding this comment.
I briefly considered trying to use copyNode for this. But like I said in another comment, I think we should remove all that stuff instead.
| @@ -75,6 +81,16 @@ public String getValue() { | |||
| return value; | |||
| } | |||
|
|
|||
| public String getChild(String childName) { | |||
There was a problem hiding this comment.
I also considered getProperty. I went with getChild because it best matches the underlying tree representation. Property makes the most sense for geojson and column makes the most sense for CSV.
| } | ||
|
|
||
| @Test | ||
| public void getChild_updates_whenChoicesAreFromRepeat() throws IOException { |
There was a problem hiding this comment.
Dude. You could, like, have a repeat that has a field called geometry. And then build a select with a map appearance on top of that. I don't know why exactly but that makes my brain explode a little bit. 🤯
I almost only populated the item field in the case of an external secondary instance because this kind of very dynamic situation scared me. But I can't actually think of a case where it wouldn't work.
lognaturel
changed the title
|
@seadowg hasn't done a final review yet but we're going to merge and make a snapshot so it's easier to integrate in ongoing Collect work (getodk/collect#4949). He'll then come back and file issues for anything that still needs to be addressed. |
Provides direct access to item children from a
SelectChoice.What has been done to verify that this works as intended?
Tests.
Why is this the best possible solution? Were any other approaches considered?
It's the best I could come up with. It makes the most sense to me that the
SelectChoiceitself would provide direct access to children of the item node that the choice is based on. Once reaching that conclusion there aren't a whole lot of options.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 should be additive and have no effect on users. I don't think there's a regression risk but I think that it might be an incomplete solution. Technically an
itemcould have a complex structure (nested nodes). I've never seen it but an XML or geojson document could have it. This doesn't give access to that more complex structure. The only use for it we have right now is to access the well-knowngeometrychild so I think it's ok.Do we need any specific form for testing your changes? If so, please attach one.
Tests.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.