Skip to content

Remove warning#18038

Merged
lukstbit merged 1 commit intoankidroid:mainfrom
Amit-kumar80844:removewarning
Mar 26, 2025
Merged

Remove warning#18038
lukstbit merged 1 commit intoankidroid:mainfrom
Amit-kumar80844:removewarning

Conversation

@Amit-kumar80844
Copy link
Contributor

Purpose / Description
Reducing Android Studio warnings by cleaning up unused code and fixing initialization.

Fixes
Contributes towards Cleanup Fix Android Studio Warnings

Approach
By using android studio suggestions and deleting never used variables or using suppression

How Has This Been Tested?

I run my code after change on emulator android 11 it runs as previous

Checklist
Please, go through these checks before submitting the PR.
You have a descriptive commit message with a short title (first line, max 50 chars).
You have performed a self-review of your own code

@welcome
Copy link

welcome bot commented Mar 1, 2025

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@Amit-kumar80844
Copy link
Contributor Author

can someone please review the changes and say why my changes got error

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thank you for contributing, I left some issues to fix. Also, please squash the code this should consist of only one commit.

can someone please review the changes and say why my changes got error

The test is failing because you introduced breaking changes to the code. See review comments.

* @see BrowserConfig.activeColumnsKey
*/

@Suppress("Unused", "KDocUnresolvedReference")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issues here shouldn't be suppressed, the proper imports should be added so the documentation references work. This mean importing the BrowserConfig and Backend classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import com.ichi2.libanki.BrowserConfig.ACTIVE_CARD_COLUMNS_KEY
import com.ichi2.libanki.BrowserConfig.ACTIVE_NOTE_COLUMNS_KEY
as both have no usage if i import it a new warning will appear that's why did not import it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but if you change the documentation to prefix the properties then it should work(I missed that the BrowserConfig class is already imported):

[BrowserConfig.ACTIVE_CARD_COLUMNS_KEY]
[BrowserConfig.ACTIVE_NOTE_COLUMNS_KEY]

and for the other two:

 * @see [Backend.setActiveBrowserColumns]
 * @see [BrowserConfig.activeColumnsKey]

after importing the net.ankiweb.rsdroid.Backend class. Suppressing is not the proper way to fix these.

@lukstbit lukstbit added Needs Author Reply Waiting for a reply from the original author squash-merge A squash & force push is required. The PR author may do this to speed up the merge process. labels Mar 2, 2025
@Amit-kumar80844 Amit-kumar80844 requested a review from lukstbit March 4, 2025 17:01
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, also please squash the commits into a single commit for merging.

* @see BrowserConfig.activeColumnsKey
*/

@Suppress("Unused", "KDocUnresolvedReference")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but if you change the documentation to prefix the properties then it should work(I missed that the BrowserConfig class is already imported):

[BrowserConfig.ACTIVE_CARD_COLUMNS_KEY]
[BrowserConfig.ACTIVE_NOTE_COLUMNS_KEY]

and for the other two:

 * @see [Backend.setActiveBrowserColumns]
 * @see [BrowserConfig.activeColumnsKey]

after importing the net.ankiweb.rsdroid.Backend class. Suppressing is not the proper way to fix these.

@Amit-kumar80844 Amit-kumar80844 requested a review from lukstbit March 5, 2025 19:12
@criticalAY criticalAY removed the squash-merge A squash & force push is required. The PR author may do this to speed up the merge process. label Mar 6, 2025
Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really not easy to review as there are a lot of related changes together, and it's hard to know which part is related to what. For example, cliboard is removed. I don't know why. I guess it was not actually useful.

I'd help if you could split in multiple logical commit. For example one that remove everything related to clipboard, with a commit message stating that all of that was unused. Then a different commit (or even a different PR) that add the "unused" suppression.

For "unused", please always add a comment explaining why we need the function/class that appears unused. The warning is here to tell us something is strange. If it's strange but actually the right implementation, we really should explain why it is the case.
In every "unused" you added, I've no idea why they are needed. So I need you to do the research, probably looking at the history, and explain to us why those are needed.

Most importantly, I assume you're here for GSoC. If so, as a mentor, I should warn you I would not consider this kind of PR sufficient to consider your application. I always appreciate code cleaning. But it does not really allow me to check whether you know how to look at a bug, investigate it, correct it, ask question if you're stuck, the way solving a user facing issue or adding a feature help us evaluate you

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

david-allison

This comment was marked as resolved.

@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the Stale label Mar 25, 2025
Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author Stale labels Mar 25, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have squashed and force pushed this, let's get it in

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok nice

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Mar 26, 2025
@lukstbit lukstbit added this pull request to the merge queue Mar 26, 2025
Merged via the queue into ankidroid:main with commit 76479ab Mar 26, 2025
9 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Mar 26, 2025
@github-actions github-actions bot added this to the 2.21 release milestone Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants