Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Konzept für programs.htm #13

Open
TomMajor opened this issue Jan 16, 2021 · 6 comments
Open

Restore Konzept für programs.htm #13

TomMajor opened this issue Jan 16, 2021 · 6 comments

Comments

@TomMajor
Copy link
Member

TomMajor commented Jan 16, 2021

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
Copy link
Contributor

ptweety commented Jan 16, 2021

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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 #9 (comment) auch noch mal gut zusammengefasst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants