Adding Settings Menu #552
Adding Settings Menu #552
Conversation
… moved some methods
…d some documentation
|
Dieser PR ist endlich ready für's Reviewen! Bitte bedenkt, dass ich die Tests für die neuen Codezeilen noch schreiben werde, freue mich aber sehr über Feedback zur Implementierung vorab! (: |
sarahvgls
left a comment
sarahvgls
left a comment
There was a problem hiding this comment.
Sehr cool!! Vielen Dank für deine Arbeit, das ist sehr cool, dass wir das jetzt haben!!
Habe ein paar kleine Anmerkungen zum Code kommentiert. Ich würde außerdem andere default-Schriftgrößen einstellen, das könnte doch etwas verwirrend sein:
Eine Frage zu den Datenbanken in den Settings: Wollen wir nicht grundlegend die gleichen Einstellungen wie ursprünglich (aktuell auf dev-branch)? Vor allem, da es aktuell deutlich weniger der Funktionen hat oder? Mir fehlt zb die schon verfügbare Database, die ja mit dem run_protzilla skript runtergeladen wird
Der Code dafür müsste ja irgendwo noch stehen, den haben wir nicht gelöscht - ich kann aber gerade nicht einschätzen, wie aufwändig es ist, das hier anzuzeigen.
Wenn diese Seite erst mal als Platzhalter gedacht ist und wir uns dann wann anders uns tatsächlich darum kümmern, ist das okay - aber dann würde ich das irgendwo im Code oder sogar auf der angezeigten Seite deutlich dokumentieren, sonst geht das vllt verloren
|
|
||
| if isinstance(plot, bytes): # base64 encoded plots | ||
| if format_ in ["eps", "tiff"]: | ||
| # TO DO: Include scale_factor here |
There was a problem hiding this comment.
Ist das ToDo noch aktuell?
There was a problem hiding this comment.
Yes, ich habe aktuell keinen Überblick, wo wir diese andere Art von Plots verwenden und wäre stark dafür, sie einfach rauszunehmen und stattdessen einheitlich Plotly verwenden. Es existiert bereits das Issue "Use Plotly Template in all Sections #549", kann aber gerne noch einen Hinweis auf dieses To Do dort einfügen.
| """ | ||
| Converts all plots from this step to files according to the format and size in the Plotly template. | ||
| :param settings: Dict containing the plot settings. | ||
| :return: List of all exported plots. |
There was a problem hiding this comment.
Vielleicht noch hinzufügen was genau ein exportierter Plot ist, also ob es die raw files sind oder metadaten als Plot objekt oder so?
| :param settings: Dict containing the plot settings. | ||
| :return: List of all exported plots. | ||
| """ | ||
| from ui.settings.plot_template import get_scale_factor |
There was a problem hiding this comment.
Ist es absichtlich, dass das erst hier importiert wird?
There was a problem hiding this comment.
Ja, tatsächlich Absicht. Wenn ich das oben importiert habe, kam dieser Error (hier nur ein Ausschnitt):
File "/home/ronja/git/PROTzilla2/ui/runs/urls.py", line 3, in <module>
from . import views
File "/home/ronja/git/PROTzilla2/ui/runs/views.py", line 22, in <module>
from protzilla.run import Run, get_available_run_names
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ronja/git/PROTzilla2/ui/../protzilla/run.py", line 8, in <module>
from protzilla.steps import Messages, Output, Plots, Step
File "/home/ronja/git/PROTzilla2/ui/../protzilla/steps.py", line 17, in <module>
from ui.settings.plot_template import get_scale_factor
File "/home/ronja/git/PROTzilla2/ui/../ui/settings/plot_template.py", line 5, in <module>
from protzilla.disk_operator import YamlOperator
File "/home/ronja/git/PROTzilla2/ui/../protzilla/disk_operator.py", line 14, in <module>
from protzilla.steps import Messages, Output, Plots, Step, StepManager
ImportError: cannot import name 'Messages' from partially initialized module 'protzilla.steps' (most likely due to a circular import) (/home/ronja/git/PROTzilla2/ui/../protzilla/steps.py)
Also steps.py möchte Messages, Output, etc. und get_scale_factor aus dem plot_template.py importieren. Dort wird der YamlOperator aus disk_operator.py importiert, der wiederum möchte unter anderem Step aus steps.py importieren, wo dann wieder Messages, Output, etc. importiert werden soll und da fängt das ganze wieder von vorne an ... Also vermeidet das Importieren an dieser Stelle diesen circular import, weil es erst geladen wird, wenn es benötigt wird. :)
Ich fand diesen Workaround auch etwas befremdlich, habe es aber hier gefunden:
It is sometimes necessary to move imports to a function or class to avoid problems with circular imports. Gordon McMillan says:
Circular imports are fine where both modules use the “import ” form of import. They fail when the 2nd module wants to grab a name out of the first (“from module import name”) and the import is at the top level. That’s because names in the 1st are not yet available, because the first module is busy importing the 2nd.
In this case, if the second module is only used in one function, then the import can easily be moved into that function. By the time the import is called, the first module will have finished initializing, and the second module can do its import.
(Sorry für die lange Antwort, aber finde es irgendwie total nice, wie wir in unseren Teilaufgaben so random stuff lernen - und vielleicht ist das ja für noch wen interessant. ^^)
There was a problem hiding this comment.
Oki danke für die Erklärung!! Ja, das habe ich auch erst mit Protzilla gelernt, dass man das tatsächlich so macht :D
There was a problem hiding this comment.
Also ich fand die lange Antwort interessant :D
There was a problem hiding this comment.
Ach, du liest hier auch mit? 👋 Das freut mich zu hören haha (:
protzilla/utilities/utilities.py
Outdated
| return f"{base_name}.{file_extension}" | ||
|
|
||
|
|
||
| def parameters_from_post(post): |
There was a problem hiding this comment.
Würde mich hier über eine kleine Dokumentation und Return-value annotation freuen, sonst ist das ohne Zusammenhang nicht so gut zu verstehen glaube ich
There was a problem hiding this comment.
Die Methode habe ich tatsächlich einfach nur aus dem run-Kontext in utilities verschoben, damit ich sie "sauber" auch in settings nutzen kann. Aber sure, kann ich gerne noch ergänzen, da hast du Recht!
| return parameters | ||
|
|
||
|
|
||
| def convert_str_if_possible(s): |
There was a problem hiding this comment.
Hier gerne auch doc-string und return type :)
There was a problem hiding this comment.
Same as for parameters_from_post, aber ergänze ich gern! :)
There was a problem hiding this comment.
Danke, dass du es trotzdem ergänzt hast ^^
There was a problem hiding this comment.
Sehr schön geschriebener Code finde ich!!
| <div class="field-wrapper"> | ||
| <label for="id_selected_database">Selected Database:</label> | ||
| <select name="selected_database" class="form-select mb-2" id="id_selected_database"> | ||
| <option value="Database 1" {% if initials.selected_database == "Database 1" %}selected{% endif %}>Database 1</option> |
There was a problem hiding this comment.
Was sind Database 1,2 und 3 und werden sie irgendwo wiederverwendet?
There was a problem hiding this comment.
Nee, das sind tatsächlich nur Platzhalter, da ich die Database-Sachen in diesem PR noch nicht verschieben wollte. Ich dokumentiere das aber ordentlicher, danke für's Anmerken!
Ist eigentlich schon passiert, habe nur vergessen die Änderung noch zu stagen, ups!
Genau richtig, es war als Platzhalter gedacht, da das Verschieben der Database-Logik doch etwas mehr Arbeit erfordert und ich das lieber in einem anderen PR erledigen (lassen) würde. Es gibt auch schon Issues dazu, dass es manchmal nicht funktioniert, und uns sind ja auch schon weirde Dinge in den Tests aufgefallen. Würde es lieber als ein "Refactoring Databases" Arbeitspaket sehen. Aber danke für Deinen Hinweis, werde das im Code besser dokumentieren und gleich mal ein Issue dazu aufmachen, damit wir das auf dem Schirm haben. (: |
sarahvgls
left a comment
sarahvgls
left a comment
There was a problem hiding this comment.
Danke für die Änderungen, Antworten und die Erklärungen!! ^^
Hatte noch eine Idee, ob man vielleicht einen "reset to default" button hinzufügen möchte, sag aber gerne mal Bescheid, was du davon hältst :))
| file_format: png | ||
| font: Arial | ||
| heading_size: 22 | ||
| height: 40 |
There was a problem hiding this comment.
Bei mir sah es jetzt trotzdem wieder so aus wie im letzten Screenshot, kommt aber vermutlich daher, dass ich es das letzte mal doch gespeichert habe und dann die default werte ja "überschrieben" werden in plots.yaml. Man könnte überlegen, ob sich ein "reset to default" Button lohnt, der einfach wieder plots.yaml mit plots_default.yaml überschreibt für solche Fälle bzw. falls man mal falsch konfiguriert und sich nicht mehr erinnern kann was sinnvoll aussah? Wäre nur ein grober Vorschlag, weiß nicht, ob es unbedingt umgesetzt werden muss :)))
There was a problem hiding this comment.
Ein "Should have" war ja die Selection von mehreren gespeicherten Plotly-Templates, zum Beispiel je Publikation (mit verschiedenen Guidelines) an der man arbeitet. Und da fänd ich es super sinnvoll, wie du es sagst, eine default Option zum Auswählen zu haben! Würde ich dann nach unserer Migration aufnehmen wollen. :)
|
|
||
| <!-- PLEASE NOTE: This HTML is a placeholder. TO DO: Move "Manage databases" content to this page. --> | ||
| <p><b>Please note: This page is an placeholder for displaying the database management in the future.</b></p> | ||
|
|
There was a problem hiding this comment.
Ahh fair... Habe ich übersehen haha
There was a problem hiding this comment.
Alles fine, finde es so nochmal deutlich einleuchtender! (:
noLeonardo
left a comment
noLeonardo
left a comment
There was a problem hiding this comment.
Es ist echt ein tolles Feature, dass wir jetzt Plots an einer Stelle einheitlich formatieren können, und auch den Demo-Plot finde ich sehr toll :)
Bei mir bugt die UI noch ein ganz wenig rum, was vermutlich daran liegt, dass der Content etwas breiter als der Screen ist. Da wäre vielleicht noch eine overflow-x Option und etwas mehr Spacing zu den Linien passend. Aber mit React wird das ja bestimmt deutlich besser und einfacher.
Eine zukünftige Idee wäre vielleicht noch, den Demo-Plot in der echten mm-Größe zu rendern, damit ich direkt sehen kann, wie groß meine Plots exportiert werden.
Insgesamt toller und auch viel Code, der aber bei meinem Testen gut funktioniert hat 🚀
Ja, das hab ich aktuell mit den 600px in der Breite hardgecoded, weil das in React eh nochmal anders wird. ^
Das ist leider nicht so einfach, weil die "echte mm-Größe" von der Auflösung des anzeigenden Monitors oder auch von deinem Browser abhängt. Daher ist hier die Idee, den Plot im richtigen Verhältnis zwischen Länge und Breite anzuzeigen. |
Description
fixes #550
Changes
settingsas a new Django appviews.pyto display the setting forms and plot previewuser_data/settings/last_viewandrun_nameto most of the view functions to enable reloading the last view after saving or canceling the settingsparameter_from_postandconvert_str_if_possiblefrom ui/runs/views_helper.py to protzilla/utilities/utilities.pysteps.pyandruns/views.pyto include scaling the downloaded plots and for fixing thetiffandepsdownload option.(Please note, that the following things are not implemented yet due to the upcoming migration to React: Sticky positioning of footer, saving settings when switching to another section, moving "Manage Database" page to settings section.)
Testing
PR checklist
Development
Mergeability
blackCode review