Merged
Conversation
5 tasks
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionItems.kt
Outdated
Show resolved
Hide resolved
david-allison
added
the
Needs reviewer reply
david-allison
added
Needs Author Reply
Member
|
Build fails |
david-allison
requested changes
Aug 3, 2025
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsItem.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsItem.kt
Show resolved
Hide resolved
BrayanDSO
requested changes
Aug 3, 2025
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsItem.kt
Outdated
Show resolved
Hide resolved
Contributor
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
david-allison
force-pushed
the
item
branch
2 times, most recently
from
645ed8c to
15a8821
Compare
There are multiple permission potentially
There was no reason to reset it each time a new listener is set. Admittedly it's only once in practice. I still think this make the code easier to read, and allows for more simplification.
It was only a setter, which can be public in Kotlin. Also, the `listener` is renamed to indicate exactly what is occurring. Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
david-allison
added
Needs Second Approval
david-allison
approved these changes
Aug 23, 2025
Member
david-allison
left a comment
david-allison
left a comment
There was a problem hiding this comment.
Rebased & applied change requests
BrayanDSO
approved these changes
Aug 23, 2025
BrayanDSO
added
Pending Merge
github-actions
bot
removed
the
Pending Merge
ericli3690
added a commit
to ericli3690/Anki-Android-ericli3690
that referenced
this pull request
Aug 27, 2025
GSoC 2025: Review Reminders Arthur pluralized most of the terms relating to permissionsItems at ankidroid#19033, but `permissionRequested` should become `permissionsRequested` in keeping with his changes. Additionally, the permissions explanation page which is coming in a later commit needs to provide the ability to ungrant permissions, which means it needs to know, when permissionsRequested is called, if the permissions are already granted or not. Hence, here I make permissionsRequested take a boolean argument, providing whether the permissions are already granted.
ericli3690
added a commit
to ericli3690/Anki-Android-ericli3690
that referenced
this pull request
Aug 27, 2025
GSoC 2025: Review Reminders Arthur pluralized most of the terms relating to permissionsItems at ankidroid#19033, but `permissionRequested` should become `permissionsRequested` in keeping with his changes. Additionally, the permissions explanation page which is coming in a later commit needs to provide the ability to ungrant permissions, which means it needs to know, when permissionsRequested is called, if the permissions are already granted or not. Hence, here I make permissionsRequested take a boolean argument, providing whether the permissions are already granted.
5 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 28, 2025
GSoC 2025: Review Reminders Arthur pluralized most of the terms relating to permissionsItems at #19033, but `permissionRequested` should become `permissionsRequested` in keeping with his changes. Additionally, the permissions explanation page which is coming in a later commit needs to provide the ability to ungrant permissions, which means it needs to know, when permissionsRequested is called, if the permissions are already granted or not. Hence, here I make permissionsRequested take a boolean argument, providing whether the permissions are already granted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing @ericli3690 PR, I found many things that I thought could be cleaned in the PermissionItem. Strating with the fact that, as there may be multiple permissions, it should use plural. And that the listener code was tool complex for what it actually does.
I hesitated to create a PermissionsItem.Delegate interface, and replace the
listenervariable by adelegatevariable. I decided against it, I don't think it would have added a lot of clarity