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

Add back English keyboard #484

Merged
merged 19 commits into from
Aug 16, 2024

Conversation

Jag-Marcel
Copy link
Member

@Jag-Marcel Jag-Marcel commented Aug 3, 2024

Contributor checklist


Description

This PR is meant to add the English keyboard back into Scribe, in preparation for 3.2. With #476, the translation function would also make sense now.

Related issues

Copy link

github-actions bot commented Aug 3, 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
Copy link
Member

Alright @Jag-Marcel :) Sorry that this took so long. You now have an English database on this branch 😊 It has the verbs, nouns and autosuggestions, but not anything else for now. We can run the process to make the emojis at some point before the release!

I've set aside some time in the sync to go through the minor changes for the verbs that I made while I was redoing the formatting process. Let me know if you have any questions!

@andrewtavis
Copy link
Member

Note also @Jag-Marcel that I've mapped out all of the conjugations in the table within the scribe-gsoc-sync-2024-8-3 notes 😊 Hope that's helpful to know what's going on!

@andrewtavis
Copy link
Member

FYI @Jag-Marcel you also now have the emoji data for English, so everything should be functional except for translation. Just need to clean up the code to generate the emojis, and then next translations 😊

@Jag-Marcel
Copy link
Member Author

@andrewtavis I think something got messed up with that last commit, I tried merging in the main branch to see if that fixed it but it's giving me the same results.
When I try to pull the changes, it's giving me these errors:

error: unable to read sha1 file of Keyboards/LanguageKeyboards/Italian/ITLanguageData.sqlite (77f0c8935af7180ebff4f38f4555c5860859beb8)
error: unable to read sha1 file of Keyboards/LanguageKeyboards/Portuguese/PTLanguageData.sqlite (971750e0ec034cf5e28508a4e160d2f9f2406361)
error: unable to read sha1 file of Keyboards/LanguageKeyboards/Russian/RULanguageData.sqlite (05cca6a4fcb107f26f69e766ce85769c9a1d17a6)
error: unable to read sha1 file of Keyboards/LanguageKeyboards/Spanish/ESLanguageData.sqlite (52c71cf704633d82ecfd2a72b98036ae7b410a49)
error: unable to read sha1 file of Keyboards/LanguageKeyboards/Swedish/SVLanguageData.sqlite (1a51208f893ca8a95bd4f507507a948f1f6a1914)
Updating 3e568ff..da5d357

In the commit itself it also seems most data got deleted:

image

@andrewtavis
Copy link
Member

I'm away from my personal computer, @Jag-Marcel, but if we just need the data back in we should be able to figure this out :)

Steps to follow:

  • Make sure that your local copies of Scribe-iOS and Scribe-Data are in the same directory, ideally scribe-org as maybe the transfer script's a bit weird
  • Pull Scribe-Data and make sure that Scribe-iOS is on your branch
  • Run the command in Scribe-Data/src/scribe_data/load/send_dbs_to_scribe.py
  • You'll then have the databases on your Scribe-iOS branch and can recommit them

Hopefully this is the full steps. I'll check in on Matrix as the notifications will come through there 😊

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

4c8fb19 makes the necessary changes to allow for setting the period and comma on the letter keys, @Jag-Marcel :) One thing to note, for conjugation it would be great if we'd present both options that are available in a following 1x2 view. You can see this in the case declension views specifically where when you press Dat after typing mit you'll see both options are at times shown to the user, which then indicates to them that they'll need to make a change. As of now when I conjugate Go I just see Go as the present form instead of Go / Goes. Once we have this in I'll give it one final review and then we'll be good to merge!

@Jag-Marcel
Copy link
Member Author

@andrewtavis does declension even work in the current state? I don't seem to get anything for non-verbs in the conjugation command, and when I tried pressing on mit in the autocompletions the German keyboard crashed.

@andrewtavis
Copy link
Member

We need to get tests at some point. Definitely something the iOS community should look into soon. On the positive side, it is working on the current version of the app:

image

Maybe you can check out why it's crashing?

@andrewtavis
Copy link
Member

There is some combination of declension and conjugation where some functions are shared and some aren't. Maybe one of the recent changes broke it? I didn't do the best job of reviewing as I forgot to check that declension was still functioning.

@Jag-Marcel
Copy link
Member Author

Jag-Marcel commented Aug 14, 2024

I'll look into it, but I also found another issue: The past perfect simple forms use the -ing forms of verbs instead of the participle (instead of "gone" in this example)
image
Only the past forms though.

@Jag-Marcel
Copy link
Member Author

Ok, declensions work now, it was the same issue I had earlier with emoji autosuggestions (26a0caa) the columns in the sqlite tables changed column names at some point and that wasn't reflected in the code yet

@andrewtavis
Copy link
Member

Fantastic, @Jag-Marcel, and ya I saw that that verb conjugation's all screwed up... Let me fix that really quick and you can pull in the new one :)

@andrewtavis
Copy link
Member

Ok, @Jag-Marcel, Scribe-Data now has the updated English DB with the correct version of the verbs :) Let me know if anything else is needed! Once we have the English keyboard in it's full speed ahead on finishing up cross language translation! Really appreciate all the efforts here 🙏😊

@Jag-Marcel
Copy link
Member Author

Alright, that should be everything now 👍

@andrewtavis
Copy link
Member

🔥🔥

Thanks @Jag-Marcel! I'll review this tonight :)

Do you want to hop over to the i18n changes now? Also let me know on Matrix if there's anything else you'd like to get done in the next week+ aside from the i18n refactor and translations 😊

@@ -250,7 +250,7 @@ extension LanguageDBManager {
word = ?
"""
let args = [word]
let outputCols = ["suggestion_0", "suggestion_1", "suggestion_2"]
let outputCols = ["autosuggestion_0", "autosuggestion_1", "autosuggestion_2"]
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch on these! I honestly don't remember when the change for this was made 🤔 Maybe is coming from changes in Scribe-Data's directory structure or something like that :)

@andrewtavis
Copy link
Member

dc554ed puts a space between conjugation table values for English and the participle where in some cases before there was just a slash. I think that we're fine with a space and the user will understand that the participle is just there for a backup so they don't need to do the command again. It also fixes a minor bug I found: That if you entered a capitalized plural word to the plural command it would show "Already plural" as expected, but then the returned word would be lower case as we lower cased it in the Plural command code to make sure it matches on the value in the DB. Something that hadn't been noticed when testing in German for obvious reasons 😇

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Thanks so much for the word to get issue number 🥁🥁🥁🥁 7 done, @Jag-Marcel! Totally amazing that this is on the way :) We'll need to update the media for the App Store so that we indicate that there are eight keyboards instead of seven 😊 I'll make that issue now.

CC @wkyoshida and @SaurabhJamadagni 🙌

@andrewtavis andrewtavis merged commit 60192f3 into scribe-org:main Aug 16, 2024
1 check passed
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