Save entries by default in the Form Dialog #97
Conversation
There was a problem hiding this comment.
Hi, here are some initial comments before I look more in depth at the unit tests (so I would say hold off on actioning my comments on the unit tests so far). Just wanted to submit this first review as is because of the bug I found in the behaviour, and I will review again later today with more detailed suggestions for the unit tests targeting the removal of opening the dialog etc.
Unfortunately, I found the dialog_example_3_save_default example didn't work as I expected. I opened the dialog, changed some content of the dialog and pressed cancel. Then found that the content I changed was still there. I think this only happens if you press cancel before you have ever pressed Ok.
I was also surprised to find that when I ran the dialog_example_2 it didn't now save the states - do you have any insight as to why? It may be the case that we only need one example - I don't think that this example and dialog_example_2 should in theory really be showing different behaviour unless there's something that I have missed.
|
@lauramurgatroyd when you are happy with the PR changes I will resolve the merge conflicts locally. |
test/test__formUI_status_test.py
Outdated
| self.assertEqual(states0, self.form.getAllWidgetStates()) | ||
| self.click_Cancel() | ||
| self.assertEqual(states1, self.form.getAllWidgetStates()) | ||
|
|
||
| def test_getWidgetState_returns_QLabel_value(self): |
There was a problem hiding this comment.
Once we have decided on the method for applying the default widget values, please add a test for this.
There was a problem hiding this comment.
After our discussion from yesterday, I do not think it is worth adding a test as I just invoked your methods. What do you think? (see new commits from today)
|
I merged the code with main (remove widget PR already included). The remove widget is not compatible with the default states because it deletes the widgets and the default cannot be restored. In particular, when we remove widgets and we press cancel, there is no way to restore the default widgets. The remove widget method needs to be changed for this PR to be merged with main. Week 27 Nov - 1 Dec After discussions with @paskino, we decided that we will save the removed widgets in a new dictionary. Thus, the remove widgets method will only hide the widgets rather than deleting them completely. In order to finalise this PR, we need to use the insert widget method (PR #109) and widget number to reinstate the widgets in the correct location in the layout (issue #105 and issue #95). Week 04 Dec - 08 Dec We decided to merge this PR and leave the remove widget example returning errors when cancel is clicked. We will fix the problem in the other PRs. |
| ''' | ||
| if hasattr(self, 'widget_states'): | ||
| if not hasattr(self, 'widget_states'): | ||
| self.set_default_widget_states_visible_true() |
There was a problem hiding this comment.
Why do we do this? What if the default state of a widget is that it is invisible and we want to re-apply that?
There was a problem hiding this comment.
Is it because the default states are added when the widgets are added which could be when the form is closed?
If so, should it be instead that the default state is saved upon first opening the form?
There was a problem hiding this comment.
Perhaps this is a later problem and we may need to open an issue about rethinking how the visibility is saved.
There was a problem hiding this comment.
I am happy with the way I saved the visibility: they are "not visible" in the default states because when we created the widgets they were, indeed, non visible.
If I want to apply default states which are not visible to a visible form, the restoration step should inherit the visibility of the form. I tried to change the visibility after the cancel button was clicked and the form was closed hoping the widgets would inherit the visibility from the opened form but I was not successful. I do agree with you and this was not done optimally here.
I cannot think of an example where we would want to restore non visible default states to a visible form.
I have addressed some problems with saving default states in my next PR. Hopefully once this and the next PR are merged, we could optimise these procedures before the milestone?
There was a problem hiding this comment.
Anyway, I think when we click the cancel button the form is open so it is not wrong to set the visibility true in that step.
There was a problem hiding this comment.
The example I was thinking of would be a case where the default case has some advanced widgets that are by default hidden and could be shown using a checkbox or similar.
But I can see the docstring explains what is happening with the visibility being restored as True so I am happy to approve
There was a problem hiding this comment.
Ok thank you, I will open an issue with this. #113
Closes #87
UIFormWidgetto populate the default widget states dictionary and one to make default widget states visible.Okis clicked and restores the previously saved states or the default states whenCancelis clicked.exampleState,set_stateandtest_dialog_buttons_default_behaviourto the unit tests.