i18n: Translator upgrades

[bgptr]

Closes #3583

Notes and screenshots:

• for diff highlighting, I've imported http://github.com/google/diff-match-patch. This feature would be difficult to implement from scratch, but this lib does the heavy lifting quite nicely. I've added a toggle button for viewing the original string for convenience.

[xaur]

Per this comment, previous_original comes from some older v1.5.x. I would use v1.5-approx in meta.version for now. When all strings are ready, this file will be updated to v1.6.3

[xaur]

Wow this is incredible and way beyond my expectations. Thank you for working on this!

I will be testing this in the coming days, but one quick thing is, is it possible to remove requests to the Eye of Cloudflare? Like commit the lib in this repo (if maintainers are ok with it) or at least use a different hosting (decred.org perhaps)?

To clarify, I will not be a heavy user of this tool and real translators might be ok with it, but asking just in case there is an easy solution.

[bgptr]

I'm ok with committing the lib in the repo.
@alexlyp, what are your thoughts about it?

[xaur]

Let's add for context that we're talking about diff_match_patch.js (21 KiB compressed, 77 KiB uncompressed).

[xaur]

Allright, sorry for the wait. First round of testing.

New features look great. This tool is much more powerful now.

Bugs:

• on Deleted list, save & next button should change to next and become enabled
  • match behavior of the previous button
• previous now means "previous in the navigation stack" and not "previous in the list of the selected entry"
  • saving >1 pairs {filter, key} in a navigation stack produces confusing behavior
  • e.g. do save & next on Unstranslated, change to Changed, mouse-click an entry, hit save & previous -> view changes to Untranslated instead of staying in Changed and going to the previous entry
  • "previous" shold be symmetrical to "next": next goes to next entry in active list, previous goes to previous entry in active list (and not in its own nav stack)
  • I suggest to remove the stack and only store 1 pair of "selected entry" + "filter name for selected entry"
  • jumpToPreviousKey() will need to be updated since it relies on previousKeys.length > 1
  • update selected entry and its filter on any method of selection (mouse click, save & next/previous)

Tweaks (optional, can do in future PRs):

• increase font size of gray labels a bit

  • in both left-hand list and right-hand fields
  • value 80% felt good (current 60%)
  • bigger font looked better with 1-2px extra top padding

• paint key labels in Deleted list in same gray color as key labels on other lists

• make Hide diff button all lowercase for consistency

• rename Download Translation to save translation or export translation or export .json

  • it's all local so no "download" is taking place
  • lowercase for consistency
  • opinions on what is more intuitive, translation/.json?

• make left-hand list width fixed

  • is is jumping when switching between Changed and Deleted lists

Performance (optional, can do in future PRs):

• operations on the All list are slow (changing to it, clicking on entries, save & next)

[xaur]

Diffing looks very helpful for translator's awareness and productivity, thanks for investigating!

Noticed the page uses 20121119/diff_match_patch.js from the Cloudflare CDN, but Google's Git repo has a newer version (+7 years but not much changes :O). If we import it, let's use the newer one.

@alexlyp or maybe @matheusd any objections to committing the lib to this repo?

[matheusd]

@alexlyp or maybe @matheusd any objections to committing the lib to this repo?

Seems reasonable to me, and indeed better then to hit CF every time this is opened.

[bgptr]

@xaur, thanks for the review. I've pushed the fixes. I didn't improve anything on performance now, but I will after it functionally works as expected.
update: Eventually, I've worked on the performance a bit. Although I don't notice any lagging, the investigation in chrome's performance tool showed that rerendering the list all the time is the bottleneck. Now, the lists are rendered only once, except the Untranslated list if there was a change. I've simplified the logic of the next and previous functions also.

[xaur]

Tested:

• two bugs found earlier do not reproduce
• All list is way faster now: initial selection of it takes a bit of time, but selection of items in it, previous/next - are all fast
• bigger label font is nice
• left list width no longer jumps

One more idea that crossed my mind is to load the translated string even on the Deleted list keys, to allow translators to recover and reuse any strings that will go away. This is optional, we can do this later.

Otherwise, I see no functional bugs. Great work!

[xaur]

Code-wise, good idea to add the uncompressed lib in case we'll ever need to tweak it.

I see a big diff with upstream master (looks formatting only), which may complicate porting upstream changes.

[xaur]

@bgptr the uncompressed version looked more future-proof in terms of maintenance, did you found a reason how the compressed one is better?

[bgptr]

@xaur I considered only the file size, but you're right. The uncompressed version could be more future-proof. I've updated it.

[xaur]

Ok great. There's still a diff with upstream lib but it's up to you guys. The thing is working pretty good.

@matheusd you may want to pick the review from here since you kickstarted this tool and are familiar with code.

[xaur]

@Insaf01 helped me verify that we have a common issue with bidirectional text, where a mix of Arabic and English is ordered incorrectly. For example, this is for key agenda.card.finishedTooltip in the ar.json file:

and this is how it shold look like:

I hacked the <textarea> element of the translation box to add the dir="rtl" attribute and I think it fixes it:

While this PR is not merged, we can use it to fix this issue too.

If my hack is good enough, we'll need to conditionally set the dir attribute based on something. How about, add the meta object to the translated files too, and if it's present and has the dir attribute, set it in the text area?

[bgptr]

@xaur I've set the dir attribute to auto. It solved the issue for me.

[xaur]

I also now see Arabic text flowing right-to-left and auto is a much simpler solution, thanks @bgptr. I'll ask Insaf to confirm that it looks proper now.

[matheusd]

Working as intended. I also verified the imported diff_match_path.js matches the one from the original github repo.