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.

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

Fix all baselined lint issues #6921

Merged
merged 23 commits into from
Oct 8, 2024
Merged

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Oct 3, 2024


This change is Reviewable

Copy link

linear bot commented Oct 3, 2024

@Rawa Rawa marked this pull request as draft October 3, 2024 15:14
@Rawa Rawa force-pushed the fix-all-baselined-lint-issues-droid-218 branch from 417b001 to 4ba7447 Compare October 3, 2024 15:16
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 19 files at r1.
Reviewable status: 13 of 19 files reviewed, 2 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/extensions/ResourcesExtensions.kt line 15 at r1 (raw file):

}

@Suppress("MagicNumber")

Please add a comment regarding this suppression

Code quote:

@Suppress("MagicNumber")

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 77 at r1 (raw file):

    }

    @Suppress("ReturnCount")

Please add a comment regarding this suppression

Code quote:

@Suppress("ReturnCount")

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 19 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)

@Rawa Rawa self-assigned this Oct 4, 2024
@Rawa Rawa added the Android Issues related to Android label Oct 4, 2024
@Rawa Rawa force-pushed the fix-all-baselined-lint-issues-droid-218 branch 2 times, most recently from 5408d88 to cc9799e Compare October 7, 2024 10:47
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewed 25 of 25 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rawa)


android/app/src/main/AndroidManifest.xml line 30 at r3 (raw file):

                 android:banner="@mipmap/ic_banner"
                 android:allowBackup="false"
                 android:fullBackupContent="@xml/full_back_content"

Have we made sure we haven't accidentally changed the backup behaviour? We could potentially backup and send sensitive data.

@Rawa Rawa marked this pull request as ready for review October 7, 2024 10:52
Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/AndroidManifest.xml line 30 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Have we made sure we haven't accidentally changed the backup behaviour? We could potentially backup and send sensitive data.

According to the android documentation this should not cause any data transfers. I've explicitly excluded all domains that exists as options to backup.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/extensions/ResourcesExtensions.kt line 15 at r1 (raw file):

Previously, albin-mullvad wrote…

Please add a comment regarding this suppression

Reworked this to get rid of the magic number. Looked at the desktop implementation where the similar code exists but they in actuality never use the months. Also they only show years if it is more than 2, so I fixed that as well.


android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 77 at r1 (raw file):

Previously, albin-mullvad wrote…

Please add a comment regarding this suppression

Added comment and referenced a ticket.

@albin-mullvad albin-mullvad requested a review from Pururun October 7, 2024 15:23
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a few minor comments to address

Reviewed 25 of 25 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/res/xml/data_extraction_rules.xml line 21 at r3 (raw file):

        <exclude domain="external" />
    </device-transfer>
</data-extraction-rules>

bad eof


android/app/src/main/res/xml/full_back_content.xml line 16 at r3 (raw file):

    <exclude domain="device_sharedpref" />
    <exclude domain="device_root" />
</full-backup-content>

bad eof


android/lib/resource/build.gradle.kts line 24 at r3 (raw file):

    }

    lint {

Is this lint config needed when we use checkDependencies?


android/lib/resource/src/main/res/values/ic_banner_background.xml line 4 at r3 (raw file):

<resources>
    <color name="ic_banner_background">#192E45</color>
</resources>

bad eof


android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 78 at r3 (raw file):

    // DROID-1407
    // Function is to be cleaned up and lint warning to be removed.

Should it be lint suppression?

Code quote:

lint warning 

@Rawa Rawa force-pushed the fix-all-baselined-lint-issues-droid-218 branch from cc9799e to cf307bb Compare October 8, 2024 07:22
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 37 of 57 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Rawa)


android/app/src/main/AndroidManifest.xml line 30 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

According to the android documentation this should not cause any data transfers. I've explicitly excluded all domains that exists as options to backup.

Tested on Android 14 and nothing is backed up ⭐

Pururun
Pururun previously approved these changes Oct 8, 2024
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewed 20 of 20 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Rawa)

@Rawa Rawa force-pushed the fix-all-baselined-lint-issues-droid-218 branch from cf307bb to 24df46b Compare October 8, 2024 13:33
Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: 31 of 58 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Pururun)


android/app/src/main/res/xml/data_extraction_rules.xml line 21 at r3 (raw file):

Previously, albin-mullvad wrote…

bad eof

Done.


android/lib/resource/build.gradle.kts line 24 at r3 (raw file):

Previously, albin-mullvad wrote…

Is this lint config needed when we use checkDependencies?

As discussed, we leave them kept, since they are needed.


android/lib/resource/src/main/res/values/ic_banner_background.xml line 4 at r3 (raw file):

Previously, albin-mullvad wrote…

bad eof

Done.


android/app/src/main/res/xml/full_back_content.xml line 16 at r3 (raw file):

Previously, albin-mullvad wrote…

bad eof

Done.

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: 31 of 58 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Pururun)


android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 78 at r3 (raw file):

Previously, albin-mullvad wrote…

Should it be lint suppression?

Fixed.


android/lib/resource/src/main/res/values/ic_banner_background.xml line 4 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Done.

(Removed it and used other color asset)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 27 of 27 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


android/lib/resource/build.gradle.kts line 24 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

As discussed, we leave them kept, since they are needed.

👍

@Rawa Rawa merged commit 65bdbb5 into main Oct 8, 2024
32 checks passed
@Rawa Rawa deleted the fix-all-baselined-lint-issues-droid-218 branch October 8, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants