Skip to content

Conversation

barlin41k
Copy link

  • RecordService
    • handling SecurityException and check RECORD_AUDIO permission for AudioRecord initialization
    • check POST_NOTIFICATIONS permission before showing notifications
    • minor >= warning fix
  • RecordThread
    • remove extra indents

@gdzx
Copy link
Owner

gdzx commented Oct 1, 2025

Hi @barlin41k,

Thanks for the PR, but I'm not sure what issue it addresses.

Permissions are checked in MainActivity.java even before starting the service, so it shouldn't be necessary to check them again (unless I'm missing something).

Best regards,

@barlin41k
Copy link
Author

Hi @barlin41k,

Thanks for the PR, but I'm not sure what issue it addresses.

Permissions are checked in MainActivity.java even before starting the service, so it shouldn't be necessary to check them again (unless I'm missing something).

Best regards,

Hi @gdzx,

You're right, the required permissions already checks in MainActivity.java, so I ended up duplicating that logic.

That said, I think try/catch around AudioRecord is still useful because if the user revokes the permission after service has started, it would just crash. The POST_NOTIFICATIONS checks around notify don't really add any value, since calling notify() is safe without the permission. Those can definitely removed.

Best regards,

@gdzx
Copy link
Owner

gdzx commented Oct 2, 2025

If the user revokes the permission after service has started, it would just crash.

I don't think this is addressed by your change either, since after the service is started, the same AudioRecord instance is kept in the RecordThread (so it won't pass through your try / catch again).

There is a lot that could be improved in the handling of permissions (like having some UI that can ask the user to grant them, whereas currently the app just doesn't work), but I don't think this PR really improves the situation, sorry.

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