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

System to pick a translation language for each keyboard #476

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Jag-Marcel
Copy link
Member

@Jag-Marcel Jag-Marcel commented Jul 26, 2024

Contributor checklist


Description

This PR creates a menu for each keyboard, where you can choose a language to be used for Scribe's translation feature. Previously, every keyboard would require English as the translation language.

Related issues

Blocked by

Removed unnecessary function from project and simplified a switch case:
Are these required or is it fine to remove them?
- Minor refactor to code for the button
- Reworked how the chevron is displayed on the button
- Added a label to the left of the chevron
- Resized elements of the button to be centred instead of slightly lower than the centre
- Added a button to language settings for picking a translation language
- Created temporary value to store currently selected translation language, the plan is for every keyboard to have one of these
+ in progress class for the new view
- Selection now changes translationLanguage variable
- Radio icons change based on selection
- Now saves a specific translation language for each keyboard instead of one value
- Simplified saving the variables for each language using UserDefaults
- Translation query now asks for the controller language's specified translation language or sets a default if it's missing
- Removed some unnecessary code
Copy link

github-actions bot commented Jul 26, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and iOS rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-iOS repo
  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis self-requested a review July 26, 2024 16:25
Jag-Marcel and others added 5 commits July 26, 2024 18:33
cd28e5e Merge pull request scribe-org#25 from weblate/weblate-scribe-scribe-i18n
dc922d8 Translated using Weblate (Swedish)
997460b Translated using Weblate (Spanish)
a99deb4 Translated using Weblate (German)
eff6dee Merge pull request scribe-org#24 from Jag-Marcel/new-strings
7ad62b4 Merge branch 'main' into new-strings
b020823 Merge pull request scribe-org#23 from weblate/weblate-scribe-scribe-i18n
463dd85 New strings for Scribe-iOS translation picker
dde9e54 Translated using Weblate (Swedish)
65d4274 Translated using Weblate (Swedish)
f2c989d Translated using Weblate (Swedish)
d230853 Merge pull request scribe-org#22 from Jag-Marcel/minor-fix
dc219d4 Minor fix to conversion script

git-subtree-dir: Scribe/i18n
git-subtree-split: cd28e5e4ad8f50026e6e4d5b63e489819152e1e6
d9b2226 Merge pull request scribe-org#26 from Jag-Marcel/action-fix
0cb65ed Small fix to action for converting jsons to xcstrings

git-subtree-dir: Scribe/i18n
git-subtree-split: d9b22262f3769545a08bd4772cae20caa2f23c67
@andrewtavis
Copy link
Member

Thanks for the detailed PR and fixing the merge conflicts, @Jag-Marcel!

@andrewtavis
Copy link
Member

Brought in the localizations to main so that we can get v3.1 out, @Jag-Marcel 😊 Would be great if we can get a "Follow us on Mastodon" localization key in! I'll bring in everything else and send along v3.1 now though as I'd like to get it out today. No stress on one menu option not being fully localized :)

@Jag-Marcel Jag-Marcel mentioned this pull request Aug 3, 2024
1 task
@andrewtavis
Copy link
Member

@Jag-Marcel, the most recent commit adds in a table for each language with a word column and then ISO-2 code columns for each of the respective translation columns for the given language. There are a ton of weird conflicts here now... Let me know if you have thoughts on this would would like me to dive deeper into then! Generally I'd say we're keeping the .sqlite files that are in the branch and then we can update them again with the new data once I've ran all of the translation scripts. We should likely get rid of the changes to Scribe.xcodeproj/project.pbxproj and reimplement anything that's here as the changes to add in the English keyboard are likely more involved than the changes for the branch. I'm not sure on Keyboards/KeyboardsBase/Utilities.swift, but this is likely just some simple conflicts that can be resolved in the editor 😊 Happy to do a call to look into this!

Previous changes were broken because Utilities.swift was removed from several places as a source
Smaller fixes:
- Changed localization keys for new feature to use same ones as Scribe-i18n
- Removed extra English.entitlements in directory root
@Jag-Marcel
Copy link
Member Author

Should be done as soon as we have the full tables now!

@andrewtavis
Copy link
Member

Nice, @Jag-Marcel! This is amazing stuff here :) I'll do a review as soon as I can. Let me know if there's anything else you'd have interest in looking into over the last week 😊

@andrewtavis andrewtavis marked this pull request as ready for review August 19, 2024 13:18
@andrewtavis
Copy link
Member

@Jag-Marcel, playing around with it a bit, and let me just say that you really have a knack for user experience :) My thought was always that the user would select a new radio button and then go back to the settings via the back button, but then given the UI they will see the change in the interface option, so it makes total sense that we take them back directly. As with the commands on highlighted words, well done 👏

@andrewtavis
Copy link
Member

