Cap-go / capacitor-flash

Switch the Flashlight / Torch of your device.
https://capgo.app
13 stars 4 forks source link

isAvailable always returning false while I have a flash #2

Closed sirgatsen closed 2 years ago

sirgatsen commented 2 years ago

Hi @riderx I checked out your plugin and found an issue where the isAvailable() function is always returning false while I do have a flash on my phone.

Steps to reproduce.

  1. Create a new blank capacitor project with angular
  2. Add Android platform
  3. Install the plugin and sync
  4. Try calling isAvailable() after platform.ready()
  5. Test on android device with a flash

Please see test repo that I created for this.

sirgatsen commented 2 years ago

Some other notes:

The plugin seems to work (activate the flash) without the defined permissions which is awesome because I believe the newer Android Camera API decouples the flash from the camera and defines it as an install time permission rather than a runtime permission. Thus the camera permission is only required by sdk level 23 and below so the documentation could be improved to indicate this like:

<!-- Permissions: Allows access to flashlight -->
<uses-permission android:name="android.permission.CAMERA" android:maxSdkVersion="23" />
<uses-permission android:name="android.permission.FLASHLIGHT" />

<!-- Actual Hardware Features Used-->
<uses-feature android:name="android.hardware.camera.flash" android:required="true" />

Also it would really be nice and convenient to have a built-in toggle() function

riderx commented 2 years ago

Thanks @sirgatsen for the report i will have check of your issue, i have update the doc with your feedback ! For toggle, i think i wasn't able to do it last time, but let me check again.

riderx commented 2 years ago

@sirgatsen i found your issue, i upgraded the code to fix it. Check the version 1.1.0 I also added the toggle feature, if you want to try it and give feedback, it work on my side.

Thanks a lot for providing a test repo, it helped a lot !

sirgatsen commented 2 years ago

@riderx Thank you so much, the fix and the toggle function work really well. That was a faster response than I expected and I am sorry I couldn't respond as fast.

While we are at it, I found another issue where the isSwitchedOn() seems to be returning false after using switchOn() while it works well after toggle()

You can check the test repo again for my updates. Hoping for another quick response

riderx commented 2 years ago

@sirgatsen i have check the code and i don't understand why this is doing that, i will check this week again

riderx commented 2 years ago

I found the issue switchOn was never calling the function to return to js. I have publish v1.1.1 who fix it, thanks again for the nice report !

@sirgatsen i was checking your js code again and saw something weird to me

    await CapacitorFlash.toggle().then(async () => {
      await CapacitorFlash.isSwitchedOn().then((result) => {
        this.lightOn = result.value;
      });
    });

Why do you await and use then ? To me when you use await you use this way:

    await CapacitorFlash.toggle();
    const result =  await CapacitorFlash.isSwitchedOn();
    this.lightOn = result.value;

And with then this way:

   CapacitorFlash.toggle().then(() => {
      CapacitorFlash.isSwitchedOn().then((result) => {
        this.lightOn = result.value;
      });
    });

i never used both. it's not related to the issue you shared but maybe interesting to discuss.

sirgatsen commented 2 years ago

Tested and all is well it seems, thanks again for the commitment to fixing issues.

I think the plugin is ready for production and community adoption now!

I actually recently released a Simple Flashlight app on Android based on the Cordova/PhoneGap plugin by EddyVerbruggen but the Google Pre-Launch report raised a lot of warnings about deprecated APIs and so I am looking to upgrade since I can also do away with the unnecessary camera permissions on newer devices.

With regard to the use of await and then together, you are right in that the await actually becomes rather redundant since the then block will only execute after the promise has completed. It is just a rather paranoid precautionary habit of mine incase I have some code following that then block that might attempt to use values set inside the then block. In such case without the await, the execution may proceed to the following code while the value from the then block might not be ready.

Since it doesn't really affect anything except maybe the semantics, I think it is a good habit to keep?

riderx commented 2 years ago

@sirgatsen super cool that fix your issue ! Thanks a lot for the feedback!

Hope to see your new app version with my plugin inside :)

Thanks for the clarification for await /then.

If you want to await the set in then, why not do the set outside of the then after wait ?

i have translate your code in await only to show you how i use to do it.

import { Component, OnInit } from '@angular/core';
import { Platform } from '@ionic/angular';

import { Toast } from '@capacitor/toast';

import { CapacitorFlash } from 'capacitor-flash';

@Component({
  selector: 'app-home',
  templateUrl: 'home.page.html',
  styleUrls: ['home.page.scss'],
})
export class HomePage implements OnInit {

  lightOn: boolean;

  constructor(private platform: Platform) {}

  async ngOnInit() {
    await this.platform.ready();
    // all code below will happen only after platform is ready
    const available = await CapacitorFlash.isAvailable();
    // auto switch on if available
    if(available.value) {
      await CapacitorFlash.switchOn({ intensity: 1 });
      const result = await CapacitorFlash.isSwitchedOn();
      this.lightOn = result.value;
      // all code below will happen only after the `lightOn` is set
    } else {
      await Toast.show({ text: 'Flashlight not available!' });
    }
  }

  async toggleLight() {
    await CapacitorFlash.toggle();
    const result = await CapacitorFlash.isSwitchedOn();
    this.lightOn = result.value;
    // all code below will happen only after the `lightOn` is set
  }

}

It seem more simple to read.

Since you didn't add catch, i did'n add too. But it could be good to catch in await way too, like that :

try {
    await CapacitorFlash.toggle();
    const result = await CapacitorFlash.isSwitchedOn();
    this.lightOn = result.value;
} catch (err) {
     await Toast.show({ text: err });
}

To me you don't need to be double safe :) Just unthen the code is enough and look more clear.

macordalfy commented 2 years ago

Im already using this https://github.com/capacitor-community/barcode-scanner however it does require a higher sdk, when is there a way to make this plugin work without the android:maxSdkVersion="23" in the manifest? .. it works if i put it but the camera stops working, and viceversa the flash stops working if i dont put it but the camera does .

riderx commented 2 years ago

@macordalfy hey sorry i don't see where you see the max version, can you share a link ?