Add comment to core

[Arthur-Milchior]

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

[github-actions[bot]]

Message to maintainers, this PR contains strings changes.

1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

• localization-administration
• download-localized-strings

[david-allison]

Can you run https://github.com/ankidroid/Anki-Android/actions/workflows/compare_apk_size.yml on your branch?

[Arthur-Milchior]

@david-allison

I tried. And got

FAILURE: Build failed with an exception.

* Where:
Initialization script '/home/runner/.gradle/init.d/gradle-actions.build-result-capture.init.gradle'

* What went wrong:
Could not compile initialization script '/home/runner/.gradle/init.d/gradle-actions.build-result-capture.init.gradle'.
> startup failed:
  General error during semantic analysis: Unsupported class file major version 65
  
  java.lang.IllegalArgumentException: Unsupported class file major version 65
  	at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:196)
  	at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:177)
  	at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:163)
  	at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:284)
  	at org.codehaus.groovy.ast.decompiled.AsmDecompiler.parseClass(AsmDecompiler.java:81)
  	at org.codehaus.groovy.control.ClassNodeResolver.findDecompiled(ClassNodeResolver.java:251)

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

[Arthur-Milchior]

Instead I used Android-Studio APK analyzer. I looked at the size of fullDebug at origin/main ( 4eab9d13d65d0fe5947140a35455db49f6e130af ) (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 assume that adding comment don't reduce the size. maybe the order in which strings are defined matter. Some alignment issue. I really hope it's not the case and that we don't need to consider the order of the strings

[Arthur-Milchior]

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

[david-allison]

Have you tried decompiling a release APK and checking the strings files?

[mikehardy]

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

[github-actions[bot]]

Old APK size: 36215049 New APK size: 36215063

[Arthur-Milchior]

@david-allison I had not. I had never done it before, so I took some time to look at it. After all, it may be interesting knowledge.

I just downloaded a release apk from github. And used ./aapt2 dump strings AnkiDroid-2.19beta2-full-universal.apk|less `. I could find

String #6144 : Inaccessible collection

I could not find any trace of the comment

Dialog title if AnkiDroid can't access the collection once the app is installed

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.

[Arthur-Milchior]

@mikehardy I assume you were the one who cause the github-action message to appear. Can you please explain how you did it? There is a change of 14 bytes. I suspect, but can't prove it, that it's caused by reordering the string. I'd like to be able to run the test myself to ensure that it's indeed string order, or whether we actually have a cost of 14 bytes due to new comments.

[mikehardy]

The workflow posts the comments With apologies I'm spending zero time on 14bytes I always see small perturbation like that and assume it's Sha and timestamp related

[mikehardy]

You may alter the PR and rerun the workflow for experiments tho

[Arthur-Milchior]

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.

Listing what appears to be every use of each string

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.

get significant documentation churn

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.

Especially as we don't have any mechanism to ensure that this permanently fixes a problem

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.

[Arthur-Milchior]

@mikehardy my question was really just about using the workflow. I was not asking you to do any work yourself appart from this explanation. I agree that those 14 bytes are not something I'd even consider to spend time on

[david-allison]

Awaiting a third opinion: https://discord.com/channels/368267295601983490/1208972284124209282/1293663771037925517

[github-actions[bot]]

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

[Arthur-Milchior]

I guess we won't have a third opinion. For the sake of understanding exactly what you want, David, would you have something intermediate between my comment and your suggestion you'd accept. I still believe that letting the translators know when a string is used in different context is important. And crowdin's screenshot system can not warn us that we have forgotten to add screenshot for some usage. (for example, if a string is both a setting entry and a menu entry, we won't get warning that we only added a screenshot for the menu entry)

[david-allison]

Definitely! I feel one or two examples would be a great improvement

[Arthur-Milchior]

One or two example of what? I'm sorry but I don't understand your last answer

[david-allison]

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

[github-actions[bot]]

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

[Arthur-Milchior]

I hope I'm closer from what you want @david-allison. Not listing every usage of the strings. Still indicating when something is both used to perform an action and as a title/label. Still seems important, at least in French, to indicate that something can be an action and a label, as that restricts the acceptable translations.

[Arthur-Milchior]

I just tested. It cause a request for a new translation. Good catch. I tested in ankidroid to check the lines are stripped, and forgot to check about crowdin

[github-actions[bot]]

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

[david-allison]

I just tested. It cause a request for a new translation. Good catch. I tested in ankidroid to check the lines are stripped, and forgot to check about crowdin

What's the outcome here? Does this mean that all changed lines in core will need to be re-translated?

[github-actions[bot]]

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

[Arthur-Milchior]

I just tested. It cause a request for a new translation. Good catch. I tested in ankidroid to check the lines are stripped, and forgot to check about crowdin

What's the outcome here? Does this mean that all changed lines in core will need to be re-translated?

If you had merged the PR the way it used to be, with non trimmed string, it would have noted that the strings needed a new translation. However, it still would have used the previous translation and not the English one, and this would have been suggested as default translation to translator.

I reverted this change. Now, all strings are trimmed again. There is no string change, so there won't be request for new translations.

[david-allison]

However, it still would have used the previous translation and not the English one, and this would have been suggested as default translation to translator.

This wouldn't have been ok, not all of our translators are still active

I reverted this change. Now, all strings are trimmed again. There is no string change, so there won't be request for new translations.

Sounds good to me. Lint is failing, then we can merge this

[Arthur-Milchior]

However, it still would have used the previous translation and not the English one, and this would have been suggested as default translation to translator.

This wouldn't have been ok, not all of our translators are still active

Yes, it would not have been okay, I entirely agree.

I was not trying to state it was acceptable. I was trying to describe as best as I can the impact that this mistake would have had.

I reverted this change. Now, all strings are trimmed again. There is no string change, so there won't be request for new translations.

Sounds good to me. Lint is failing, then we can merge this

I don't see any failure right now.

[Arthur-Milchior]

Ping @david-allison Anything else you want me to change here? or can we merge or at least request a second review?