Added menubar and buttons to the GUI #12
Conversation
julius-kress
changed the title
napytau/gui/checkbox_datapoint.py
Outdated
| :param coordinates: Tuple of the coordinates. | ||
| :param is_checked: Boolean value if the checkbox is checked. | ||
| """ |
There was a problem hiding this comment.
This seems redundant
There was a problem hiding this comment.
I rephrased the comment.
napytau/gui/checkbox_datapoint.py
Outdated
| @@ -0,0 +1,24 @@ | |||
| class CheckboxDataPoint: | |||
There was a problem hiding this comment.
I would suggest moving this class to a model directory, as it is purely a data representation, this helps cleanly segregate your code
napytau/gui/checkbox_datapoint.py
Outdated
| if not isinstance(coordinates, tuple) or len(coordinates) != 2: | ||
| raise ValueError("coordinates must be a tuple of two float values") | ||
| if not all(isinstance(coord, float) for coord in coordinates): | ||
| raise TypeError("all coordinates must be floats.") | ||
| if not isinstance(is_checked, bool): | ||
| raise TypeError("is_checked must be a bool.") |
There was a problem hiding this comment.
This class should only ever be constructed from internal data, therefore all of these should be caught by our static analysis.
We should probably discuss if we want to perform this kind of data validation outside of the modules performing IO
There was a problem hiding this comment.
You're right that validation does not have to happen here.
napytau/gui/checkbox_datapoint.py
Outdated
| """ | ||
| Toggles the boolean value | ||
| :return: None | ||
| """ |
There was a problem hiding this comment.
In general i would like to avoid trivial comments, due to our extensive type hints and naming conventions these should not be necessary
There was a problem hiding this comment.
I rephrased the comment.
napytau/gui/checkbox_datapoint.py
Outdated
| self.coordinates = coordinates | ||
| self.is_checked = is_checked | ||
|
|
||
| def toggle_checkbox(self) -> None: |
There was a problem hiding this comment.
This class is not dependent on being toggled by a checkbox, it only encapsulates a coordinate that can be toggled, please consider adjusting the naming within this class to remove the semantic dependency on a checkbox
There was a problem hiding this comment.
Sure, i renamed the method.
napytau/gui/gui_app.py
Outdated
| class GUIApp(customtkinter.CTk): | ||
| def __init__(self) -> None: | ||
| """ | ||
| Constructor for the GUIApp, initializes the GUI. |
There was a problem hiding this comment.
We might want to emphasize, that this is the logical entry point into the entire GUI
There was a problem hiding this comment.
Yes, added it.
napytau/gui/gui_app.py
Outdated
| self._init_graph() | ||
|
|
||
| # Initialize Checkboxes with datapoints | ||
| self._init_datapoint_checkboxes() |
There was a problem hiding this comment.
Your intuition to move these steps into a method is great, however this keeps them in the same file making it much harder to review and edit the code later (especially as we keep adding functionality).
Please move all _init_x methods into separate classes named x and the create and mount them here
There was a problem hiding this comment.
I did this on purpose because I didn't want to over-modularize the code.
If for example the menubar is a separate class, one could argue to also refactor the buttons within the menubar (like File, View, ...) into separate classes and also the options within these buttons.
Then you end up with a lot of classes which are all initialized and used once in the same spot.
But I also see your point, especially for the graph.
What is your opinion on splitting up the GUI only into the classes MenuBar, Graph, Informationfield and Checkboxfield (names more or less like that)?
I would refactor the menubar and checkboxes which this PR/Issue is about and leave the graph and info-field for a separate Issue/PR.
napytau/gui/gui_app.py
Outdated
| :param value: The value. | ||
| :param window: The window. | ||
| :return: The canvas. |
There was a problem hiding this comment.
As noted above this comment seems trivial
There was a problem hiding this comment.
This belongs to the graph mockup and shouldn't be part of this PR, as it is gonna be replaced anyways in on of the next issues.
napytau/gui/gui_app.py
Outdated
| # the figure that will contain the plot | ||
| fig = Figure(figsize=(3, 2), dpi=100, facecolor="white", edgecolor="black") | ||
|
|
||
| # list of squares | ||
| y = [(i - 50) ** value for i in range(101)] | ||
|
|
||
| # adding the subplot | ||
| plot1 = fig.add_subplot(111) | ||
|
|
||
| # plotting the graph | ||
| plot1.plot(y) | ||
|
|
||
| # creating the Tkinter canvas | ||
| # containing the Matplotlib figure | ||
| canvas = FigureCanvasTkAgg(fig, master=window) | ||
| canvas.draw() | ||
|
|
||
| return canvas.get_tk_widget() |
There was a problem hiding this comment.
I will leave these comments to you, they don't seem terribly trivial but we don't really need all of them, do as you see fit.
There was a problem hiding this comment.
Same as above, this is some leftover code from the graph mockup and will be replaced soon.
napytau/gui/gui_app.py
Outdated
| @@ -0,0 +1,478 @@ | |||
| from typing import List, Tuple | |||
There was a problem hiding this comment.
The review of this file wasn't 100% thorough, as there is some higher level work to do moving the Widgets into their own classes. After that is done i will review each one more extensively.
|
I forgot to write it in the final review comment, but the GUI looks really nice, good job! |
napytau/gui/gui_app.py
Outdated
| self._init_control_area() | ||
|
|
||
| # Initialize information area | ||
| self._init_information_area() |
There was a problem hiding this comment.
Area is a really general term, as is information. We might consider renaming this to control_panel and log_list.
There was a problem hiding this comment.
I like these suggestions and noted them. But I think this should be part of separate issues.
napytau/gui/gui_app.py
Outdated
| self._init_control_area() | ||
|
|
||
| # Initialize information area | ||
| self._init_information_area() |
There was a problem hiding this comment.
There is an open draft that i thought was for adding this, you might want to split this out into a separate PR and attach that to the other Issue.
There was a problem hiding this comment.
This PR only adds the buttons like described in the related issue. However I had to move/replace the previous mockup in some places.
But logically the graph, the control panel and the log list are not implemented yet.
Co-authored-by: Kobsel <[email protected]> Co-authored-by: benedikt brunner <[email protected]>
There was a problem hiding this comment.
Muy bueno, there were a few comments where i would have adjusted the wording, but let's just make an issue for reviewing all comments across the repository and push that towards the end of the project. For now it's much more important to get this huge chunk of code into the repo and take that momentum into the next iteration. 👍
| self.menu_bar = MenuBar(self, menu_bar_callbacks) | ||
|
|
||
| # Initialize the graph | ||
| self.graph = Graph(self) | ||
|
|
||
| # Initialize the checkbox panel | ||
| self.checkbox_panel = CheckboxPanel(self) |
| if TYPE_CHECKING: | ||
| from napytau.gui.app import App # Import only for the type checking. |
There was a problem hiding this comment.
Props for going above and beyond for static analysis 💪
Summary
This adds a menu bar to the GUI, as well as checkboxes for selecting which datapoints to use for the fitting of the graph and the calculation of tau.
It also replaces the previous UI mockup.
PR checklist
How to test
There are no end-to-end tests for the UI yet, therefore you can only check the GUI manually.
You can check if it is working properly by making sure it starts without errors.
All buttons should be able to be pressed without errors.
Buttons where the backend functionality is not yet implemented will at least give an output to the developer terminal.
Closes/Fixes/Related to