search

homematic-community / hm-print

A fork of the HomeMatic CCU Addon "ProgrammeDrucken" from http://homematic-forum.de/forum/viewtopic.php?f=41&t=19296
12 stars 5 forks source link

Restore Konzept für programs.htm #13

Open TomMajor opened 3 years ago

TomMajor commented 3 years ago

In Kombination mit anderen AddOn ist das Restore Konzept ungünstig, bei uninstall die alten Backup-Copies einfach wieder zu restoren ohne Rücksicht auf Änderungen die andere AddOn in der Zwischenzeit gemacht haben. Da bin ich neulich wieder drauf reingefallen (kein Drucken Button).

Das betraf vor allem die Datei /www/rega/pages/tabs/admin/views/programs.htm, welche auch von Jeromes JP-HB-Devices-addon gepatched wird (und auch mein abgeleitetetes -Reduced AddOn)

Mein Vorschlag wäre, für programs.htm den uninstall genau so mit sed wie den install vorzunehmen, nur reverse. Meine Tests dazu waren ok, konnte aber nur auf RM testen:

patch

#!/bin/sh

# String to include the functions.js for Programm/s
SSTRINGZWO="setFooter(s);"
RSTRINGZWO="setFooter(s); var scriptpp = document.createElement(\"script\"); scriptpp.type = \"text/javascript\"; scriptpp.src = \"/addons/print/functions.js\"; \$(\"body\").appendChild(scriptpp);"

# String to include the print button in single "Programm"
SSTRING="<td style='text-align:center; vertical-align:middle;'><div class='FooterButton CLASS04801' onclick='new HMScriptExecutor();'>\${footerBtnTestScript}</div></td>"
RSTRING="<td style='text-align:center; vertical-align:middle;'><div class='FooterButton CLASS04801' onclick='new HMScriptExecutor();'>\${footerBtnTestScript}</div></td><td style='text-align:center; vertical-align:middle;'><div class='FooterButton' onclick='PrintPage();'>Drucken</div></td>"

FNAME=/tmp/test/programs.htm
FNAMESAVE=${FNAME%.htm}.save

cp $FNAME $FNAMESAVE
sed -i "s|$SSTRING|$RSTRING|g" $FNAME
sed -i "s|$SSTRINGZWO|$RSTRINGZWO|g" $FNAME

restore

#!/bin/sh

# String to include the functions.js for Programm/s
SSTRINGZWO="setFooter(s);"
RSTRINGZWO="setFooter(s); var scriptpp = document.createElement(\"script\"); scriptpp.type = \"text/javascript\"; scriptpp.src = \"/addons/print/functions.js\"; \$(\"body\").appendChild(scriptpp);"

# String to include the print button in single "Programm"
SSTRING="<td style='text-align:center; vertical-align:middle;'><div class='FooterButton CLASS04801' onclick='new HMScriptExecutor();'>\${footerBtnTestScript}</div></td>"
RSTRING="<td style='text-align:center; vertical-align:middle;'><div class='FooterButton CLASS04801' onclick='new HMScriptExecutor();'>\${footerBtnTestScript}</div></td><td style='text-align:center; vertical-align:middle;'><div class='FooterButton' onclick='PrintPage();'>Drucken</div></td>"

FNAME=/tmp/test/programs.htm
FNAMESAVE=${FNAME%.htm}.save

sed -i "s|$RSTRING|$SSTRING|g" $FNAME
sed -i "s|$RSTRINGZWO|$SSTRINGZWO|g" $FNAME

ich mache erst mal bewusst keinen PR sondern eine issue da noch ein PR offen ist und ich nicht weiß wie der Status der Weiterentwicklung von hm-print ist.

Weitere Beobachtungen:

  1. Alle if Abfragen werden bei jedem Start des Systems ausgeführt, ein besserer Stil imho wäre ein Flag zu haben ob das AddOn installiert ist oder nicht a la https://github.com/jp112sdl/JP-HB-Devices-addon/blob/master/src/rc.d/jp-hb-devices-addon#L69 und dann nur einmalig ausführen

  2. Jede if Bedingung hat innerhalb ein extra mount RW/RO. Effizienter wäre ein einziges RW/RO "außen" beim install. Hängt natürlich auch mit 1. zusammen.

  3. programs.org sieht wie ein ungenutzter Rest von früher aus da sonst immer nur mit programs.org1 gearbeitet wird: https://github.com/homematic-community/hm-print/blob/master/rc.d/programmedrucken#L162

  4. Nach meinen Beobachtungen braucht hm-print ein restart auf RM wenn es nicht installiert war, das belegen auch Beiträge von usern im HM Forum thread zu diesem AddOn. Insofern sollte man imho zumindest für RM den Neustart erwzingen.

ptweety commented 3 years ago

Hallo @TomMajor, prinzipiell ist m.E. das ganze Addon in seiner Struktur etwas outdated. Am liebsten würde ich hergehen und ein neues Addon ohne direkte Veränderung der originalen Dateien der CCU erstellen. Dazu braucht es aber einen größeren Umbau.

Zumindest deine 2. Beobachtung ist aber in meinem PR #9 bereits enthalten.

PS: ich weiß nicht, wer hier überhaupt Rechte für den Merge hat, daher liegt mein PR auch schon eine Weile rum ...

