search

RedApparat / Fotoapparat

Making Camera for Android more friendly. 📸
Apache License 2.0
3.82k stars 405 forks source link

Suggestion: stop() should not throw RuntimeException if camera is not started #152

Closed siralam closed 6 years ago

siralam commented 6 years ago

This is because when we ask for runtime permission (before calling start()), onPause() will be triggered (to show the permission dialog).

Therefore, of course, in such case, camera has not been started. That means you forces your users to try catchstop() inside onPause() I think this is not a good design.

If your code checked that camera has not been started when calling stop(), why not simply return?

Diolor commented 6 years ago

You should not call start if you don't have permissions

siralam commented 6 years ago

No, you didnt get me correctly. Of course we should not call start() without permission. But before gaining permission, onPause() will be called and therefore stop(). Because start() is not called, stop() throws an exception, which makes little sense.

dmitry-zaitsev commented 6 years ago

@siralam we are actually calling camera methods inside onStart and onStop and do an extra check for the permissions.

As for the exception - I think we might actually remove it from stop() and make it no-op in case if camera was not started. @Diolor what do you think?

Diolor commented 6 years ago

TL:DR; You should not call stop() either if you have no permission. See sample.

In general you should not interact with FA & camera if you don't have permissions. Regarding stop being partially no-op: will make the code less cryptic and less easy to notice bugs.