We do have some keyboard key spacing issues that are just the damn conditionals for language getting mixed as always. Might be something that we should consider setting up an enum for at some point, but as you can see there's a space being applied after the L that's bringing all the keys in to close with the gap to Ñ:

Simulator Screenshot - iPhone 15 Pro Max - 2024-08-19 at 20 21 35

This is the German keyboard with Spanish translation language :)

Padding code in general starts here.

For the German keyboard there are spacing issues for:

  • French (padding issues)
  • Russian (see image below)
  • Spanish (padding issues)
  • Swedish (padding issues)

Simulator Screenshot - iPhone 15 Pro Max - 2024-08-19 at 20 26 51

The issue above is more key size than padding, which is in KeyboardKeys.swift#L212. Might need to check these sorts of things for iPads as well.

Any thoughts on this? An enum would be ideal, or maybe a variable for the translation language itself so that we're not checking between what the keyboard language is and whether translation is in use (not what the translation language actually is).

@andrewtavis
Copy link
Member

andrewtavis commented Aug 19, 2024

I'm also a bit confused on the order of the translations. When I'm translating from Spanish -> German I'd expect to be able to do Brasil (a word that we did transfer that's in ESLanguageData) to Brasilien (the de key), but I'm getting Nicht in Wikidata like it's not a key. Are we using DELanguageData for this? I don't get Nicht in Wikidata for trying to translate 00 that's a word in DELanguageData which returns back 00 as it is in its en key, but then this should not be found in this case as it's not in ESLanguageData.

Ah I see what might be going on here (pardon the stream of conscious). We have it setup where it's from es in DELanguageData to word? It should be from word in ESLanguageData to de :) Reason for this is that we want there to be a unique result for any word that we'd return. In the machine translation process there are multiple words that might have been/might be translated to the same word, and then if we go from es to word in DELanguageData we'd then have multiple word options to choose from. Thinking of the Spanish word for over, sobre. As of now we've had 'nüber translate to it, and we can assume that über will to, but we wouldn't want to return the former. We want to translate sobre to über, have sobre be the word and über be the de key, and then have a unique result for each word that we're translating :)

Let me know if this makes sense! Hope I'm understanding what I'm seeing from the translation results 😊 Will look into the code a bit more later. Happy to discuss or hop on a call if need be!

@Jag-Marcel
Copy link
Member Author

I'll look into the first issue, maybe a function that returns the controller language's translation language is good? That could also simplify other places where it's used.
I'll change the way the translation tables are used so it works as intended as well.

@Jag-Marcel
Copy link
Member Author

maybe a function that returns the controller language's translation language is good?

Turns out that function already exists, I just forgot I made it at some point 🙃

@andrewtavis
Copy link
Member

Looking forward to the changes, @Jag-Marcel! :)

Mistook something in the re-activation of the English keyboard
- Refactored some logic to be more brief & consistent
- Added checks for the translation keyboard so spacing and key size is consistent with regular version of the keyboard
- Simplified code to use translation language code function
- Fixed a bug that crashed iPad version of the app
@Jag-Marcel
Copy link
Member Author

@andrewtavis just committed what I have for changing the database behaviour so far, but there seems to be a problem reading data from other databases. I added all the keyboards as memberships for each database, but in the reading process they seem to never find the word I'm looking for. Is there maybe something here I missed that makes the DBs show as empty unless they're used by the correct keyboard?

@andrewtavis
Copy link
Member

Hmmmm, ya this could prove to be a problem. Maybe the structure of the database is off? I understand your original inclination on this to use one DB for the whole thing rather than go across multiple, which also makes sense given downloading data in the future. As I'm thinking, we'd now need to bring down the ESLanguageData DB to translate to German, which is a bit weird, but I guess does make sense in so far as if the user has say German and French, then they'd just have the ESLanguageData for both :)

I'll take a look at this later!

How are we on the layouts?

Thanks for all the efforts, @Jag-Marcel!

@Jag-Marcel
Copy link
Member Author

The layouts should be done, I'll try looking into the DBs some more as well.
Maybe they don't get initialized quick enough for data to be read because of some async processes? We could also try putting translations for all languages in one separate database file

@andrewtavis
Copy link
Member

That's a thought as well, @Jag-Marcel! Could be something in the language keyboards base where we have all the tables in one names after their source language or something like that. If we can't figure out the current implementation, then I can rework the process :)

@andrewtavis
Copy link
Member

Very sorry for the wait on all this, @Jag-Marcel! Thanks so much for your patience :) d4e2b0a sends along new language DBs without a translation table, and then there's a common TranslationData.sqlite DB that we can now use for translation. This will be consistent even post Outreachy, and we won't need to worry about a single language DB being assigned anymore.

How do you want to proceed here? Would you want to finalize the new process via the SQLite access of the new TranslateData.sqlite DB? Anything I can do to support?

@Jag-Marcel
Copy link
Member Author

@andrewtavis I'll look into this and tell you if I need anything, glad that the process and everything is finally figured out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants