Conversation
Thank you for the pull request! 💙The Scribe-Android team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest that you use the Element client as well as Element X for a mobile app, and definitely join the Note Scribe uses Conventional Comments in reviews to make sure that communication is as clear as possible. |
Maintainer ChecklistThe 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 :)
|
|
Amazing to have a PR open for this so quickly, @catreedle! @DeleMike and @angrezichatterbox, let's try to review this before the sync on Saturday :) |
|
Sure, I will drop my review soon! |
|
thank you! looking forward to the feedback on this 😊 |
DeleMike
left a comment
There was a problem hiding this comment.
Hi, @catreedle, this is very good, and I think it is good to go for both "translate" and "plural" command states.
For "conjugate" command state, you said you needed assistance, and I have checked it. I noticed it does a pretty good job handling different states of the user's input. So all looks good here!
I logged some outputs from the conjugateManager, and the results coming from it are all lowercase, and it was able to produce outputs irrespective of how the word was formed.
Check Printing first 10 results for '{WORD}' --- to see the words I tried for English and German.
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D --- Printing first 10 results for 'pLaY' ---
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 1: Tense: Present | Pronoun: Pr. Simple | Form: play
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 2: Tense: Present | Pronoun: Pr. Simple | Form: plays
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 3: Tense: Present | Pronoun: Pr. Perfect | Form: have played
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 4: Tense: Present | Pronoun: Pr. Perfect | Form: has played
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 5: Tense: Present | Pronoun: Pr. Continuous | Form: am playing
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 6: Tense: Present | Pronoun: Pr. Continuous | Form: are playing
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 7: Tense: Present | Pronoun: Pr. Continuous | Form: is playing
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 8: Tense: Present | Pronoun: Pr. Perf. Continuous | Form: have been playing
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 9: Tense: Present | Pronoun: Pr. Perf. Continuous | Form: has been playing
2025-11-26 16:49:27.353 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 10: Tense: Past | Pronoun: Past Simple | Form: played
2025-11-26 16:49:56.669 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:56.670 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:56.670 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:56.672 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.938 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.938 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.938 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.939 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.992 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.993 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.993 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:58.993 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.760 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.760 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.760 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.761 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.774 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.774 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.775 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.775 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.776 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.776 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.776 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.776 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.780 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.781 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.781 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:49:59.781 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D --- Printing first 10 results for 'spielen' ---
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 1: Tense: Präsens | Pronoun: Präsens | Form: spiele
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 2: Tense: Präsens | Pronoun: Präsens | Form: spielst
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 3: Tense: Präsens | Pronoun: Präsens | Form: spielt
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 4: Tense: Präsens | Pronoun: Präsens | Form: spielen
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 5: Tense: Präsens | Pronoun: Präsens | Form: spielt
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 6: Tense: Präsens | Pronoun: Präsens | Form: spielen
2025-11-26 16:50:21.297 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 7: Tense: Preterite | Pronoun: Preterite | Form: spielte
2025-11-26 16:50:21.298 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 8: Tense: Preterite | Pronoun: Preterite | Form: spielten
2025-11-26 16:50:21.298 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 9: Tense: Preterite | Pronoun: Preterite | Form: spieltest
2025-11-26 16:50:21.298 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 10: Tense: Preterite | Pronoun: Preterite | Form: spieltet
2025-11-26 16:50:27.349 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:50:27.349 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:50:27.349 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:50:27.349 18068-18068 ≠ be.scri.debug I The current state is not conjugate view
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D --- Printing first 10 results for 'SpIEleN' ---
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 1: Tense: Präsens | Pronoun: Präsens | Form: spiele
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 2: Tense: Präsens | Pronoun: Präsens | Form: spielst
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 3: Tense: Präsens | Pronoun: Präsens | Form: spielt
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 4: Tense: Präsens | Pronoun: Präsens | Form: spielen
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 5: Tense: Präsens | Pronoun: Präsens | Form: spielt
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 6: Tense: Präsens | Pronoun: Präsens | Form: spielen
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 7: Tense: Preterite | Pronoun: Preterite | Form: spielte
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 8: Tense: Preterite | Pronoun: Preterite | Form: spielten
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 9: Tense: Preterite | Pronoun: Preterite | Form: spieltest
2025-11-26 16:50:48.151 18068-18068 CONJUGATE_DEBUG be.scri.debug D Result 10: Tense: Preterite | Pronoun: Preterite | Form: spieltet
However, I was just thinking, instead of passing the rawInput, we can convert it to lowercase(I tried it and it was the same experience as using rawInput without converting it to lowercase). The GeneralKeyboardIME.kt file isn't in the PR so I can't add a direct code suggestion. Check the handleConjugateState function in GeneralKeyboardIME.kt to normalise the rawInput to lowercase.
See suggested change. In the function, we should use searchInput instead of rawInput.
private fun handleConjugateState(rawInput: String) {
val searchInput = rawInput.lowercase()
// ...other section of the function
val tempOutput =
dbManagers.conjugateDataManager.getTheConjugateLabels(
languageAlias,
dataContract,
searchInput, // instead of rawInput earlier.
)
// other section of the function
conjugateLabels =
dbManagers.conjugateDataManager.extractConjugateHeadings(
dataContract,
searchInput, // also change here
)
}But apart from this, everything LGTM! 🎉✨
Well done again for this!
|
Such a nice review, @DeleMike 😊❤️ |
Thanks @andrewtavis 😄 |
|
Checking in here, @catreedle 👋 Would you be able to look into the changes that @DeleMike suggested? :) Please let us know if there's anything we can do to support! |
|
@catreedle, just a quick note as well that finalizing this PR with your initial outreachy tasks would be great, if possible 😊 |
|
Hi @andrewtavis @DeleMike, so sorry for missing this. looking into this soon 😊 |
No issues! 😊 I just checked and, I think all LGTM! 🎉 Can @angrezichatterbox verify if there is anything that needs to be improved for this PR? |
angrezichatterbox
left a comment
There was a problem hiding this comment.
Thanks for the PR @catreedle.
Functionality wise I feel everything is good. I have some suggestions on the code after which it would be good for merging.😊
app/src/main/java/be/scri/helpers/data/TranslationDataManager.kt
Outdated
Show resolved
Hide resolved
|
Would it be a good idea that now we start writing tests as well within the PR to introduce new functions like these which often get broken. We would slowly improve our coverage as well and not end up breaking this at any point. |
Thanks @angrezichatterbox ! I agree with you. We need to start writing tests, yes. @axif0 and I even had the same conversation regarding Scribe-Server. We want to reduce or eliminate any unexpected behaviour. What plans do you have for the test cases? |
|
I'd agree as well that it would be good to start testing the base behavior of the app. The plan was to focus on this post release, but I think another epic or two that focus on tests for specific parts would be a good idea. Maybe @catreedle can write tests for what she's adding, and others can focus on testing current functionality 🤔 |
|
Thank you for the feedback @angrezichatterbox, working on the suggestions 😊 |
angrezichatterbox
left a comment
There was a problem hiding this comment.
Everything looks good to me @catreedle
Great work 🎉
There was a problem hiding this comment.
praise: All's working very well here, @catreedle 😊 Well done! 🙌 Really appreciate the quick work and the testing of the solution 🚀
note: I think we need to look into the conjugations being returned also being capitalized if the entered text is capitalized. I'll make an issue for this and ping you all :)
Contributor checklist
./gradlew lintKotlin detekt testcommand as directed in the testing section of the contributing guideDescription
Related issue
This PR adds changes to Translate command and Plural command to normalize capitalized inputs to be recognized in the database, so it can return valid outputs. This also adds specific handling for German language, especially for its capitalized nouns.