Anki-android: Regression in nested conditional fields

[JHerseth]

Nested conditional fields stopped working between 2.10Alpha71 and 2.10Alpha72.
At least if the same field is "nested within itself" so to speak.

Of course the workaround is to not have the same conditional field nested within itself. I don't have a good reason for doing it that way, but my template was created organically with a lot of trying and testing, and thats how it ended up. Everything was working until I updated from Alpha 70 i think to beta3, and since I used quite some time identifying where the regression happened and what was the root cause, I felt I should make this report :)

Reproduction Steps

Make a card with nested conditional fields, e.g.

{{#One}}
    {{#One}}
        {{One}}<br>
    {{/One}}
    {{#Two}}
        {{Two}}
    {{/Two}}
{{/One}}

I've made a minimal deck that demonstrates the issue. Works in Alpha71, but not in 72.
download

Expected Result

Render as normal

Actual Result

{{invalid template}}

Debug info

Refer to the support page if you are unsure where to get the "debug info".

Research

Enter an [ x ] character to confirm the points below:

[x] I have read the support page and am reporting a bug or enhancement request specific to AnkiDroid

[x] I have checked the manual and the FAQ and could not find a solution to my issue

[x] I have searched for similar existing issues here and on the user forum

[mikehardy]

Have you retested with 2.10beta3? Please do so as I think this was just fixed #6108 and released in 2.10beta2 at first. The betas are also going out on the alpha channel right now so if you're testing alphas it should be easy to access

[david-allison-1]

Hi @JHerseth,

Thanks so much for the detailed report (especially the two versions - makes life so much easier) 🙏. Will look into it.

https://github.com/ankidroid/Anki-Android/compare/v2.10alpha71...v2.10alpha72

Will get a regression test in and see where it breaks.

[JHerseth]

I've tried both beta3 and alpha73, and I can reproduce it there

[mikehardy]

Thanks and echoing David, thanks especially for the version bisect, that is like a laser to find and fix these things.

[JHerseth]

I'm the one who should be thanking you! Spending 30-60 minutes documenting a bug I found is the least I could do. After all I'm benefiting from your hard work every day :) Thank you!

[david-allison-1]

First use of git bisect ...cool stuff!

6511ae148023461d60efbe4ec66576e3e362ed6d is the first bad commit
commit 6511ae148023461d60efbe4ec66576e3e362ed6d

    render section use string buffer

 .../java/com/ichi2/libanki/template/Template.java  | 27 +++++++++++-----------
 1 file changed, 14 insertions(+), 13 deletions(-)
bisect run success

https://github.com/ankidroid/Anki-Android/commit/39f180e8f8a3fa08da0813fa7fb1f83088828958

[mikehardy]

smells similar to #6108 / unintended semantic difference (the discussion on #6103 was the interesting bit I think)

[Arthur-Milchior]

You may want to consider http://milchior.fr/anki/Testdeck.apkg which contains more test. In particular, it tests the case where One is not filled.
I tested it on anki. It is only with 2.1.20 that this started to work. On 2.1.17 it shows "{unknown field /One}" when One is not filled. So I'd be amazed if having the same field conditional nested was something that used to work

Actually, things were even stranger. If you put

{{#One}}
    {{#One}}
        {{One}}<br>
    {{/One}}
    {{#Two}}
        {{Two}}
    {{/Two}}
{{/One}}

in Front ( http://milchior.fr/anki/Reverse.apkg ) and fills only two, the card was generated, because it didn't even detect that the conditional on two was in a one conditional

[david-allison-1]

Forgot to mention. Regression test showing expected and actual value after the change. It worked previously in AnkiDroid, and this can be used to bisect if/when it started working, as well as if/when it started failing.

TemplateTest.java

/*
 Copyright (c) 2020 David Allison <[email protected]>

 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
 Foundation; either version 3 of the License, or (at your option) any later
 version.

 This program is distributed in the hope that it will be useful, but WITHOUT ANY
 WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
 PARTICULAR PURPOSE. See the GNU General Public License for more details.

 You should have received a copy of the GNU General Public License along with
 this program.  If not, see <http://www.gnu.org/licenses/>.
 */

package com.ichi2.libanki.template;

import org.junit.Test;
import org.junit.runner.RunWith;

import java.util.HashMap;

import androidx.test.ext.junit.runners.AndroidJUnit4;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;

@RunWith(AndroidJUnit4.class)
public class TemplateTest {

    @Test
    public void nestedTemplatesRenderWell() {
        //#6123
        String problematicTemplate = "{{#One}}\n" +
                "    {{#One}}\n" +
                "        {{One}}<br>\n" +
                "    {{/One}}\n" +
                "    {{#Two}}\n" +
                "        {{Two}}\n" +
                "    {{/Two}}\n" +
                "{{/One}}";

        HashMap<String, String> context = new HashMap<>();
        context.put("One", "Card1 - One");
        context.put("Two", "Card1 - Two");
        Template template = new Template(problematicTemplate, context);

        String result = template.render();

        //most important - that it does render
        assertThat(result, not("{{invalid template}}"));
        //Actual value (may be subject to change).
        assertThat(result, is("\n    \n        Card1 - One<br>\n    \n    \n        Card1 - Two\n    \n"));
    }
}

[Arthur-Milchior]

I just tested on Ankidroid v2.10alpha 71. There was the same problem. If you don't fill Field Two, you got the error message "{unknown field /One}".

So, as I expected, nested conditional on the same field was already broken before my PR. Simply, it is now broken in a different way.

To be more precise, before, when One was filled the replacement was as follow:

{{#One}}
    {{#One}}
        {{One}}<br>
    {{/One}}
    {{#Two}}
        {{Two}}
    {{/Two}}
{{/One}}

Then

    {{#One}}
        {{One}}<br>
    {{#Two}}
        {{Two}}
    {{/Two}}
{{/One}}

Then

        {{One}}<br>
    {{#Two}}
        {{Two}}
    {{/Two}}

and finally

        {{One}}<br>
        {{Two}}

or

        {{One}}<br>

Now it starts with

{{#One}}
    {{#One}}
        {{One}}<br>
    {{/One}}
    {{#Two}}
        {{Two}}
    {{/Two}}
{{/One}}

and then detects that the content of the first conditional is

    {{#One}}
        {{One}}<br>

and the second is

        {{Two}}

It tries to apply the replacing rule to the first conditional content and fails, so it leaves it as it is (he does not even detect there is a cloze opening)
The second content has nothing to consider, so it stay as it is.
This lead to the result

    {{#One}}
        {{One}}<br>
        {{Two}}
{{/One}}

I want to note that, in this case, it's pretty simple to get both the increase of speed and the old system (which will work on this example, but was broken as soon as One was not empty). It suffices to put render_sections in a while(true) loop, as it was before my PR, and break out of the loop when the matching fails.
This way, it will check for those cases with nested conditionals, and in this case do a second, third... iteration of the process. But the case where the same conditional is not nested inside itself will be considered in a single pass of the matcher, and still be quick.

[Arthur-Milchior]

Thanks @dae for letting me know about this bug

[Arthur-Milchior]

You may also want to consider milchior.fr/anki/121.apkg , which shows the example

{{#One}}
  1
  {{#Two}}
    12
    {{#One}}
      121
    {{/One}}
  {{/Two}}
{{/One}}

If we allowed nested conditional for the same field, this would not create a problem. Anki 2.1.17 show an error message as soon as either One or Two is missing.
Current Anki accepts it.

[Arthur-Milchior]

So, to be clear, until recently, Anki did not dealt with a field conditional nested inside the same field conditional. To be more precise it still worked as long as all field are filled, but it showed an error message in a field is empty. Which makes the very principle of fields conditional quite useless if they should absolutely be field to avoid the error message.
Anki switched recently to way to really parse the full conditional tree, which solves this problem.

As far as AnkiDroid goes, with used to have the same limitation. We have not (yet) followed anki's recent change. I tried to make the code faster (because this part was slow on my phone, while the computer was able to handle it so quickly that it never really was an important part of anki's slowness)
I see multiple way this can go:

• Either we revert my PR. Nested fields with same conditional will still be broken the way they were in Anki. Except in the case where everything is filled where it's not broken, but is a strange limitation
• Either someone do the work to tell ankidroid to uses Anki's rust (is it possible, since AnkiDroid is GPLv3 and Anki is AGPLv3, to have both in the same codebase ?)
• Either someone port Anki's rust to AnkiDroid (i.e. creates a parser and do the real syntactical tree). That would be cool, but I clearly don't want to do the work.
• Either we do nothing and state explicitly that the same conditional can't be nested inside itself. After all, this was allowed under such restrictive conditions that it made little sens to accept it in the first place.
• Either we do the change I proposed above. Nested conditionals will still be broken in general, but it will work in the case where all fields are filled. This is a strange behavior, but at least it will be the strange behavior we used to have while still being quick.

I have sent a request to my employer to be authorized to do PR to Anki and AnkiDroid's code. In theory they say they accept almost everything as long as it's not similar to one of their product. But even if I expect them to accept, I can not do a PR immediately without putting my job at risk. I still hope that if we choose to go the last way, my comment above was clear enough so that someone can find what are the four lines that should be changed to repair it

[mikehardy]

Hey @Arthur-Milchior

Either we do the change I proposed above. Nested conditionals will still be broken in general, but it will work in the case where all fields are filled. This is a strange behavior, but at least it will be the strange behavior we used to have while still being quick.

• I would like to not revert because performance is not the highest priority but is still a priority.
• I would like to wrap the Rust code but that will not happen for 2.10
• I do not want a fresh template parser, so a Rust port is not desired
• Stating we don't support it is possible but...
• Your last option, the 'Either we do the change I proposed above' is ambiguous because there are many things proposed above.

The last item seemed like it would be attractive if I could find the actual antecedent for what you recommend. Is there some combination that is fast and still allows nested conditionals if they are fully filled in (so that it is at least not a regression)? I'd like to do that if possible.

Otherwise I will revert for 2.10 so that we at least don't have regressions vs prior behavior even if the ecosystem has moved on and we now have compatibility issues

[Arthur-Milchior]

I was referring to:

I want to note that, in this case, it's pretty simple to get both the increase of speed and the old system (which will work on this example, but was broken as soon as One was not empty). It suffices to put render_sections in a while(true) loop, as it was before my PR, and break out of the loop when the matching fails.
This way, it will check for those cases with nested conditionals, and in this case do a second, third... iteration of the process. But the case where the same conditional is not nested inside itself will be considered in a single pass of the matcher, and still be quick.

[mikehardy]

@david-allison-1 you have a regression test for this already - could combine the test and Arthur's comment above into a PR? Then I can do a 2.10beta4 and our beta list will be down to 0️⃣ again

[david-allison-1]

I want to note that, in this case, it's pretty simple to get both the increase of speed and the old system (which will work on this example, but was broken as soon as One was not empty). It suffices to put render_sections in a while(true) loop, as it was before my PR, and break out of the loop when the matching fails.
This way, it will check for those cases with nested conditionals, and in this case do a second, third... iteration of the process. But the case where the same conditional is not nested inside itself will be considered in a single pass of the matcher, and still be quick.

@Arthur-Milchior, I'm not sure I understand your comment well enough to implement this.

Isn't this what the code is still doing: https://github.com/ankidroid/Anki-Android/commit/39f180e8f8a3fa08da0813fa7fb1f83088828958 . The while loop is broken out of if match.find() returns false.

[mikehardy]

Should be fixed by #6130 - 2.10 beta with it coming up in about a day

[mikehardy]

2.10beta4 is processing and heading out to beta now