DantSu / ESCPOS-ThermalPrinter-Android

Useful library to help Android developpers to print with (Bluetooth, TCP, USB) ESC/POS thermal printer.
MIT License
1.22k stars 370 forks source link

Embedding Android permissions requests in the library #411

Closed KannarFr closed 1 year ago

KannarFr commented 1 year ago

Due to changes in permissions on Android 12 (https://developer.android.com/about/versions/12/behavior-changes-12#bluetooth-permissions) and Android 13 (https://developer.android.com/reference/android/Manifest.permission_group#NEARBY_DEVICES).

WDYT about directly embedding permissions management in this library?

DantSu commented 1 year ago

Because many people don"t want to use Bluetooth just only USB or LAN. Is not to the library to manage permission.

KannarFr commented 1 year ago

Then ask for permissions on related methods calls not everywhere?

DantSu commented 1 year ago

same answer, you may need Bluetooth for other task, not only for print.

It's a printer library, not permissions helper library, look on Github if an Android permission helper library exists.

KannarFr commented 1 year ago

same answer, you may need Bluetooth for other task, not only for print.

If the library checks Bluetooth permissions for only Bluetooth methods, the same for USB & TCP. Then it only requires permissions for specified features so I don't understand your answer.

It's a printer library, not permissions helper library, look on Github if an Android permission helper library exists.

Sure, but the library is not usable without permissions and it does not handle permissions errors so...

DantSu commented 1 year ago

... so it's not to this library to do permissions, you have to do by yourself....

DantSu commented 1 year ago

Ok je vois que tu es Français ça va être plus simple pour moi de s'expliquer.

Les permissions nécessitent un contexte donc ton activité, onRequestPermissionsResult est dans ton activité. Ma librairie est justement dépourvu de dépendance avec une quelconque activté pour pouvoir être executé partout sans se poser de question de contexte. D'ailleurs dans mes applications, je préfère demander les permissions bien en amont.

De plus les permissions sont aussi liées au AndroidManifest, si je met toutes les permissions requises dans l'AndroidManifest de la librairie (USB, BLUETOOTH, LAN) alors l'application qui utilise la librairie mentionnera qu'elle utilise toutes ces permissions alors que ce n'est pas le cas.

Ce n'est donc pas dans la librairie d'avoir les permissions, il faut le faire ailleurs, dans l'activité de l'application, c'est bien plus logique dans la conception d'une application.

DantSu commented 1 year ago

Après libre à toi de fork et de faire ce que tu veux avec la librairie, la licence MIT est justement là pour ça. Moi j'ai tripé et créé un parseur qui transforme un pseudo XML vers une imprimante thermale. Fais en ce que bon te souhaites ;)

KannarFr commented 1 year ago

Tout ça fait sens, et je te remercie du temps passé à me répondre. Néanmoins, je trouve dommage que la lib n'expose pas les permissions dont elle a besoin en fonction de la version d'Android, j'imaginais bien une méthod getPotentialPermissions(AndroidVersion) qui retourne la liste des permissions en mode Map[Mode, Set[Permission]], avec Mode une enum Tcp/Bluetooth/USB.

Mon but c'est pas tant de gérer les permissions dans la lib, ça tu as parfaitement expliqué pourquoi c'était naze et j'ai compris. Alors, je cherche plutôt comment éviter d'être stuck en dev sans aucune erreur lorsqu'il manque une permission. J'utilise une lib react-native qui utilise cette lib et elle gère les permissions je pensais que c'était cool de propager. D'ailleurs, elle a le contexte de l'activité via getCurrentActivity(). Mais je ne connais pas bien, ça fait peut-etre pas le même taff (https://github.com/AllInOneYT/react-native-thermal-printer/blob/main/android/src/main/java/com/reactnativethermalprinter/ThermalPrinterModule.java#L101). Bon après ca supporte pas android 12+, mais je voulais directement migrer tout ça dans la lib source. Après l'android manifest est bloquant dans ce cas indeed.

Je me suis dis qu'une méthode exposant "juste" les permissions nécessaires semble pas mal plutôt que les écrire dans le README. Qu'en penses-tu ?

Après avoir écris tout ça, il semble que la meilleure solution reste en fait de virer les permissions checks dans la lib RN dont je parle et mettre un message comme ici dans le README pour préciser qu'il les faut dans la manifest et voilà.

DantSu commented 1 year ago

Le français c'est tellement plus clair 😁

Alors, le probleme avec Android 13 est dans BluetoothConnection voir : https://github.com/DantSu/ESCPOS-ThermalPrinter-Android/issues/396#issuecomment-1530906048

Pour les développeurs, je pensais que l'application de test (Si tu fais un Git clone du repo) permettait à tous de voir comment les faire en fonction du besoin. En plus de ça il y a une rubrique complète dans la doc sur l'implementation des permissions.

Tu n'as peut être pas tord pour une méthode Printer.requestedPermissionsForBluetooth() mais bon par rapport à la doc je n'ai pas trop vu l'interêt. Pour le bluetooth il faut simplement faire ça :

if (ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH}, MainActivity.PERMISSION_BLUETOOTH);
} else if (ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_ADMIN) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_ADMIN}, MainActivity.PERMISSION_BLUETOOTH_ADMIN);
} else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_CONNECT) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_CONNECT}, MainActivity.PERMISSION_BLUETOOTH_CONNECT);
} else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_SCAN) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_SCAN}, MainActivity.PERMISSION_BLUETOOTH_SCAN);
} else {
    // Your code HERE
}
DantSu commented 1 year ago

