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

add 0134-WebUI-Add-ShowInternalSysvars patch #1400

Conversation

jp112sdl
Copy link
Contributor

@jp112sdl jp112sdl commented Sep 6, 2021

Description

  • show/hide internal system variables in the system variables list
  • (un)set the Internal()-attribute to system variables within the webui

Bildschirmfoto 2021-09-06 um 09 10 51

Related Issue

#1321

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Alternate Designs

Possible Drawbacks

Verification Process

Release Notes

Contributing checklist

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING and LICENSE document.
  • I fully agree to distribute my changes under Apache 2.0 license.

@jens-maus
Copy link
Owner

Wow, das sieht ja schon recht gut aus! Danke @jp112sdl schon einmal. Nur auf eine andere Hintergrundfarbe sollten wir uns hier einigen, da "rot" sicherlich zu sehr mit "Achtung" oder "Problem" verknüpft ist :) Haben wir nicht an anderen Stellen bereits eine Hintergrundfarbe im Einsatz die dafür angemessen ist und folglich dann auch recyclt werden kann?

@jens-maus jens-maus added 💡 enhancement-ideas New feature or change request 🏷️ WebUI This refs the WebUI component labels Sep 6, 2021
@jens-maus jens-maus added this to the next release milestone Sep 6, 2021
@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Nur auf eine andere Hintergrundfarbe sollten wir uns hier einigen

Ja, das rote ist auch nicht meine erste Wahl... aber so richtig weiß ich auch nicht,welche Farbe man da nehmen sollte.
Ob man sie überhaupt anders färbt? Bei Programmen haben die "systeminternen" keine andere Farbe.
Oder man macht es auch über den Kontrast, wobei es wahrscheinlich auch nicht so gut ist, "interne SV" so aussehen zu lassen wie "inaktive Programme"

Hmm...

@jens-maus
Copy link
Owner

Das mit der Farbgebung schau ich mir gerne nochmal an sobald ich eine Testversion hier fertig habe. Was aber ggf. gleich auch anderen auffallen wird ist, das die systeminternen Variablen in der Auflistung ja immer hinten landen, d.h. es wäre die Frage ob wir das als Feature definieren oder ob wir die einfach wie bei den Programmen doch versuchen alphabetisch einzusortieren?

Und eine weitere Sache wäre noch ob wird dann vielleicht auch gleich die andere Systemvariablenauflistung (die man erhält wenn man eine Systemvariable in einem Programm auswählen will) gleich auch noch die andere Hintergrundfarbe verpassen wollen oder nicht? Und dort ist die Auflistung übrigens jetzt schon alphabetisch inkl. der "internen" Variablen. Vielleicht könnte man sich das davon abschauen?

Bildschirmfoto 2021-09-06 um 09 41 39

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Was aber ggf. gleich auch anderen auffallen wird ist, das die systeminternen Variablen in der Auflistung ja immer hinten landen, d.h. es wäre die Frage ob wir das als Feature definieren oder ob wir die einfach wie bei den Programmen doch versuchen alphabetisch einzusortieren?

Bei den Programmen ist es bei mir auch so, dass die systeminternen unten angehängt werden.
Selbst wenn ich noch mal auf den Tabellenkopf "Name" klicke (was bei den SV jedoch geht).
Wie man das gerade ziehen kann (also auch für Programme), weiß ich leider nicht.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Und dort ist die Auflistung übrigens jetzt schon alphabetisch inkl. der "internen" Variablen. Vielleicht könnte man sich das davon abschauen?

Komisch auch, dass die internen SV im Programm als Condition-Auswahl angeboten werden, als Destination-Auswahl dann wiederum nicht.

Das ist wohl der wilden Verwendung von
EnumIDs(), EnumUsedIDs(), EnumEnabledIDs() und EnumEnabledInternalIDs()
geschuldet.

Nun könnte man das gesamte Konzept zu überdenken.
Man holt sich grundsätzlich mit EnumIDs() erstmal alles in ein lokales Array und prüft dann nur innerhalb der Webseite, was wann und wie angezeigt werden soll.

Aber die Baustelle mach ich nicht auf ;-)

@jens-maus
Copy link
Owner

Das ist wohl der wilden Verwendung von
EnumIDs(), EnumUsedIDs(), EnumEnabledIDs() und EnumEnabledInternalIDs()
geschuldet.

Nun könnte man das gesamte Konzept zu überdenken.
Man holt sich grundsätzlich mit EnumIDs() erstmal alles in ein lokales Array und prüft dann nur innerhalb der Webseite, was wann und wie angezeigt werden soll.

