Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove warning #18038

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove warning #18038

wants to merge 1 commit into from
+14 −34

Conversation

Amit-kumar80844
Copy link

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 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
Author

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

lukstbit
lukstbit requested changes Mar 2, 2025
View reviewed changes
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.

AnkiDroid/src/main/java/com/ichi2/anki/analytics/UsageAnalytics.kt
@@ -160,7 +160,7 @@ object UsageAnalytics {
Timber.d("getOptIn() status: %s", sOptIn)
return sOptIn
}
private set(optIn) {
set(optIn) {
Copy link
Member

Choose a reason for hiding this comment

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

Revert, this isn't a raised issue.

Copy link
Author

Choose a reason for hiding this comment

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

this was redundant

Copy link
Member

Choose a reason for hiding this comment

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

If it's not an issue then don't change it.

Copy link
Author

Choose a reason for hiding this comment

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

Redundant 'private' modifier as a warning

AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnCollection.kt Outdated
@@ -37,6 +37,8 @@ import timber.log.Timber
* @see Backend.setActiveBrowserColumns
* @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
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 The pull request currently requires maintainers to "Squash Merge" labels Mar 2, 2025
@Amit-kumar80844 Amit-kumar80844 requested a review from lukstbit March 4, 2025 17:01
lukstbit
lukstbit requested changes Mar 4, 2025
View reviewed changes
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.

AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
/**
* Returns the text stored in the clipboard or the empty string if the clipboard is empty or contains something that
* cannot be converted to text.
*
* @return the text in clipboard or the empty string.
*/
private fun clipboardGetText(): CharSequence {
Copy link
Member

Choose a reason for hiding this comment

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

Also delete the documentation above for this method.

Copy link
Author

Choose a reason for hiding this comment

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

ok

AnkiDroid/src/main/java/com/ichi2/anki/ui/DoubleTapListener.kt Outdated
@@ -13,9 +13,7 @@
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Please don't change random spaces.

Copy link
Author

Choose a reason for hiding this comment

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

i reverted it

AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnCollection.kt Outdated
@@ -37,6 +37,8 @@ import timber.log.Timber
* @see Backend.setActiveBrowserColumns
* @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.

AnkiDroid/src/main/java/com/ichi2/anki/analytics/UsageAnalytics.kt
@@ -160,7 +160,7 @@ object UsageAnalytics {
Timber.d("getOptIn() status: %s", sOptIn)
return sOptIn
}
private set(optIn) {
set(optIn) {
Copy link
Member

Choose a reason for hiding this comment

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

If it's not an issue then don't change it.

@Amit-kumar80844 Amit-kumar80844 force-pushed the removewarning branch 2 times, most recently from f8decc9 to 8d20df9 Compare March 5, 2025 18:46
@Amit-kumar80844
remove warning
9ee3ca9 
final change
@Amit-kumar80844 Amit-kumar80844 requested a review from lukstbit March 5, 2025 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukstbit lukstbit Awaiting requested review from lukstbit

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Needs Author Reply Waiting for a reply from the original author New contributor squash-merge The pull request currently requires maintainers to "Squash Merge"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Amit-kumar80844 @lukstbit