oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
324 stars 523 forks source link

[BUG]: Fix all Android lint failures. #5169

Open seanlip opened 1 year ago

seanlip commented 1 year ago

Describe the bug

We would like to fix all the remaining lint issues on Oppia Android. There are currently 195 errors and 1009 warnings. Also, 38 checks are currently disabled.

We'd like to get rid of all these errors and then enable all the lint checks fully on CI (see https://github.com/oppia/oppia-android/issues/1742).

Steps To Reproduce

Run the following command from a terminal in a checked-out Oppia Android repo:

./gradlew :app:lint

Expected Behavior

Running the command above should not show any errors.

Screenshots/Videos

This is the first part of the error report:

Screenshot from 2023-10-02 00-44-03

Project Organization

This project can be divided into multiple parts. Please specify which part you want to take up and show a screenshot of a lint report with no errors for the corresponding issues in order to claim a part (and make a PR for it). Also, explain your general approach to fixing the errors for the part you claimed (don't just suppress the errors).

The relevant parts are listed below:

theMr17 commented 1 year ago
  • [ ] All warnings in "Accessibility" category

Hi @adhiamboperes, I would like work on this part. Under this category, the warnings are mainly of 3 types.

  1. Warning: Image without contentDescription. Fix: Add a contentDescription to each of those ImageView.

  2. Warning: 'onTouch' lambda should call 'View#performClick' when a click is detected Fix: Call customView.performClick( ).

  3. Warning: 'clickable' attribute found, please also add 'focusable'. Fix: Set android:focusable="false" as it is a button.

theMr17 commented 1 year ago
  • [ ] MissingDefaultResource + AppCompatCustomView + FragmentTagUsage

Hi @adhiamboperes, I would like to work on this next.

  1. MissingDefaultResource mainly has a single issue with a string which has different names in english and other translations. It can be easily fixed, by matching the name of that string in english and other translations.
  2. For AppCompatCustomView, it can be fixed by replacing the Normal views with AppCompat views
  3. <fragment/> tags can be replaced with <androidx.fragment.app.FragmentContainerView/>.
Rd4dev commented 1 year ago

Hi @adhiamboperes,

I am a new contributor to oppia-android and would like to start my contributions with the project. I've signed the CLA, filled out the survey form, and set up the environment locally. After some initial exploration, I believe I can assist with some lint issues.

I'd like to begin with the task related to All warnings in "Internationalization" While working on it locally, I encountered two types of issues:

Fix:

I have the code changes in my forked repository [Link] for your review and have attached before-and-after screenshots.

Screenshots: Lint Issues Internationalization Before After

If this solution aligns with the project's requirements, I'd appreciate it if you could assign the task to me so I can proceed with making a pull request.

Thank you!

Best regards, RD [Rama Devi]

sichchurov commented 1 year ago

InvalidPackage + NewApi + FragmentLiveDataObserve + RestrictedApi

Hi @BenHenning, I would like to work in this part. There is a compile-time error on line 231 in class ControlButtonsViewModel.

Call requires API level 26 (current min is 19): java.util.Base64#getEncoder

I can solve this error by:

  1. using if-else operator. If API 26 and higher use java.util.Base64 class else use android.util.Base64 class

    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
        Base64.getEncoder().encodeToString(compressedMessage)
    } else {
        android.util.Base64.encodeToString(compressedMessage, 0)
    }
  2. android.util.Base64 has encodeToString as equivalent of getEncoder().encode(args) it both Passing DEFAULT results in output that adheres to RFC 2045. So I can try to return right away:
    return android.util.Base64.encodeToString(compressedMessage, 0)
Rd4dev commented 1 year ago

Hi @adhiamboperes,

I would like to continue to work on - All warnings in "Usability:Typography", "Usability:Icons", and "Usability" categories task.

The issues can be categorized as:

  1. Borderless Buttons
  2. OK text capitalization
  3. Ellipsis/Hyphen unicode replacement
  4. Image in density drawable folder
  5. Text size too small
  6. To incorporate Autofill

Fix:

  1. Borderless Buttons: Added style="?android:attr/buttonBarButtonStyle" to implement borderless buttons as suggested.
  2. OK Text Capitalization: Capitalized the text in string resources, changing "ok" to "OK" for better consistency.
  3. Ellipsis/Hyphen Unicode Replacement: Used Unicode values to replace ... and - instances in Arabic translations, and added tools:ignore="TypographyDashes" where Unicode couldn't be applied.
  4. Image in Density Drawable Folder: Created and reallocated density images to appropriate folders such as
    • drawable-hdpi
    • drawable-mdpi
    • drawable-xhdpi
    • drawable-xxhdpi
    • drawable-xxxhdpi.
  5. Text Size Too Small: Increased text size to 12sp, as recommended, from the previous size of 10sp.
  6. Incorporating Autofill: Added autofill hint text to the Feedback Edit Text.

I have the code changes in my forked repository [Link1][Link 2] for your review and have attached before-and-after screenshots.

Screenshots Before After Usability Lint Issues If this solution aligns with the project's requirements, I'd appreciate it if you could assign the task to me so I can proceed with making a pull request.

Thank you!

Best regards, RD [Rama Devi]

sichchurov commented 1 year ago

Hi @adhiamboperes, I've found a solution of this issue: RestrictedApi

class AppCompatCheckBoxBindingAdapters has an error:

AppCompatCheckBox.setSupportButtonTintList can only be called from within the same library group prefix (referenced groupId=androidx.appcompat with prefix androidx from groupId=oppia-android)

The reason is using a private API. To resolve it, we can use:

CompoundButtonCompat.setButtonTintList(@NonNull CompoundButton button, @Nullable ColorStateList tint)

Here is a screen of lint report Before

restricted-api-before.png

After

restricted-api-after.png

Rd4dev commented 1 year ago

Hi @adhiamboperes,

I would like to continue along side on the - InconsistentLayout lint warnings

Cause: Some of the view items defined in the 'layout-sw600dp' configuration were not found in the main layout resource files. Lint warning: The id "id_name" in layout "resource_file" is missing from the following layout configurations:                         layout (present in layout-sw600dp)

Fix: Included the absent components in the primary resource layout file. I have the code changes in my forked repository Link for your review and have attached before-and-after screenshots.

Screenshots Inconsistent layouts oppia-android lint warning before after

If this solution aligns with the project's requirements, I'd appreciate it if you could assign the task to me so I can proceed with making a pull request.

Thank you!

Best regards, RD [Rama Devi]

deonwaju commented 1 year ago

Before Screenshot 2023-11-17 at 11 06 19 Updated @adhiamboperes

After Screenshot 2023-11-17 at 11 28 18

Hi @adhiamboperes, I would like work on this tasks

  1. Warning: VectorDrawableCompat Fix: A. Increase minimum API level to 21 or open the app's build.gradle file. Inside the android block, add or update the vectorDrawables section under defaultConfig:

android { ... defaultConfig { ... vectorDrawables { useSupportLibrary true } } }

Note: This fix also reduced the VectorRaster: from 62 to 18 warnings

  1. Warning: Addressing Typos from [Typos + StringFormatCount + UnusedQuantit + AllowBackup] Fix: Change "Ok" to both letters as caps "OK". Fix 2: corrected ratio_error_invalid_colons that had a repeated word "dois" in string resource file pt-rBR/strings.xml

  2. Warning: Addressing UnknownIdInLayout warning on the 13th issue on the list [DefaultLocale + OldTargetApi + UnknownIdInLayout + GradleDependency] Fix: Delete the reference that doesn't exist on line 88

Screenshot 2023-11-17 at 10 56 30

Updated @adhiamboperes

  1. Warning: Addressing DisableBaselineAlignment warning on the 13th issue on the list [DisableBaselineAlignment + InefficientWeight + Overdraw + UseCompoundDrawables + UselessLeaf + UselessParent]

Fix: (A) Add android:baselineAligned="false" to LinearLayout on line 12 in " layout-sw600dp-land/topic_info_fragment.xml" (B) Add android:baselineAligned="false" to LinearLayout on line 91 in " "layout/topic_lessons_story_summary.xml" (C) Add android:baselineAligned="false" to LinearLayout on line 93 in " "layout-sw600dp-land/topic_lessons_story_summary.xml" Example below: Before

Screenshot 2023-11-26 at 10 58 37

After

Screenshot 2023-11-26 at 10 58 53
  1. Warning: Addressing InefficientWeight warning on the 13th issue on the list [DisableBaselineAlignment + InefficientWeight + Overdraw + UseCompoundDrawables + UselessLeaf + UselessParent] Fix: adding width 0dp to Textview on line 31
Screenshot 2023-11-17 at 11 13 10