Genau so ist es. Bzw. das liegt an dem erwähnten Unterschied von IseIdArray und normalen ObjectArrays in ReGaHss. Beide haben prinzipiell ein EnumIDs(), aber leider verhalten sich beide anders. Das EnumIDs() der normalen ObjectArrays geben alle IDs zurück die in dem jeweiligen Array drinstecken (und so gehört sich das normal), das EnumIDs der IseIdArray gibt aber leider nicht alle zurück und lässt eben die Internal weg. Deshalb ja der Umweg über nochmaligen auslesen via ?EnumEnabledInternalIDs()und dann hinzufügen zu den IDs die man vorher mit EnumIDs() erhalten hat. Das ist also leider eine Inkonsistenz in ReGaHss im umgang mit verschiedensten Arrayarten. Kurzum, des muss bei Condition wie auch bei Destination natürlich alle (auch die internen) auswählbar sein, sonst ergibt das kein Sinn IMHO.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Kurzum, des muss bei Condition wie auch bei Destination natürlich alle (auch die internen) auswählbar sein, sonst ergibt das kein Sinn IMHO.

Aus irgendeinem Grund hat sich eQ-3 dafür entschieden, den SysVarChooser bei der Condition mit iShowAll = 1 anzuzeigen und
bei der Destination mit iShowAll = 0. /www/rega/pages/tabs/admin/views/programs.htm#L681-L706

https://github.com/eq-3/occu/blob/master/WebUI/www/rega/pages/msg/sysVarsSelection.htm#L13-L30

Um Änderungen so klein wie möglich zu halten, bräuchte es meiner Meinung nach in Zeile 697 nur eine 1 statt der 0

@jens-maus
Copy link
Owner

Kurzum, des muss bei Condition wie auch bei Destination natürlich alle (auch die internen) auswählbar sein, sonst ergibt das kein Sinn IMHO.

Aus irgendeinem Grund hat sich eQ-3 dafür entschieden, den SysVarChooser bei der Condition mit iShowAll = 1 anzuzeigen und
bei der Destination mit iShowAll = 0.
/www/rega/pages/tabs/admin/views/programs.htm#L681-L706

Nun, da kann ich mir nur vorstellen, das sie verhindern wollten/wollen, das der Nutzer so Sachen bastelt wie die Anzahl Alarmmeldungen / Systemvariablen zu modifizieren die dann irgendetwas durcheinander bringen könnten. Die Frage wäre also in der Tat noch einmal zu überlegen. Gibt es ein sinnvollen use-case das ein Nutzer eine als "intern" deklarierte Systemvariable via WebUI Programm ändern lassen will?

@jens-maus
Copy link
Owner

So, hab den Patch von dir mal ein wenig zwecks style angepasst. Nun sieht es so aus:

Bildschirmfoto 2021-09-06 um 12 16 27

Was meinst du? Sollte nun etwas kompakter sein und das "LightGrayBkg" vielleicht nicht gar so invasiv für das darstellen der internen systemvariablen. Meinungen?

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Was meinst du?

Sieht gut aus! Der Abstand zwischen den 3 Rows war wirklich recht groß, das sieht so schon angenehmer aus.
Auch das mit der Farbe... letztendlich sieht man ja auch am Haken, ob die SV (nicht) systemintern ist.

Die Frage wäre also in der Tat noch einmal zu überlegen. Gibt es ein sinnvollen use-case das ein Nutzer eine als "intern" deklarierte Systemvariable via WebUI Programm ändern lassen will?

Ich würd es wohl auch dabei belassen. Als Condition ja, als Destination nein.

Da fällt mir noch ein: Es ist jetzt möglich, die internen SVen "Alarmmeldungen" und "Servicemeldungen" kaputt zu machen, wenn man z.B. den Typ auf "Logikwert" ändert.
Und da man in Einstellungen->Systemvariablen auch den Wert setzen kann, wäre eine Manipulation dessen auch von hier aus möglich.

Nicht gut glaub ich... Die beiden essentiellen SVen müssen noch irgendwie "nicht editierbar" gemacht werden, oder?

@jens-maus
Copy link
Owner

Da fällt mir noch ein: Es ist jetzt möglich, die internen SVen "Alarmmeldungen" und "Servicemeldungen" kaputt zu machen, wenn man z.B. den Typ auf "Logikwert" ändert.
Und da man in Einstellungen->Systemvariablen auch den Wert setzen kann, wäre eine Manipulation dessen auch von hier aus möglich.

Nicht gut glaub ich... Die beiden essentiellen SVen müssen noch irgendwie "nicht editierbar" gemacht werden, oder?

Da gibt es ja noch die .Unerasable() Methode die wird ja jetzt schon zum deaktivieren des Löschen knopfes verwendet. Die könnte man abprüfen und dann entsprechend elemente abschalten, usw.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