jp112sdl commented 3 years ago

Ich hatte in den letzten Tagen mit Tom schon per Mail ein wenig "gebrainstormt".

Insofern stimme ich - mit Ausnahme von Punkt 1 - grundsätzlich zu. Ein globales "installed"-Flag ist m.Mn. nach nicht wichtig. Man könnte schon jede Datei einzeln auf Vorhandensein der Modifikation prüfen und dann auch bei Bedarf patchen.

Am liebsten würde ich hergehen und ein neues Addon ohne direkte Veränderung der originalen Dateien der CCU erstellen.

Klingt interessant - wie kann sowas funktionieren?

Mir schwebte mal sowas vor, dass man beim Booten das komplette /www in den RAM lädt und dort dann halt bei jedem Start die Addon-Patche anwendet. Das würde mir die Arbeit mega erleichtern, weil ich dann davon ausgehen kann, dass die Quelldateien garantiert immer unverändert sind.

Aber: Nachteilig bei CCU2 und CCU3 ist, dass die Addon-Installation zu einem späten Zeitpunkt stattfindet, wo ReGaHss und RFD (die für unser Addon relevanten Dienste) laufen. Das würde dann bei jedem Reboot zur Folge haben, dass nach der Addon-Installation die Dienste noch mal abgewürgt und neu gestartet werden.

Ein /www aus dem RAM servieren würde jedoch nur ab CCU3 gehen und dann müsste halt noch mehr drumherum umgebaut werden. Ich denke nicht, dass original-FW-Nutzer sowas wollen.

PS: ich weiß nicht, wer hier überhaupt Rechte für den Merge hat, daher liegt mein PR auch schon eine Weile rum ...

Ich glaube nur @jens-maus.

Mein Vorschlag an Tom war auch, erst mit der Arbeit zu diesem Issue hier zu beginnen, wenn dein PR (@ptweety) gemerged ist. Sonst fängt man wieder an zu basteln.

jens-maus commented 3 years ago

Also ich kann den PR von @ptweety gerne mergen, wegen des großen Umfanges der Änderungen ist ein review aber recht schwierig. Hier wäre es besser gewesen das ganze kleinteiliger umzusetzen.

Auch möchte ich noch einmal in Erinnerung rufen, das nein eigentlicher Plan für dieses Addon ist, dieses direkt als webui patch in RaspberryMatic zu integrieren, denn es handelt sich bei diesem eigentlich un nichts anderes als ein großer webui und da ist es ein leichtes das umzusetzen. Und eigentlich bereitet mir das addon mit seinen massiven Änderungen an der webui via remount ziemliche Bauchschmerzen.

Das würde dann das addon auf die Nutzung mit einer CCu3 oder CCU2 Firmware in zukunft beschränken.

jp112sdl commented 3 years ago

Um hier in irgendeiner Weise weiter voranzukommen: Wenn die Änderungen von @ptweety bei ihm funktionieren, warum dann kein "Beta"-Release veröffentlichen? Tester gibt es doch einige (so zumindest bei RaspberryMatic Snapshots zu sehen)

So, wie es jetzt ist, ist es 💩 , wenn jemand das hm-print-Addon deinstalliert und parallel Toms oder mein Addon eingesetzt wird.

Und eigentlich bereitet mir das addon mit seinen massiven Änderungen an der webui via remount ziemliche Bauchschmerzen.

WebUI-technisch wird doch nur ein Footer-Button implementiert? Ja und das Remount erfolgt nach dem PR von @ptweety auch nur noch einmalig...

das nein eigentlicher Plan für dieses Addon ist, dieses direkt als webui patch in RaspberryMatic zu integrieren,

Das kannst du ja gern machen, aber RM ist halt nur ein Stück der großen CCU-Derivate-Torte.

TomMajor commented 3 years ago

Zumindest deine 2. Beobachtung ist aber in meinem PR #9 bereits enthalten.

@ptweety ja stimmt, hatte ich mir nicht alles anschauen können, der PR ist zu groß, da muss ich Jens zustimmen. Besser fürs Review wären kleinere Schritte.

Wenn Jens das AddOn für RM integrieren würde wäre das super und IMHO ein Schritt nach vorn um die Konflikte mit anderen AddOn aufzulösen und natürlich die RM besser und kompfortabler zu machen.. Die anderen Derivate können ja auf dem aktuellen Stand verbleiben und verlieren erst mal nichts, nur ich finde es ungut Fortschritt für RM verhindern mit dem Argument die anderen Derivate haben nichts davon.

Kurzfristig schlage ich das reverse restore mit sed anstatt dem backup copy vor, wie oben geschrieben, das würde das Hauptproblem dieser issue adressieren.

jp112sdl commented 3 years ago

Besser fürs Review wären kleinere Schritte.

Nun, wenn man gemeinsam genutzte Funktionen aus 25 Dateien in eine einzelne Datei packt und dann darauf verweist, dann hat man in 25 Dateien Änderungen vorgenommen. Wie will man so etwas in kleinere Schritte packen? 🤔

@ptweety hatte seine Änderungen in einem Kommentar https://github.com/homematic-community/hm-print/pull/9#issuecomment-725000991 auch noch mal gut zusammengefasst.