En ReactNative il y a un acces à l'activité, car c'est elle qui gère ton code React. C'est plus simple à mettre en place. En librairie native Java, pour laisser libre champs sur l'utilisation de la librairie, c'est plus complexe.

KannarFr commented 1 year ago

Le français c'est tellement plus clair grin

Alors, le probleme avec Android 13 est dans BluetoothConnection voir : #396 (comment)

Pour les développeurs, je pensais que l'application de test (Si tu fais un Git clone du repo) permettait à tous de voir comment les faire en fonction du besoin. En plus de ça il y a une rubrique complète dans la doc sur l'implementation des permissions.

Tu n'as peut être pas tord pour une méthode Printer.requestedPermissionsForBluetooth() mais bon par rapport à la doc je n'ai pas trop vu l'interêt. Pour le bluetooth il faut simplement faire ça :

if (ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH}, MainActivity.PERMISSION_BLUETOOTH);
} else if (ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_ADMIN) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_ADMIN}, MainActivity.PERMISSION_BLUETOOTH_ADMIN);
} else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_CONNECT) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_CONNECT}, MainActivity.PERMISSION_BLUETOOTH_CONNECT);
} else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_SCAN) != PackageManager.PERMISSION_GRANTED) {
    ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_SCAN}, MainActivity.PERMISSION_BLUETOOTH_SCAN);
} else {
    // Your code HERE
}

Erf, non sur la lib RN j'ai ajouté ça, et ca ne fonctionne pas l'activity est stuck sur le premier if car sur Android 12 Manifest.permission.Bluetooth n'existe pas. Il y a sûrement une couche de check à faire en plus. Pour ça le requestedPermissionsForBluetooth me semblait pas mal. Sur cette PR https://github.com/AllInOneYT/react-native-thermal-printer/pull/52, avec un mobile Android 12 seul le dernier fixup fonctionne.

En ReactNative il y a un acces à l'activité, car c'est elle qui gère ton code React. C'est plus simple à mettre en place. En librairie native Java, pour laisser libre champs sur l'utilisation de la librairie, c'est plus complexe.

Ack.

DantSu commented 1 year ago

WoW chelou, j'ai un téléphone Android 13 et avant Android 12 et je n'ai absolument aucun soucis. C'est hyper étrange. tu es sur un compile SDK combien ? 33 ? pour être sur essaye avec compileSdk 32 et 33, pour voir si il y a une diff.

KannarFr commented 1 year ago

33, je viens de tester en 32 pareil. C'est vraiment le Manifest.permission.Bluetooth n'existant pas qui fait peter le checkSelfPermission.

DantSu commented 1 year ago

ok donc ça, ça devrait marcher :

        if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH) != PackageManager.PERMISSION_GRANTED) {
            ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH}, MainActivity.PERMISSION_BLUETOOTH);
        } else if (ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_ADMIN) != PackageManager.PERMISSION_GRANTED) {
            ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_ADMIN}, MainActivity.PERMISSION_BLUETOOTH_ADMIN);
        } else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_CONNECT) != PackageManager.PERMISSION_GRANTED) {
            ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_CONNECT}, MainActivity.PERMISSION_BLUETOOTH_CONNECT);
        } else if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S && ContextCompat.checkSelfPermission(this, Manifest.permission.BLUETOOTH_SCAN) != PackageManager.PERMISSION_GRANTED) {
            ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.BLUETOOTH_SCAN}, MainActivity.PERMISSION_BLUETOOTH_SCAN);
        } else {
            // CODE HERE
        }

C'est hyper étrange les différences d'interprétation de code entre deux téléphones Android avec la meme version de l'OS. C'est quand meme fou que deux téléphones sur Android 12 ou 13 ne gèrent pas les permissions de la même manière !

DantSu commented 1 year ago

J'ai mis à jour la lib v3.3.0. ça corrige certains problèmes de Bluetooth. J'ai aussi mis à jour la doc avec android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.S && pour les permissions bluetooth !

KannarFr commented 1 year ago

Arf j'ai oublié de préciser mais c'est pareil pour BLUETOOTH_ADMIN. Faut aussi add la condition de version sdk.

DantSu commented 1 year ago

bon ben ça tombe bien, j'ai eu des problèmes avec Jitpack, j'ai mis à jour la doc et l'application exemple !

KannarFr commented 1 year ago

Top, il semble que tu n'ai pas fait les releases, y a "que" la 3.3.0 dans ton repo d'artefacts. Me trompe-je?

KannarFr commented 1 year ago

Ah mais c'est ça jitpack. Ok, bin.. can't wait then :p.

DantSu commented 1 year ago

Oui Jitpack c'est pour utiliser la librairie en l'ajoutant dans les dépendances du fichier gradle