Bonjour @vedrine,
Ayant récemment acquis un roomba, je me permet de te soumettre un pr pour plugin-kroomba afin de corriger quelques points:
voici les changement que j’ai fait:
cleaning du code js et php « desktop » + update mise en page pour correspondre à la v4
optimisation:
du cron
création des commandes uniquement lors de l’insert et pas à chaque save (gain de temps lors du save)
cleaning du cmd execute
fix:
vérification des dépendances
la commande rafraichir doit avoir « refresh » comme logicaId pour être correctement affichée sur les widget par défaut
il y a encore un soucis avec l’affichage du refresh sur le widget v3 que je vais fixer plus tard: la commande est toujours affichée même si elle est cochée comme non visible
Je regarde le code que vous avez modifié et je me pose une question:
Quand est executé postInsert ? Pour postSave je vois bien quand.
Dans la classe kroombaCmd, fonction execute, il manque des break à chaque case.
Sans break quand c’est case start, il va exécuter pause resume stop et dock
Correct pour les break!
J’ai été trop vite; je vais corriger le pr; merci pour le review
Le postinsert est appelé après la création de l’équipement.
Par principe, aucun besoin de « créer » les commandes à chaque sauvegarde.
Cela prend un temps vraiment perceptible, Il y a vraiment une différence de réactivité de l’interface au moment du save de l’équipement je trouve.
Bonjour @Mips,
Merci pour le PR. C’est vrai que beaucoup du code de ce plugin est déjà ancien et date même d’avant que je reprenne le plugin (créé par Kavod) et il a bien besoin d’un bon nettoyage. Je suis donc OK pour presque tout le contenu du PR.
Mais je suis très réticent à déplacer la création des commandes dans le postInsert car je vois plusieurs avantages à le faire dans le postSave en particulier car si on met certaines instructions en dehors du bloc if (!is_object(… on est sûr de rétablir les changements « malvenus » faits par un utilisateur qui gêneraient le fonctionnement du plugin.
Le temps pris est négligeable car le postSave est rarement exécuté.
Donc je souhaite que çà reste comme çà dans ce plugin comme dans les autres que je maintiens.
J’ajoute qu’une chose que j’ai commencé à faire en local mais pas envoyé sur Github c’est de supprimer le toHtml et le widget dédié pour passer sur 3 widgets propre au plugin pour les commandes info status, binfull et battery gérés par la fonction templateWidget en V4 et 3 widgets dédiés en V3 car je trouve que les widgets d’eqLogic ça limite trop les possibilités de customisation par les utilisateurs.
On peut toujours faire un bouton « recréer les commandes » au cas où, sinon.
Et concrètement, aucun changement « impactant » ne peut être fait par un utilisateur car l’interface d’édition ne le permet pas (excepté le remove de la commande): impossible de changer le LogicalId, le type ou le subtype.
Le temps pris c’est vraiment un avis d’utilisateur: on sent chaque fois 1s de freeze ou plus quand il y a beaucoup de commandes sur les plugins faisant cela et ca me frustre à chaque clic car ca ne répond pas immédiatement
mais c’est toi qui voit.
ca me fait penser que j’ai oublié de corriger les generic type assignés par défaut qui ne sont pas utile: il faudrait mettre MODE_SET_STATE sur les actions (puisque c’est MODE_STATE sur l’état)
et il faut aussi mettre cette commande état en value sur les commandes actions pour que cela fonctionne.
Avec ça il y aura moyen de piloter les actions et avoir le retour d’état dans l’app mobile (jeedom).
Je peux faire un autre PR pour cela avec le fix du refresh sur le widget v3 si tu veux.
edit sur ton edit :
oui, le tohtml ca limite beaucoup, je trouve aussi; parfois il n’y a pas le choix de faire autrement.
Sur ce plugin ce n’est peut-être pas nécessaire de faire quoi que ce soit mais de tout laisser en standard vu qu’en gros il y a un état et 4, 5 actions…
Libre à chacun de faire ce qu’il veut ensuite.
Et bon, moi je suis encore en v3 en prod donc j’aimerais autant que le plugin fonctionne (le widget je m’en fou un peu, je vais pas dans mon dashboard pour vérifier l’état du robot ) mais par contre ce n’est peut-être plus la peine d’investir du temps de dev pour refaire un widget propre pour la v3: ca serait moi je le supprimerais complétement;
autre option, vu qu’il est là, le laisser comme il est et dans quelques mois on n’en parle plus car la v3 sera finie…
@Mips Ce n’était pas un review complet. Je suis tombé dessus par hasard.
Comme @vedrine je préfère garder la création des cmds dans postSave. Cela permet de les compléter, corriger après création de l’équipement lors d’une future mise à jour du plugin.
@vedrine Dans mon BoschIndego, le répertoire docs n’est pas dans le plugin. Cela permet d’alléger chaque plugin d’environ 4 à 5 Mo et 160 fichiers.
Les docs restent sur Github.
Il y a la function de l’update du plug-in pour cela. Payer un update dB de chaque commande a chaque save c’est chère alors qu’il y a des vrais mécanisme pour gérer cela.
Mais qu’importe, il n’y a pas de débat, chacun fait ce qu’il veut.
@vedrine veux tu que je change le pr en remplacement par un postsave ?
Du coup, logiquement il faudrait supprimer la possibilité de supprimer une commande : pour l’instant dans la stable l’utilisateur peut supprimer une commande mais à chaque save elle est recréé.
C’est aussi le problème de la création des commandes au postsave.
Bonjour @Mips si tu peux refaire un PR avec les commandes dans le postSave et tout le reste pareil c’est super sinon si tu n’as pas le temps je peux partir de ton premier PR et faire le changement.
Un grand merci pour ton aide sur les generic types car je ne voyais pas comment faire. Ce serait un plus de pouvoir commander le robot. Là aussi si tu peux faire un PR c’est top, sinon je peux le faire maintenant que tu m’as donné les valeurs à mettre (et aussi de ne pas oublier le setValue, j’aurais bien été capable de ne pas y penser)
Merci @Mips j’ai mergé tous tes changements à la fois en béta et en stable et c’est disponible sur le market.
Je n’ai pas encore essayé avec l’app mobile ou avec Homebridge mais je le ferai.
De rien.
Je n’ai pas eu le temps de régler les problèmes d’affichage sur le widget v3. C’était plus a faire que je ne pensais.
Ça sera pour une prochaine
: