Fixed the problem with parsing instances

[grzesiek2010]

Closes #415

What has been done to verify that this works as intended?

I tested the attached form and the form provided by @kkrawczyk123 getodk/collect#2882 (comment) (Remembering previously entered values)

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

The if statement was edited in this pr #406 and it was a mistake. I think @cooperka can confirm.

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 change should just fix the problem with the attached form. I edited the code implemented for remembering previously entered values so it would be good to confirm there is no problem with that functionality as well.

Do we need any specific form for testing your changes? If so, please attach one.

The form attached to the issue and the form provided by @kkrawczyk123 getodk/collect#2882 (comment)

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

[lognaturel]

@grzesiek2010 This looks like it will break last-saved support. Is that something you tried? Edit: hmm, looks like you did.

I haven't dug very deep yet but the check on a jr://file prefix moved to https://github.com/opendatakit/javarosa/pull/406/files#diff-3c2dbe0e3dc62d2d751abeb77add7986R533.

[grzesiek2010]

Yes, I tried as I said.
It can't break it because if we use that feature instanceSrc = jr://file/last-saved.xml so startsWith("jr://file/") retruns TRUE.

I haven't dug very deep yet but the check on a jr://file prefix moved to

It was moved there just to return rawSrc = instance.getAttributeValue(null, "src"); so the same as it was before https://github.com/opendatakit/javarosa/pull/406/files#diff-3c2dbe0e3dc62d2d751abeb77add7986L462

but we still need that additional check https://github.com/opendatakit/javarosa/pull/406/files#diff-3c2dbe0e3dc62d2d751abeb77add7986L464

[cooperka]

Hmm, won't this break support for jr://file-csv/? That might be why the bug went away, because that form is trying to use jr://file-csv/.

Above, instanceSrc = parseInstanceSrc() should already only be non-null if src starts with "jr://file/, "jr://file-csv/, or "jr://instance/last-saved/ so I don't think this is quite the right fix.

[lognaturel]

Agree with @cooperka. The exception is a deserialization exception. Did you clear the cached file between each trial, @grzesiek2010?

[grzesiek2010]

Did you clear the cached file between each trial

yes, maybe I missed something...

[grzesiek2010]

Ok a few last words from me before I go to sleep:
This pr just makes that the code works like it worked before we merged #406
I can confirm that I cleared cache every time and with my change, I can open the form like it was before #406
I think there is something wrong with that form because I can open it but can's see values from csv files but it works in the same way on older versions, maybe @mmarciniak90 edited the form. I tried the original one I had and then everything is fine https://drive.google.com/file/d/0B3xwpCYFo556eVptNUdyWmRoMmc/view?usp=sharing.

Maybe I missed something I don't really understand your concerns @cooperka but you know the code here better since you are the last editor.
Unfortunately, I won't be available tomorrow so everything I can do is just sharing these thoughts.

@lognaturel @cooperka if you are 100% sure I was wrong please go ahead and fix it.

[cooperka]

The code prior to #406 didn't have the trailing slash that was added here (see 406 diff), so I think this change basically removes support for file-csv and the error is fixed because JR doesn't even try to load the CSV.

Thank you for the context @grzesiek2010, I'll see what I can do Hélène is on it!

[lognaturel]

so I think this change basically removes support for file-csv and the error is fixed because JR doesn't even try to load the CSV

Sort of. This form doesn't actually use the file-csv support, it uses the client-only database-driven implementation of pulldata so the CSV gets loaded that way. Let's see what else I can figure out!

[lognaturel]

I thought that odk-csv in the FileNotFoundException in the error dialog looked really odd. What's going on is that the jr://file prefix is being used to derive the path because no reference factory exists for file-csv. When that prefix is removed from jr://file-csv/hhplotdetails.csv, we're left with -csv/hhplotdetails.csv and a path that doesn't exist.

Regardless, in that test form, we don't actually want to load hhplotdetails.csv as an external CSV instance. More details at #397 (comment).

All that to say that the problem has nothing to do with #406, it's actually caused by #397. But this change does incidentally work because it disables support for jr://file-csv, as @cooperka notes. I've spent a little bit of time trying to think of a quick way to make #397 play nice with the existing external CSV implementations but am coming up short. It seems risky this close to a release anyway.

I'm going to mark this as needs testing for @kkrawczyk123 to confirm that it works in all supported external data cases (last saved, external secondary XML instance, search/pulldata like the original form in the issue and http://xlsform.org/en/#external-selects). It won't work with external secondary CSV instances but it looks like those never worked, anyway. Provided all that looks good, I'll add a comment and merge.

[kkrawczyk123]

I've checked sample preloading forms, forms with external itemsets, external secondary instances, select one external - I haven't noticed anything suspicious, any error messages, I was able to finalize forms in all cases. I have also looked for the regression in the last saved submission feature and here nothing suspicious. I hope I didn't miss anything.
Verified on Androids 6.0 and 4.4.
@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

[lognaturel]

@ggalmazor, when you are satisfied with this, please merge and re-snapshot from master! 🙏

Thanks again for the initial legwork, @grzesiek2010. 👍

[ggalmazor]

Looking good :)