und dann entsprechend elemente abschalten

Dann bräuchte man ja eigentlich nur den "Bearbeiten" Button analog zum "Löschen" auch noch sperren

@jens-maus
Copy link
Owner

und dann entsprechend elemente abschalten

Dann bräuchte man ja eigentlich nur den "Bearbeiten" Button analog zum "Löschen" auch noch sperren

Was ist dann aber mit der Systemvariable "Anwesenheit"? Die ist ja auch Unerasable, aber lässt sich eben bearbeiten, schon immer wohl.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Stimmt. Anwesenheit ist aber ((!Internal) && (Unerasable))
Dann sollte der "Bearbeiten" Button nur deaktiviert sein, wenn ((Internal) && (Unerasable))

@jens-maus
Copy link
Owner

Dann könnte man ja immernoch einfach die Alarmmeldungen+Servicemeldungen Variablen von Intern->Normal umswitchen und dann kann man auch die bearbeiten.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Richtig, aber die Wahrscheinlichkeit, dass das "versehentlich" passiert, ist um eine Hürde gestiegen

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

Oder ganz auf die Attribute verzichten und prüfen, ob die ID 40 oder 41 ist

@jens-maus
Copy link
Owner

Oder ganz auf die Attribute verzichten und prüfen, ob die ID 40 oder 41 ist

Naja, feste IDs sind IMHO nie eine gute Idee. Dann lieber ein Kompromiss dazwischen.

@jp112sdl
Copy link
Contributor Author

jp112sdl commented Sep 6, 2021

feste IDs sind IMHO nie eine gute Idee

Ja schon, aber sie werden ja vom System mithilfe der hm_autoconf auch immer fix so angelegt.

Bin ich jetzt aufm Holzweg...?
Sind die beiden 40 und 41 gar nicht intern?
sv.Internal(0); in https://github.com/eq-3/occu/blob/master/WebUI/bin/hm_autoconf#L218-L228

@jens-maus
Copy link
Owner

feste IDs sind IMHO nie eine gute Idee

Ja schon, aber sie werden ja vom System mithilfe der hm_autoconf auch immer fix so angelegt.

Ich sagte doch, das ist nie eine gute Idee ;) Und das eQ3 die initial mit 40/41 anlegen lassen hat bzw. die so in ReGa fest eingebongt sind ist auch IMHO keine gute Sache.

Bin ich jetzt aufm Holzweg...?
Sind die beiden 40 und 41 gar nicht intern?
sv.Internal(0); in https://github.com/eq-3/occu/blob/master/WebUI/bin/hm_autoconf#L218-L228

Tja, vmtl. schiebt die ReGa die intern immer auf internal bei einem neustart oder so. müsste man mal testen.

@jens-maus
Copy link
Owner

Ok, mit der aktuellsten Änderung check ich jetzt einfach nach ID_GW_SYSSERVICE und ID_GW_SYSALARM um zu schauen ob das die beiden systemvariablen sind statt hier 40/41 einfach stur zu nehmen. Denke so sollte es nun passen.

@jens-maus jens-maus merged commit ce1a6eb into jens-maus:master Sep 6, 2021
@mbhomie007
Copy link

mbhomie007 commented Sep 9, 2021

@jens-maus @jp112sdl

Vielen Dank für den Patch! 👍
Kurze Rückmeldung:
Kleinigkeit:

  1. Die Buttons "Löschen" und "Bearbeiten" sind nicht gleich groß gegenüber oben.
  2. Rechts ist sehr viel Platz, könnte man lieber links nutzen. Etwas verrutscht.

Sieht etwas anders aus als bei eurem Entwurf.

Chrome Version 93.0.4577.63

image

@jens-maus
Copy link
Owner

@mbhomie007 das sind IMHO vernachlässigbare Kleinigkeiten würde ich sagen. Die abgeschalteten Buttons sind deshalb kleiner weil der font drin eben nicht bold ist wie oben. Wie gesagt, Kleinigkeit und IMHO überhaupt nicht relevant und da die WebUI ohnehin antiquiert ist, würde ich für die Kleinigkeit jetzt dafür nicht nochmal hand anlegen wollen.

@mbhomie007
Copy link

Vielen Dank für die schnelle Rückmeldung.
Alles gut, und funktioniert! 😎

@jp112sdl jp112sdl deleted the improvement/0134-WebUI-Add-ShowInternalSysvars branch November 20, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 enhancement-ideas New feature or change request 🏷️ WebUI This refs the WebUI component
Projects
Development

Successfully merging this pull request may close these issues.

3 participants