Skip to content

Conversation

ebariaux
Copy link
Contributor

No description provided.

@ebariaux ebariaux requested a review from Miggets7 May 28, 2025 08:54
Copy link
Contributor

@Miggets7 Miggets7 left a comment

Choose a reason for hiding this comment

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

Some minor things to be discussed.

Overall it looks very well structured!


@androidx.annotation.RequiresPermission(allOf = [android.Manifest.permission.ACCESS_FINE_LOCATION, android.Manifest.permission.BLUETOOTH_ADMIN, android.Manifest.permission.BLUETOOTH])
suspend fun searchESPDevices(devicePrefix: String): List<DeviceRegistry.DiscoveredDevice> {
return withContext(Dispatchers.Main) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do it on IO and when returning the result then on Main? Or doesn't that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how that would work, the callbacks are not suspended and I can't switch context there.
In the end, I'm not seeing any issue with dispatching this on the Main context, so keeping it like that.

suspend fun getDeviceInfo(): DeviceInfo {
val request = Request.newBuilder()
.setDeviceInfo(Request.DeviceInfo.getDefaultInstance())
.setId(messageId++.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to create a function for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why ?
I tried creating one, scoping it with the getDeviceInfo() function as it's only used there and thought it made the code less readable as we're defining a function and just calling it once the line below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be committed? I would say it wil be generated locally when working on the project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a bit but couldn't get the gradle build to work.
I will give it another go.

@ebariaux
Copy link
Contributor Author

Thanks, I'll take a look at those, maybe we can have a quick call next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants