Ecdc 3633 add component window to widget #1044
Conversation
Co-authored-by: Matt Clarke <[email protected]>
| @@ -128,11 +132,13 @@ def _confirm_cancel(self) -> bool: | |||
|
|
|||
| def _cancel_new_group(self): | |||
| if self._confirm_cancel(): | |||
| self._rejected() | |||
There was a problem hiding this comment.
Have you tried seeing what happens if you spam the edit button after selecting a group?
There was a problem hiding this comment.
I have reintroduced the bug where multiple panes appear. I don't think the relevant code is in this part but in mainwindow.py .
There was a problem hiding this comment.
I didn't pick this piece of code specifically. I just needed somewhere to report it and was too lazy to do a proper comment :D
There was a problem hiding this comment.
had to check the attribute existed again but it's a cleaner fix than initially (no try catch)
| if hasattr(self, "add_component_window") and not self.add_component_window.isHidden(): | ||
| self.add_component_window._rejected() | ||
| self.add_component_window.setHidden(True) | ||
|
|
There was a problem hiding this comment.
This is just a variation of the hack you were unhappy with last week.
It is also just papering over the cracks.
The question to ponder is: why are we creating a new AppComponentDialog every time?
There was a problem hiding this comment.
I see the point - the answer is because each component is different in terms of parent node, type etc. You can see in :120 when we edit a group the difference is we pass a different flag which states whether we use empty groups or existing groups. It doesn't simply 'fill in' the dialog/window etc, but creates a new object that has many members initialised with particular values which are important for various methods in the dialog (for example, remembering where the component goes after it is made, or remembering the initial state of the group so it can be restored if the user cancels it). However, there are various object inside the AddComponentDialog class, such as the main UI layout, that mean it makes sense to have as a separate part (rather than as part of this MainWindow class). Finally, the dialog is deleted afterwards so we are not getting a memory leak or anything like that.
I've removed the hasattr check, and now we have have a clean and consistent way of handling the dialog
There was a problem hiding this comment.
This is not clean.
Repeated clicks of the edit button each create a new AppComponentDialog which are possibly not destructed (I see the memory usage increase on my machine at least).
It should not be necessary to construct a new AppComponentDialog every time just to pass some values into it, i.e. add a method something like def set_initial_values(group, is_new_group).
nexus_constructor/main_window.py
Outdated
| @@ -60,6 +60,7 @@ def setupUi(self, main_window): | |||
| self.component_tree_view_tab.set_up_model(self.model) | |||
| self._update_views() | |||
| self.simple_tree_view.triggered.emit() | |||
| self.add_component_window = QWidget() | |||
There was a problem hiding this comment.
This is where you should create the (reusable) AppComponentDialog.
…nent_window_to_widget' into ECDC-3633-add_component_window_to_widget
…nent_window_to_widget' into ECDC-3633-add_component_window_to_widget
Issue
We want to move the add component window from being a window to being a widget. The behaviour should be similar, but NOT block the navigation of the tree or other components. The behaviour should otherwise be the same.
Description of work
QDialog is a child of QWidget, so that is simple to change. However, the window object is normally deleted, and that is something we want to avoid, so instead we use the hidden attribute. The height of the window changes based on what it is populated with.
Acceptance Criteria
Files associated with the 'add component' window
UI tests
All tests still valid
Nominate for Group Code Review