#1092 Enforced ID-bound alternative translations not recognized

[msoutopico]

Preconditions

The source file file2.xlf contains a segment with source text "foo":

<trans-unit id="2" approved="yes">
<source xml:lang="en">foo</source>
<target xml:lang="es">foo</target>
</trans-unit>

The /tm/enforce/tm.tmx file contains one default translation and one alternative translation, both of which are a 100% match for the segment.

<!-- Default translations -->
    <tu>
      <tuv lang="en">
        <seg>foo</seg>
      </tuv>
      <tuv lang="es">
        <seg>BAZ</seg>
      </tuv>
    </tu>
<!-- Alternative translations -->
    <tu>
      <prop type="file">file2.xlf</prop>
      <prop type="id">2</prop>
      <tuv lang="en">
        <seg>foo</seg>
      </tuv>
      <tuv lang="es">
        <seg>QUX</seg>
      </tuv>
    </tu>

Steps to reproduce

Open the attached project in OmegaT 5.7, which contains the file and TM described above.

Expected results

1. The context properties (ID and filename) of the alternative translation are used to get a in-context exact match.
2. Both translations appear in the multiple translations pane.
3. The isAlt = true appears in the segment properties pane.
4. The alternative translation is used to pretranslate the segment.

Actual results

1. The context properties (ID and filename) of the alternative translation are ignored.
2. Both translations are displayed as matches in the Matches pane but not in the Multiple Translations pane.
3. The isAlt = true does not appear in the segment properties pane.
4. The default translation is used to pretranslate the segment.

Additional info

This screencast shows compares between versions 4.2 and 5.7, to show how the expected behaviour happens in the former but not in the latter version:

https://recordit.co/sVotyBqoej

The same problem occurs if I use the Okapi XLIFF filter (and change source file and ID property accordingly).

[Aaron Madlon-Kay]

Thanks for the report. The best way you can help get this fixed faster is if you can identify in which version of OmegaT the behavior first changed.

[msoutopico]

Thank you so much, Aaron. I have tested this in all 5.x versions: it works well up to 5.2, and version 5.3 is the first one where I can reproduce the issue (also present in all versions after that). Does this help?

Is there anything else I can do?

[Aaron Madlon-Kay]

The next thing to do is figure out which change between 5.2 and 5.3 caused the behavior. If you are comfortable building OmegaT then it's "just" a matter of doing the following.

1. Check out source at a particular commit. You would probably start with the v5.2.0 tag.
2. Build and run or just run (./gradlew run) OmegaT
3. See if the behavior is the undesired pattern
   • If no, repeat from top with the next commit

Looking at the change history I see [bf52b2] looks quite suspicious.

[msoutopico]

Thank you, Aaron. You have been incredibly useful. I have identified the commit where the bug was introduced, I can confirm it's the one you found suspicious.

And I think I have identified the problem. It looked overwhelming at first, but after looking at it for some time, I could make sense out of it.

Somehow it's necessary to make it explicit that the enforced 100% match should be used to populate the segment also when a translation exists and it is default (not alternative).

I have added that case to the other two already there (namely, if there's no translation, and if the existing translation doesn't come from an enforced TM) and have updated the comment. I cannot reproduce the issue anymore now when I build and run version 5.8.

You can see the new version here: https://github.com/msoutopico/omegat/blob/6324563f0a8b55d70e03d52b6e8fc71eb1ade3bb/src/org/omegat/core/data/ImportFromAutoTMX.java#L97

If you give me the green light, I'll make a PR.

[Aaron Madlon-Kay]

Thanks for taking initiative on this.

I will (hopefully this weekend) sit down and try to understand

1. The problem that [bf52b2] was intended to fix and how exactly [bf52b2] fixed it
2. What the correct behavior should be in light of all this
3. Whether your patch gets us to the correct behavior

My suspicion is that the undesired behavior is an oversight, so hopefully the path forward should be clear. But it's also possible that, for instance, we might back out [bf52b2] entirely.

Taking a quick look at your change, it's small enough that a PR isn't necessary unless you'd like bragging rights.

[Aaron Madlon-Kay]

Thanks for your help diagnosing this and for your proposed fix.

I think your proposed fix is almost correct; the only problem I see is that I think it would allow for any subsequently encountered default enforced translation to override an existing default enforced translation. That would go against the intent of [bf52b2], which was to only allow use of the first-encountered enforced translation.

I've pushed a fix to master ([2f531b]). It appears to handle your test case correctly. Please let me know if you see anything amiss.

[msoutopico]

Thank you so much, Aaron. It's good that this particular issue is now fixed.

I believe it's not the only inconsistency in the way matches are handled, I'm testing that thoroughly but it'll take some time -- as you suggested somewhere else I'll try to write a specification draft before writing any further tickets.

In any case, for me an important takeaway of this filter is the successful collaboration: how a non-developer can help fixing a problem if some helpful guidelines and tips are provided about where to look. Hopefully this approach can be applicable in other cases. Thanks again.

[Hiroshi Miura]

released 6.0