Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/translations: add 🇮🇹 🇵🇹 #4047

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Jul 21, 2024

Turkish translation fails click tests and from UI reports it seems there are also some missing glyphs. PR adds this third language in the current state but does not enable tests for it.

@mmilata mmilata added the translations Put this label on a PR to run tests in all languages label Jul 21, 2024
Copy link

github-actions bot commented Jul 21, 2024

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T2B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

Copy link

github-actions bot commented Jul 21, 2024

legacy UI changes device test(screens) main(screens)

@mmilata mmilata force-pushed the mmilata/translations-itpttr branch 3 times, most recently from 7f42b6c to c2e8e61 Compare July 22, 2024 22:03
@mmilata mmilata changed the title feat(core/translations): add it, pt, tr core/translations: add 🇮🇹🇵🇹🇹🇷 Jul 22, 2024
@mmilata mmilata marked this pull request as ready for review July 22, 2024 23:05
@mmilata mmilata requested review from matejcik and prusnak as code owners July 22, 2024 23:05
@mmilata mmilata requested review from obrusvit and removed request for prusnak July 22, 2024 23:05
@mmilata mmilata self-assigned this Jul 22, 2024
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some problems:

  • the new lang JSONs contain UPPERCASED titles and buttons (remnant from TR, TT. Now they are uppercased automatically by the used font as of Unicode font uppercasing issues #3696
  • some changes which occurred recently are not yet reflected, e.g. reset__slip39_checklist_num_shares_x_template is missing in the new JSONs
  • it device test still failing

I guess we can merge even with these limitations so that someone can start testing it. WDYT @mmilata

@mmilata
Copy link
Member Author

mmilata commented Jul 23, 2024

Since we're not in a rush wrt blob signing I'd say let's fix these issues & then merge.

If QA or anybody wants unsigned blobs then please let me know.

@mmilata mmilata force-pushed the mmilata/translations-itpttr branch from 18be7b9 to 55683f1 Compare July 23, 2024 21:49
@Hannsek
Copy link
Contributor

Hannsek commented Jul 24, 2024

Do you know why it device tests are failing? Is there a reason for it?

@mmilata
Copy link
Member Author

mmilata commented Jul 24, 2024

The failing T2B1 click tests are mysterious, it's happening in every language including english, and this PR doesn't even touch any code that's involved, seemingly?

https://github.com/trezor/trezor-firmware/actions/runs/10066871496/job/27829340045

@Hannsek
Copy link
Contributor

Hannsek commented Jul 24, 2024

I was reffering to it device test still failing

@mmilata
Copy link
Member Author

mmilata commented Jul 24, 2024

Got it in a state where only (it,pt,tr) T2B1 click tests are failing. There is some issue with how the test code handles translated strings. Possibly related: if inputs__delete translation string is not uppercase the T2B1 click tests also break apart.

@mmilata mmilata marked this pull request as draft July 24, 2024 22:30
@mmilata mmilata force-pushed the mmilata/translations-itpttr branch 3 times, most recently from b42b854 to 3ca45d9 Compare August 29, 2024 10:18
@mmilata mmilata changed the title core/translations: add 🇮🇹🇵🇹🇹🇷 core/translations: add 🇮🇹,🇵🇹 Aug 29, 2024
@mmilata mmilata changed the title core/translations: add 🇮🇹,🇵🇹 core/translations: add 🇮🇹 🇵🇹 Aug 29, 2024
@mmilata mmilata force-pushed the mmilata/translations-itpttr branch from d32850b to 8c568d8 Compare August 29, 2024 23:03
@mmilata mmilata marked this pull request as ready for review August 29, 2024 23:03
@mmilata mmilata requested a review from ibz September 2, 2024 12:35
@ibz
Copy link
Contributor

ibz commented Sep 2, 2024

LGTM as far as this PR is concerned.

Whether the translations are good, complete, etc, is outside the scope of this.

We will probably need some way to at least dump keys that are missing in various translations, but more generally to ease keeping translations in sync.

Copy link
Contributor

@ibz ibz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a mechanism to detect if a char is missing (as part of the tests?). For example, you did not include some chars that are technically allowed in some languages, and I understand that it is an optimization, but that also means that if something containing an "È" suddenly appears in Italian, for example, we will probably not notice it, and it will render as a "?"...

@mmilata
Copy link
Member Author

mmilata commented Sep 2, 2024

Thanks for the review!

We will probably need some way to at least dump keys that are missing in various translations, but more generally to ease keeping translations in sync.

core/transaltions/cli.py sounds like the right place for this.

I think we need a mechanism to detect if a char is missing

Agreed, opened #4149.

@mmilata mmilata force-pushed the mmilata/translations-itpttr branch 3 times, most recently from ea7a35e to 91bd994 Compare September 3, 2024 16:15
@mmilata mmilata force-pushed the mmilata/translations-itpttr branch from 91bd994 to eab2e18 Compare September 3, 2024 17:54
@mmilata mmilata merged commit 777ad11 into main Sep 3, 2024
137 of 139 checks passed
@mmilata mmilata deleted the mmilata/translations-itpttr branch September 3, 2024 19:58
@bosomt
Copy link

bosomt commented Sep 24, 2024

QA OK

Info:

  • Suite version: desktop 24.10.0 (55ac8cb8a4edc1ac546c163acb3482cf53c4e4f8)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.10.0 Chrome/124.0.6367.243 Electron/30.3.1 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.3 regular (revision 7f373ae)
  • Transport: BridgeTransport 2.0.33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translations Put this label on a PR to run tests in all languages
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants