Add comment to core #17107
Add comment to core #17107
Conversation
The main goal of this PR is to ensure that all strings get a comment. While I used to naively think that most string were clear, and that translators would be able to figure out what they were translating, I realized, looking at the French translation, that it's not actually the case. Instead of just fixing the French translation, I prefer to improve the process so that AnkiDroid can be correctly translated in all languages. In particular, some strings are used in multiple context. E.g. as a button and the title of the dialog/view opened by this button. In English, it's certainly correct. In French it usually is too. I have no idea whether it's correct in all other languages. By letting the translators know that the strings is used in both context, they can provide feedback if ever they need us to split the string in two. As an example, I recently split "Search" in two strings, one for the verb and one for the noun "search", because, in French, we need to use distinct words to consider the search action, and the content of the search request. I should note that this task requires to look at all the strings, and look for all its usage in the codebase. Which means this task can only be done by a developer that knows the codebase and the app enough to understand all usages of each string. Worse, either reviewing such changes will be very painful, or the reviewer will have to have a high trust in the person doing this work. Which means, and I'm quite sad about it, that it can't really be delegated. As a secondary goal, the commit reorders the string, so that they strings appearing in the same view get shown together. This is not probably not important for translators, but I find it cleaner. I don't exactly expect this PR to be merged as-is. I mostly open it to gather feedback. If you are against this change (e.g. because it'll destroy the history of all our string files), I can stop and not spend more time on it. Or, if you have any idea of a better way to write comment, I can use your idea in the future comments. Once we agree this work is worthwhile and can eventually be merged, if you find typos, gramattical errors, or general improvement, I'd appreciate if you could directly push the correction as an additional commit, or provide the correction as a suggestion in github review system. Because, discussing every comment individually would be a nightmare of bikeshedding
|
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
|
Can you run https://github.com/ankidroid/Anki-Android/actions/workflows/compare_apk_size.yml on your branch? |
|
I tried. More on https://github.com/Arthur-Milchior/Anki-Android/actions/runs/10981505951/job/30488613056 I started the run on AnkiDroid's main branch just to check whether it's a problem specific to my repo, or whether the action is broken. Honestly I've no idea what it means |
|
Instead I used Android-Studio APK analyzer. I looked at the size of fullDebug at origin/main ( 4eab9d1 ) (I assume release is more optimized, but I fail to build it, it seems it's a key issue. Both version have 36.6 estimated download size. What makes no sense at all is that my commit REDUCE the size by 27.4kb according to the tool to compare APK. 23.7kb in clasess16.dex, 1.9 in classes13.dex and 1.8 in classes17.dex. I double checked there is really no other commit in difference than this very single commit. I honestly don't know what to make with this information. |
|
I just tested removing all comment in 01-core. It saves 4.7kb compared to this commit. So yeah, comment take space in the debug version. I'll need to figure out how to check the optimizd version |
|
Have you tried decompiling a release APK and checking the strings files? |
|
Creating a release APK requires a signing key and it's a pain in the butt to set up which is why I was hoping to automate it fully here on every PR, or at least make it on demand (which is what the apk size calculator is supposed to do) so people don't have to go through the pain of getting release builds working locally. I'm looking in to the apk size calculator and I'll fix it I can't believe this would actually cause a real change in release mode but ... I could be surprised |
|
Old APK size: 36215049 |
|
@david-allison I had not. I just downloaded a release apk from github. And used
I could not find any trace of the comment
Of course, it just mean that it's not outputed by this aapt2 command. I fail I'm not competent enough to ensure it's not anywhere else in the binary. I understand where you request come from, David. Really, I do. Still, I must admit I have a hard time realizing that improving the translation process requires to learn about decompilation tools. |
|
@mikehardy I assume you were the one who cause the github-action message to appear. Can you please explain how you did it? |
|
The workflow posts the comments |
|
You may alter the PR and rerun the workflow for experiments tho |
There was a problem hiding this comment.
After a partial review, I don't find this level of verbosity to be useful. Some context is useful. Screenshots definitely are.
Listing what appears to be every use of each string means we're going to get significant documentation churn for little benefit
Especially as we don't have any mechanism to ensure that this permanently fixes a problem
| <string name="drawing">Drawing</string> | ||
| <string name="send_feedback">Send feedback</string> | ||
| <string name="decks" comment="The entry in the navigation drawer that leads to the Deck Picker">Decks</string> | ||
| <string name="card_browser" comment="The entry in the navigation drawer that leads to the Card Browser. Also the name of the Android Shortcut that open the card browser. Also the title of the Card Browser page.">Card browser</string> |
There was a problem hiding this comment.
"Opens the card browser"
Aside: at some point we'll want to rename the card browser, it can now be used to view notes
| <string name="send_feedback">Send feedback</string> | ||
| <string name="decks" comment="The entry in the navigation drawer that leads to the Deck Picker">Decks</string> | ||
| <string name="card_browser" comment="The entry in the navigation drawer that leads to the Card Browser. Also the name of the Android Shortcut that open the card browser. Also the title of the Card Browser page.">Card browser</string> | ||
| <string name="statistics" comment="The entry in the navigation drawer that leads to the Card Browser. Also the title of the Statistics page.">Statistics</string> |
There was a problem hiding this comment.
Leads to the card browser?
| <string name="card_browser" comment="The entry in the navigation drawer that leads to the Card Browser. Also the name of the Android Shortcut that open the card browser. Also the title of the Card Browser page.">Card browser</string> | ||
| <string name="statistics" comment="The entry in the navigation drawer that leads to the Card Browser. Also the title of the Statistics page.">Statistics</string> | ||
| <string name="settings" comment="The entry in the navigation drawer that leads to the settings. Also the title of the settings page.">Settings</string> | ||
| <string name="help" comment="The entry in the navigation drawer that open the help menu. Title of the help menu. Tooltip on top of ? that provides help. Buttons that offer help information.">Help</string> |
|
I'll answer to your main feedback first, because if we end up not merging this PR, looking at your comment on individual string seems useless.
I'm not trying to get every use. I want to note when something is used as a button, as a title, and so on. But if a button or a menu entry appears in a lot of view, I don't intend to list all of the views it appears in. In particular it means that any string that is reused require to change the documentation only if it's a new kind of usage. I would expect most string are used only once and the comment will never have to be changed.
So, I'll totally agree that it's significant churn. And I'm very sorry for it, I would wish that it's not the case. However, I'd note that taking screenshot is even more churn and adding them to crowdin requires even more work, and is even harder to ensure to be up to date. If we have a string that is used only as a button label, and suddenly it start being used as a title, indeed, it causes churn, but I believe this churn is actually useful for translators. (Admittedly, it'll only be useful for new translations. As it won't cause translators to check past translations) The issue is that crowdin will not tell us there is no screenshot that shows the string being use as a title, and so we are at risk of never uploading this screenshot. While it would be easier for a reviewer to check whether a string reused is consistent with the comment.
There is the review process. Same as with code. I admit it'd be nice to have CI pipeline, but I fear I don't really see how it could be done. I'd even state that, currently, with 360 strings that has no screenshot on crowdin, it seems clear to me that the current process does not work. And just taking screenshot to cover most of those strings will also require significant work. And require to re do this work regularly. I'm not stating that what I suggest here is perfect. Just that it's better than what we currently have, and probably easier to keep some standard of quality. It won't be perfect, it won't remain perfect. It'll just be better than current state of affairs. |
|
@mikehardy my question was really just about using the workflow. I was not asking you to do any work yourself appart from this explanation. |
david-allison
added
Needs Author Reply
|
Awaiting a third opinion: https://discord.com/channels/368267295601983490/1208972284124209282/1293663771037925517 |
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
I guess we won't have a third opinion. |
|
Definitely! I feel one or two examples would be a great improvement |
|
One or two example of what? I'm sorry but I don't understand your last answer |
|
One or two examples of the usage of a string is great (realistically they should be on CrowdIn as screenshots instead, but that's a future goal). More examples rarely add value, causing wasted time changing the string comment, and using up the time of reviewers (+dev cycle time) and translators on CrowdIn |
The main goal of this PR is to ensure that all strings get a comment. While I used to naively think that most string were clear, and that translators would be able to figure out what they were translating, I realized, looking at the French translation, that it's not actually the case.
Instead of just fixing the French translation, I prefer to improve the process so that AnkiDroid can be correctly translated in all languages.
In particular, some strings are used in multiple context. E.g. as a button and the title of the dialog/view opened by this button. In English, it's certainly correct. In French it usually is too. I have no idea whether it's correct in all other languages. By letting the translators know that the strings is used in both context, they can provide feedback if ever they need us to split the string in two.
As an example, I recently split "Search" in two strings, one for the verb and one for the noun "search", because, in French, we need to use distinct words to consider the search action, and the content of the search request.
This problem would not even be solved by looking at the screenshot. Indeed, while it's easy to see which strings has no screenshot, it's not possible to state which usage is not screenshotted. So the translator may not have seen that "search" was used in two distinct contexts.
I should note that this task requires to look at all the strings, and look for all its usage in the codebase. Which means this task can only be done by a developer that knows the codebase and the app enough to understand all usages of each string. Worse, either reviewing such changes will be very painful, or the reviewer will have to have a high trust in the person doing this work. Which means, and I'm quite sad about it, that it can't really be delegated.
As a secondary goal, the commit reorders the string, so that they strings appearing in the same view get shown together. This is not probably not important for translators, but I find it cleaner.
I don't exactly expect this PR to be merged as-is. I mostly open it to gather feedback. If you are against this change (e.g. because it'll destroy the history of all our string files), I can stop and not spend more time on it.
Or, if you have any idea of a better way to write comment, I can use your idea in the future comments.
Once we agree this work is worthwhile and can eventually be merged, if you find typos, gramattical errors, or general improvement, I'd appreciate if you could directly push the correction as an additional commit, or provide the correction as a suggestion in github review system. Because, discussing every comment individually would be a nightmare of bikeshedding