Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

Lint: no Locale.ROOT in text user see

Open Arthur-Milchior opened this issue 4 years ago • 19 comments

It would be nice to add a lint rules that ensure that all text that user see is formatted with the local specified by Ressources. A recent PR added Locale.ROOT in a text that user could actually see, which should not occur.

It seems impossible to really define correctly in which case a text is user facing and in which case a text is internal. So maybe it should be a warning everything and suppress warning in acceptable uses. There are 26 uses right now

Arthur-Milchior avatar May 23 '21 06:05 Arthur-Milchior

I would like to work on this.

ShridharGoel avatar May 23 '21 14:05 ShridharGoel

No. There are cases the rule makes no sense, like the one Arthur is actually talking about. I don't like this rule.

mikehardy avatar May 23 '21 14:05 mikehardy

Ah wait - having had a cup of coffee (I just woke up) - even formatting nothing but a single number is important because periods and commas are used differently etc. I forget that sometimes with my US-centric brain.

mikehardy avatar May 23 '21 15:05 mikehardy

Ah wait - having had a cup of coffee (I just woke up) - even formatting nothing but a single number is important because periods and commas are used differently etc. I forget that sometimes with my US-centric brain.

I think this comment is meant for #8915.

ShridharGoel avatar May 23 '21 15:05 ShridharGoel

That comment was locale-specific, as a code rule, it's in the right spot. #8915 is to me more about formatting (spacing) and how to implement that locally so that you don't have to wait so much for CI to tell you that you missed something.

mikehardy avatar May 23 '21 16:05 mikehardy

@mikehardy I am really sorry, but I have no idea what you are suggesting. What is the path you want AnkiDroid to take?

  1. We don't automate anything and just try to catch errors in each PR
  2. we automate something, but something different than what I was asking for?
  3. we follow my suggestion?

This made me realize that my suggestion is actually not appropriate. I do not know what is appropriate but my suggestion would not have catched the original error until you added ROOT to it, which is clearly not what I want either. So I guess that what I really would have needed is to ensure that every single text shown to user either come from a ressource or is localized

Arthur-Milchior avatar May 24 '21 07:05 Arthur-Milchior

3 - we follow your suggestion

mikehardy avatar May 24 '21 13:05 mikehardy

Oh, and 3a. There was a lint rule that I followed while de-linting that was the proximate cause to this error, so it would catch it if we turned on both. I forget what the lint error was exactly but if you look at the code in the first de-lint transform it was this:

             browser.mActionBarTitle.setText(Integer.toString(browser.checkedCardCount()));

So whatever lint rule is triggered by doing setText on a text view in that style - turn that lint on and it will force you to use Locale's for transformation, then this lint check will force non-ROOT by default. There are very rare instances where you do want Locale.ROOT (I've needed it before and we may have one or more in AnkiDroid actually...) but then by default we will catch the rest and we can put in a suppression in for just the rare cases

mikehardy avatar May 24 '21 13:05 mikehardy

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

github-actions[bot] avatar Jul 23 '21 14:07 github-actions[bot]

Are you stil on it @ShridharGoel (I know there are other priorities. But might be nice to let other know if it's available)

Arthur-Milchior avatar Jul 25 '21 12:07 Arthur-Milchior

I'm not working on this. Anyone who might be interested can pick it.

ShridharGoel avatar Jul 25 '21 19:07 ShridharGoel

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

github-actions[bot] avatar Sep 23 '21 20:09 github-actions[bot]

I think this is actually a reasonable first issue, reopening and tagging as such

mikehardy avatar Oct 01 '21 13:10 mikehardy

Greetings, can I take it up?

vinaydeepkaur avatar Sep 12 '22 03:09 vinaydeepkaur

Sure thing! Cheers

mikehardy avatar Sep 12 '22 06:09 mikehardy

Can you please guide me what I need to do in this issue? I have learnt what lint rules are and how to implement them. I could locate the lint-rules directory in the code base. I am assuming I have to create a kotlin file in the lint directory and write a Detector class but I dont know what it has to detect. Am I thinking in the right direction? Can you please elaborate on what I should do or redirect me to some resources?

Thanks!

vinaydeepkaur avatar Sep 25 '22 23:09 vinaydeepkaur

i want to work in this issue please asign me this issue

muhib-siddique avatar Mar 20 '23 12:03 muhib-siddique

@mikehardy what's the status of this?

dae avatar Aug 31 '23 09:08 dae

Still needs an implementation and it would be useful as these are user-visible problems.

Solution is (based on above comments):

  • to enable a standard lint check where code is called with no Locale to make sure the example above would trigger a lint error
  • add a custom lint check where Locale.ROOT is flagged as an error, then any code correctly using Locale.ROOT (there are a few cases where Locale.ROOT is appropriate, but only a few!) suppresses the error in place

mikehardy avatar Aug 31 '23 16:08 mikehardy