Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Save entries by default in the Form Dialog #97
Save entries by default in the Form Dialog #97
Changes from all commits
906dd76
c143250
071aa0c
9473391
2e05d97
0538ef5
46231b5
d2cfb9a
ffe74e4
00dbb7a
6be5a9c
4c5ca41
540f27e
006e4a9
54235ec
424d6b4
92d1332
94f172e
d6d2cc6
73f419c
ccdf0fa
05ff037
d3bf171
90ea8e8
1ecc16a
69701cd
1afac70
6643167
970be18
53104e8
e99a648
f9a342b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you, I will open an issue with this. #113