Proposal for #4059 First nested repeat not created…

[dimwight]

Proposal for resolving #4059

This is very much a draft PR since it's on the wrong repository. I tried forking Javarosa but can't build it due to Checkstyle's dislike of Windows line endings.

As a workaround I cloned to Collect the classes that seem to need attention, then committed my proposed changes on top of them. This approach has the merit of being testable within Collect more or less as described in #4059.

The issue

Logging the state of the form instance shows that the crucial code is indeed in Javarosa. The initial instance tree is created with its nested repeat(s), but subsequent repeats are not.

Stepping through creation of a repeat shows that in the template tree acquired in FormDef.createNewRepeat the root has the right descendants.
However when called via FormInstance.copyNode, TreeElement.deepCopy fails to copy these because their multiplicity is INDEX_TEMPLATE ie -2; even if it did copy them their multiplicities would stop them being indexable.

The fix

The obvious solution is to clone the template tree and set multiplicities to DEFAULT_MULTIPLICITY ie 0: this will be correct for nested repeats and the appropriate value for the repeat root is set in copyNode.

Both copyNode and deepCopy have multiple call sites, so modifying either will most likely (certainly?) result in regression.

However in createNewRepeat we can pass to copyNode a clone of the template with adjusted multiplicities, created using an analogue in TreeElement of deepCopy. (The subsequent copying in copyNode will then be harmlessly redundant.)

This small change has the desired effect and should be safe from regression.

The code

As noted above

• the first commit clones the two Javarosa classes - incidentally generating some tens of PMD complaints
• the second makes the proposed changes
• the third comprises the test class and form

Testing

NestedRepeatTest loads the form specified in
NestedRepeats.xlsx, incidentally demonstrating that the fix works for at least one further level of nesting.

[dimwight]

Incidentally, being well aware there's a lot going on with Collect at the moment, I'm not expecting review of my offerings anytime soon.

When someone does have time to look at them, can I suggest in the following order?

• Show repeat in hierarchy view even if first instance has no relevant content #4762 is childishly simple
• Proposal for #4059 First nested repeat not created… #4797 (this) is essentially simple but complicated by the repository issue
• Resolves #4500 autocomplete minimal… #4712 is a bit of a magnum opus, especially the testing

[seadowg]

It looks like quality checks are failing and need fixed before we look at this. Running ./gradlew checkCode (or gradlew.bat checkCode) will show the report locally.

[dimwight]

All checks* seemed ok at my end, but I'll run them again and update as required.

* Except of course for the thirty or so PMD fails in the cloned Javarosa code (and another thirty from circleci Checkstyle) . Now updated these - with some misgivings as not my code.

Which reminds me that of course there's no way I'd want these commits merged.

[seadowg]

Marking this as a draft seeing as it's a set of changes for JavaRosa rather than Collect.

[seadowg]

@dimwight what errors are you seeing building JavaRosa on your machine? It'd be good to make sure JavaRosa development can happen on Windows as well as *nix.

[dimwight]

As mentioned above,

I tried forking Javarosa but can't build it due to Checkstyle's dislike of Windows line endings.

Unlike in Collect's separate checkCode task, Checkstyle is run as part of the JavaRosa build task, so on Windows the whole task fails. Is there any way to remove or disable the Checkstyle element (preferably within AS)?

[seadowg]

@dimwight I'm wondering if there is a way we could get CheckStyle running on Windows rather than avoiding. What exact error/failure are you seeing?

[dimwight]

get CheckStyle running on Windows

There's no problem running it, it just fails every time on Windows line endings. Do not use Windows line endings [RegexpMultiline]
In Collect this happens at the end of checkCode so it doesn't get in the way. In Javarosa on the other hand it stops build in its tracks.

[seadowg]

@dimwight is that on a clean check out or only once you've added/changed code? I'm wondering if there is an editor setting that can sort this out.

[dimwight]

a clean check out or only once you've added/changed code?

To be exact,

1. I follow instructions at https://github.com/getodk/javarosa,
   to fork and import project to Intellij.
