1012519 - Re-translation should use original content instead of newly translated content

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

After a first translation is finished, the UI shows:

- This page has been translated from [A] to [B].

Choosing a new language C on the dropdown triggers a new translation, however the page's content will be in the translated language B, not its original language A, which would cause an incorrect translation.

We could adjust the from/to languages to be from B to C, instead of from A to C. However this would be translation over translation which decreases quality significantly (see http://translationparty.com)

The best is to go back to original language before re-translation.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Ensure trDocument._state == STATE_ORIGINAL before translating

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Ensure trDocument._state == STATE_ORIGINAL before translating

Forgot a thing

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Felipe, please provide an estimate.
Marco, please add to the current iteration.

Comment 4

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8424586 [details] [diff] [review]
Ensure trDocument._state == STATE_ORIGINAL before translating

Different approach was discussed on irc

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Updating patch with new approach

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: trYieldShow

I also kept the TranslationDocument state code from the previous patch because it prevents a _swapDocumentContent process from beginning before the last one had finished.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Use original content for re-translation

It's never as simple as it first looks :)

The problem was that the generateTextForItem function was meant to be called only once (because in addition to generate the string representation it also builds the `original` array). So this would break with retranslation.

From a code-cleanness point of view, one would split the current code in generateTextForItem into two separate functions: one to build the `original` array, and the other to generate the string representation from it. Then, on re-translation, only the second function would be called.

However, I really don't want to lose the optimization benefit that is that generateTextForItem can do the two things at once, avoiding having to pass through the entire data structure twice.
So what I did was to build the second function as described above, but only use it in the re-translation case. There's some code duplication but the performance advantage is significant.

There was one complication in the new function: the non-significant nodes (converted to <br/>) are not stored in the original array, but we must re-generate them on the string text. But it was simple to solve because if two adjacent items are text, there existed a non-significant node between them.

I added the regenerateTextFromOriginalHelper function outside of the class because it doesn't do anything with `this`, so it felt more like a static function than a class function.

(In reply to :Felipe Gomes from comment #7)
> I also kept the TranslationDocument state code from the previous patch
> because it prevents a _swapDocumentContent process from beginning before the
> last one had finished.

I removed this part from this patch because it requires some extra changes so it doesn't make sense to ride in the same patch anymore

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8426783 [details] [diff] [review]
Use original content for re-translation

Review of attachment 8426783 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable :-).

::: browser/components/translation/TranslationDocument.jsm
@@ +352,5 @@
> +    return item.original[0];
> +  }
> +
> +  let localName = item.isRoot ? "div" : "b";
> +  let str = '<' + localName + ' id="n' + item.id + '">';

I tend to dislike duplicated code.

@@ +354,5 @@
> +
> +  let localName = item.isRoot ? "div" : "b";
> +  let str = '<' + localName + ' id="n' + item.id + '">';
> +
> +  let wasLasItemText = false;

wasLas_t_ItemText

@@ +373,5 @@
> +    }
> +  }
> +
> +  str += "</" + localName + ">";
> +  return str;

How would you feel about a generateTagForItem helper that would look like:

function generateTagForItem(item, content) {
  let localName = item.isRoot ? "div" : "b";
  return '<' + localName + ' id="n' + item.id + '">' + content + '</' + localName + '>';
}

Comment 12

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes from comment #8)

> There was one complication in the new function: the non-significant nodes
> (converted to <br/>) are not stored in the original array, but we must
> re-generate them on the string text. But it was simple to solve because if
> two adjacent items are text, there existed a non-significant node between
> them.

If we have 2 adjacent text nodes in the original DOM (which is possible, if the page does DOM manipulations and doesn't call https://developer.mozilla.org/en-US/docs/Web/API/Node.normalize), does this still work?


Also, I really think retranslation should be covered by some tests.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Use original content for re-translation

Comment 14

[:Felipe Gomes (needinfo for replies!)]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to :Felipe Gomes from comment #8)
> 
> > There was one complication in the new function: the non-significant nodes
> > (converted to <br/>) are not stored in the original array, but we must
> > re-generate them on the string text. But it was simple to solve because if
> > two adjacent items are text, there existed a non-significant node between
> > them.
> 
> If we have 2 adjacent text nodes in the original DOM (which is possible, if
> the page does DOM manipulations and doesn't call
> https://developer.mozilla.org/en-US/docs/Web/API/Node.normalize), does this
> still work?

The handling of 2 adjacent text nodes is not great yet, but it's not particular to retranslation. However, due to the way that getNthNonEmptyTextNodeFromElement and clearRemainingNonEmptyTextNodesFromElement works, various cases may still work, depending on the structure of the page.
> 
> 
> Also, I really think retranslation should be covered by some tests.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> Also, I really think retranslation should be covered by some tests.

Mentioned on IRC, but writing it here: a lot of the translation engine details are still in flux, with these bug fixes and the new expected behaviors from the follow-ups from bug 971054 (e.g., bug 1012530, bug 1012522). Once they are done i'm gonna work on the test suite.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8427743 [details] [diff] [review]
Use original content for re-translation

Review of attachment 8427743 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! :-)

Comment 17

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes from comment #14)
> (In reply to Florian Quèze [:florian] [:flo] from comment #12)
> > (In reply to :Felipe Gomes from comment #8)
> > 
> > > There was one complication in the new function: the non-significant nodes
> > > (converted to <br/>) are not stored in the original array, but we must
> > > re-generate them on the string text. But it was simple to solve because if
> > > two adjacent items are text, there existed a non-significant node between
> > > them.
> > 
> > If we have 2 adjacent text nodes in the original DOM (which is possible, if
> > the page does DOM manipulations and doesn't call
> > https://developer.mozilla.org/en-US/docs/Web/API/Node.normalize), does this
> > still work?
> 
> The handling of 2 adjacent text nodes is not great yet

Is there a bug on file for this?

Comment 18

[:Felipe Gomes (needinfo for replies!)]

There are new try builds now that include this fix and can be used to verify this bug and the others that were marked as duplicates: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-34039e42aea8/

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/e8229a59d520

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8229a59d520

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Did some exploratory testing around this on Windows 7 64bit, Mac OS X 10.9.2 and Ubuntu 13.10 32bit, verified that the language [C] translates language [A] (original language)