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

add new permissions from android API 31 #1642

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

Conversation

tomaszduda23
Copy link

Pull Request Guidelines for Bleak

Before you submit a pull request, check that it meets these guidelines:

  1. If the pull request adds functionality, the docs should be updated.
  2. Modify the CHANGELOG.rst, describing your changes as is specified by the
    guidelines in that document.
  3. The pull request should work for Python 3.8+ on the following platforms:
    • Windows 10, version 16299 (Fall Creators Update) and greater
    • Linux distributions with BlueZ >= 5.43
    • OS X / macOS >= 10.11
  4. Squash all your commits on your PR branch, if the commits are not solving
    different problems and you are committing them in the same PR. In that case,
    consider making several PRs instead.
  5. Feel free to add your name as a contributor to the AUTHORS.rst file!

@dlech
Copy link
Collaborator

dlech commented Oct 2, 2024

We've been discussing this in #1621.

My preference would be to move this to a separate function in addition to adding the require permissions for newer Android versions.

@tomaszduda23
Copy link
Author

We've been discussing this in #1621.

My preference would be to move this to a separate function in addition to adding the require permissions for newer Android versions.

done

Permission.ACCESS_COARSE_LOCATION,
Permission.ACCESS_BACKGROUND_LOCATION,
]
VERSION = autoclass('android.os.Build$VERSION')
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be consistent with other constants in the android backend, I would expect this to be defined in the defs module.

@@ -307,6 +307,15 @@ def set_scanning_filter(self, **kwargs) -> None:
"""
raise NotImplementedError()

@abc.abstractmethod
async def request_permissions(self, **kwargs) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this only applies to Android, I don't think we should make this a common function to all platforms.

Also, it is not specifically related to the BleakScanner, so should not be a method on that object.

Making it a standalone function in p2andoid.utils seems like the logical place to put it.

@@ -41,7 +41,9 @@ async def example(self):
while self.running:
try:
self.line("scanning")
scanned_devices = await bleak.BleakScanner.discover(1)
scanner = bleak.BleakScanner()
await scanner.request_permissions()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should only be called once at the start of the program. Also, it needs to be guarded with an if or in a try/except so that the program can run on platforms other than android.

scanned_devices = await bleak.BleakScanner.discover(1)
scanner = bleak.BleakScanner()
await scanner.request_permissions()
scanned_devices = scanner.discover(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

discover() is not an instance method, so we should not be creating a BleakScanner() instance.

BleakError("User denied access to " + str(permissions))
)
)
if not kwargs.get("permissions"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of kwargs, I would just make a named arg that defaults the default permissions.

@dlech
Copy link
Collaborator

dlech commented Oct 5, 2024

Also, we will need a changelog entry explaining that this is a breaking change.

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.

2 participants