2. I run gradle build.
3. The build fails with these messages:

Execution failed for task ':checkstyleJmh'. Checkstyle rule violations were found. See the report at: file:///...jmh.html Checkstyle files with violations: 18 Checkstyle violations by severity: [error:1984]

All the errors are Do not use Windows line endings which is what I get when Checkstyle is run in Collect checkCode

To my eye the most straightforward solution would be to omit Checkstyle from the Javarosa build, is this not possible?

[dimwight]

Further update. Woke up this morning and decided to try the brute force solution of fixing by hand each of the eighteen files complained of by checkstyleJmh, which then indeed passed.

However checkstyleMain then failed another 298 files, also for Windows line endings.

Is there anyone on the Javarosa team I could raise this with, who's an expert on the build files?

[seadowg]

@dimwight my best guess here is that git is checking out files with Windows line endings.

To make your Git config more Unix friendly (probably a good idea for working with any open source project) you can set core.autocrlf to input to have Git check files out as-is and commit with Unix line endings (more here). Then so that IntelliJ doesn't add windows line endings that will break builds locally, you can switch the Line Separator to "Unix and macOS" (more here). If that works, we can add that to the JavaRosa (and possibly Collect's) docs.

Thanks for soldiering through the horrible Unix/Windows incompatibilities!

[dimwight]

you can set core.autocrlf to input to have Git check files out as-is and commit with Unix line endings

Sounds promising, even if it means I have to bite the bullet on the Git CLI.

[seadowg]

@dimwight if you're using SourceTree, you can hit "Settings" (top right) then "Advanced" and then "Edit Config File..." to change the Git config being used. That way you can stay away from the CLI I think!

[dimwight]

Thanks @seadowg for these suggestions. They were very helpful in that they put me on the right track, though the actual anwer came out different.

On looking autocrlf up in the Git book I found at https://git-scm.com/docs/git-config the following:

core.autocrlf...Setting this variable to "true" is the same as setting the text attribute to "auto" on all files and core.eol to "crlf"...
to have CRLF line endings in your working directory...
can be set to input, in which case no output conversion is performed.

So this makes no promises about working files which are what Checkstyle was failing on. But on the same page is the following:

core.eol...Sets the line ending type to use in the working directory for files that are marked as text... ignored if core.autocrlf is set to true or input

This seemed much more promising and indeed does the trick.

So now I was ready to close this PR with thanks for your assistance, except that as a next stage I wanted to test my updated Javarosa code in Collect.

Unfortunately these instructions tripped me up at step 2 with the issue described here. In the absence of any error message it does seem likely that I need to prepare my Javarosa folder somehow before trying to import into Collect.

Any thoughts?

[dimwight]

I had a thought - switch to the instructions for building a jar and referencing it in Collect. I built my jar, but didn't succeed in setting the path.

1. To start with I deliberately broke the Collect build by opening Dependencies.kt and changing the value of javarosa to the name of my new jar.
2. In collect_app/build.gradle, found the dependencies section.
3. Took a guess that I needed to update the implementation(Dependencies.javarosa) block.
4. Pasted in the suggested code, deliberately not changing the dummy path and hoping for just a complaint about the bad path.
5. Instead got Could not find method compile(). Apparently
   "Gradle updates have deprecated compile ...in favor of implementation ..." which suggests the instructions are a bit dated.
6. Unfortunately Gradle documentation is even more opaque than Git and doesn't even seem to be searchable.

[seadowg]

"Gradle updates have deprecated compile ...in favor of implementation ..." which suggests the instructions are a bit dated.

Looks like our instructions are out of date, and you should use implementation instead of compile as the error suggested.

[seadowg]

Closing this. If you have run into any more problems, then please post in #javarosa-code or #collect-code on Slack.

[dimwight]

Hi @seadowg, you'll be interested to know I finally worked out my own solution to this, which works quite elegantly in the latest AS. Can document this if anyone is interested.

Since #4059 was yours originally, do take a look at the revised version of this PR now on JR as #645