Please assign me these tasks if this solution meets the project requirements so I can move on with submitting a pull request.

Kind regards.

@adhiamboperes Please address from no. 2 to 5. Before

Screenshot 2023-11-26 at 12 04 52

After Screenshot 2023-11-26 at 12 11 26

adhiamboperes commented 1 year ago

@DeonWaju, you can work on this once your current issue is resolved and merged.

deonwaju commented 1 year ago

@adhiamboperes Ok, Thanks, I will as soon as it is.

deonwaju commented 1 year ago

Hi @adhiamboperes, Can I start work on this pending the time my request is resolved and merged?

deonwaju commented 1 year ago

Hello @adhiamboperes, I recently updated my solutions, Please help go through them again So I can create a PR.

adhiamboperes commented 1 year ago

Hello @adhiamboperes, I recently updated my solutions, Please help go through them again So I can create a PR.

@DeonWaju, I'm confused because a part of the updated comment seems to be addressing somewhat different warnings than the original comment.

For vector drawable and implied quantity sections, I already created separate issues and mentioned you there. Any further discussions for those specific scenarios will best discussed in the respective issue threads. Also reference those issues when creating a PR.

For additional segments of this main issue that you want to work on, please clarify which item from the list it belongs to so I can generate a separate issue for it. For example, the 'Typos' that you mention in your previous comment, isn't an item on the list, so I'm unsure of which item you are referring to.

deonwaju commented 1 year ago

Hello @adhiamboperes, I recently updated my solutions, Please help go through them again So I can create a PR.

@DeonWaju, I'm confused because a part of the updated comment seems to be addressing somewhat different warnings than the original comment.

For vector drawable and implied quantity sections, I already created separate issues and mentioned you there. Any further discussions for those specific scenarios will best discussed in the respective issue threads. Also reference those issues when creating a PR.

For additional segments of this main issue that you want to work on, please clarify which item from the list it belongs to so I can generate a separate issue for it. For example, the 'Typos' that you mention in your previous comment, isn't an item on the list, so I'm unsure of which item you are referring to.

Ok, sorry about the confusion, still getting the hang of this. I will write my fixes on those issues you created, and also for "Typos" I mentioned, I saw it on the list along with " Typos + StringFormatCount + UnusedQuantit + AllowBackup" the 11th item on the list in the description of this PR.

Regards

deonwaju commented 1 year ago

Hello @adhiamboperes, please help review my solution from 2 to 5 so i can work on them if its fine by standards.

deonwaju commented 1 year ago

Hello @adhiamboperes

adhiamboperes commented 1 year ago

@DeonWaju, might I suggest that you split each of your numbered sections into a seperate comment in the future?
Re:

Warning: Addressing Typos from [Typos + StringFormatCount + UnusedQuantit + AllowBackup] Fix: Change "Ok" to both letters as caps "OK". Fix 2: corrected ratio_error_invalid_colons that had a repeated word "dois" in string resource file pt-rBR/strings.xml

There is a fix for ok button capitalization in https://github.com/oppia/oppia-android/pull/5196.

It would be great to provide a solutiion for the entire [Typos + StringFormatCount + UnusedQuantit + AllowBackup] Item so that I can convert it to one issue.

deonwaju commented 1 year ago

@DeonWaju, might I suggest that you split each of your numbered sections into a seperate comment in the future? Re:

Sure, thanks, I am learning everyday. I will split them as soon as i am done with the other issue you assigned to me.

deonwaju commented 12 months ago
  1. Fix part of [Typos + StringFormatCount + UnusedQuantit + AllowBackup] Fix for Typos: Change "Ok" to both letters as caps "OK" in the string resources affected and also correct ratio_error_invalid_colons that had a repeated word "dois" in string resource file pt-rBR/strings.xml
deonwaju commented 12 months ago
  1. Fix part of [DefaultLocale + OldTargetApi + UnknownIdInLayout + GradleDependency] Fix for UnknownIdInLayout: Delete the reference that doesn't exist in solution_summary.xml on line 88
deonwaju commented 12 months ago
  1. Fix part of [DisableBaselineAlignment + InefficientWeight + Overdraw + UseCompoundDrawables + UselessLeaf + UselessParent] Fix for DisableBaselineAlignment: (A) Add android:baselineAligned="false" to LinearLayout on line 12 in "layout-sw600dp-land/topic_info_fragment.xml" (B) Add android:baselineAligned="false" to LinearLayout on line 91 in " "layout/topic_lessons_story_summary.xml" (C) Add android:baselineAligned="false" to LinearLayout on line 93 in " "layout-sw600dp-land/topic_lessons_story_summary.xml" Example below: Warning: Addressing DisableBaselineAlignment warning on the 13th issue on the list [DisableBaselineAlignment + InefficientWeight + Overdraw + UseCompoundDrawables + UselessLeaf + UselessParent]
deonwaju commented 12 months ago
  1. Fix part of [DisableBaselineAlignment + InefficientWeight + Overdraw + UseCompoundDrawables + UselessLeaf + UselessParent] Fix for InefficientWeight: adding width 0dp to Textview on line 31
deonwaju commented 12 months ago

@adhiamboperes

aryanmishra29 commented 12 months ago

Hi @adhiamboperes, I am unable to repro the issue. When I run the specified command in the terminal in the checked-out repo of the project it shows some different errors but none specified in the issue itself. I am still fairly new to oppia-android's codebase, hence kindly help me repro this issue.

theMr17 commented 12 months ago

Hi @aryanmishra29, an easy way to repro the issue is to navigate to Code -> Inspect Code.

image

Then check on Whole Project and click on Analyze.

image

aryanmishra29 commented 12 months ago

Hi @adhiamboperes , there are 145 incomplete translations. I want to work on this issue and my methodology will be to add the strings in string.xml of that respective language by using Google Translate to translate from English to the language specified.

adhiamboperes commented 11 months ago

Hi @adhiamboperes , there are 145 incomplete translations. I want to work on this issue and my methodology will be to add the strings in string.xml of that respective language by using Google Translate to translate from English to the language specified.

Hi @aryanmishra29, we have a traslation team currently working on the translations, could you please pick another item here to work on?

pingu-73 commented 11 months ago

Hi @adhiamboperes, i would like to work on UnusedAttribute Warning is happening due to min. sdk version specified in the gradel.build is 19. We can change it to 29 and avoid those warnings. Before:

Screenshot 2023-12-14 at 1 53 24 PM

After:

Screenshot 2023-12-14 at 2 18 25 PM
adhiamboperes commented 11 months ago

Hi @adhiamboperes, i would like to work on UnusedAttribute Warning is happening due to min. sdk version specified in the gradel.build is 19. We can change it to 29 and avoid those warnings.

Hi @pingu-73, at the moment, we are unable to update the minSdk version. That removes support for older devices as well as will need a lot of work to verify compatibility with other libraries in the repo(we have a lot!). Could you please suggest another solution? For example, can you identify these unused attributes, and see if we can address each on a case by case basis?

deonwaju commented 11 months ago
  1. Fix part of [Typos + StringFormatCount + UnusedQuantit + AllowBackup] Fix for Typos: Change "Ok" to both letters as caps "OK" in the string resources affected and also correct ratio_error_invalid_colons that had a repeated word "dois" in string resource file pt-rBR/strings.xml

@adhiamboperes

pingu-73 commented 11 months ago

Hi @adhiamboperes, i would like to work on UnusedAttribute Warning is happening due to min. sdk version specified in the gradel.build is 19. We can change it to 29 and avoid those warnings.

Hi @pingu-73, at the moment, we are unable to update the minSdk version. That removes support for older devices as well as will need a lot of work to verify compatibility with other libraries in the repo(we have a lot!). Could you please suggest another solution? For example, can you identify these unused attributes, and see if we can address each on a case by case basis?

Hi @adhiamboperes Another Approach: we can add checks using dataBindings for example:

Screenshot 2023-12-14 at 3 58 00 PM Screenshot 2023-12-14 at 4 04 31 PM Screenshot 2023-12-14 at 3 55 20 PM
adhiamboperes commented 11 months ago

Hi @adhiamboperes, i would like to work on UnusedAttribute Warning is happening due to min. sdk version specified in the gradel.build is 19. We can change it to 29 and avoid those warnings.

Hi @pingu-73, at the moment, we are unable to update the minSdk version. That removes support for older devices as well as will need a lot of work to verify compatibility with other libraries in the repo(we have a lot!). Could you please suggest another solution? For example, can you identify these unused attributes, and see if we can address each on a case by case basis?

Hi @adhiamboperes Another Approach: we can add checks using dataBindings for example:

@pingu-73, this new approach sounds good to me. I have created the issue here: https://github.com/oppia/oppia-android/issues/5273