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
implements __copy__ for widgets #2436
base: main
Are you sure you want to change the base?
Conversation
@DanielAristidou thanks for your PR. The best practice with GitHub PRs is to not open them from your master branch, but from a feature branch, otherwise, you may have trouble updating your master branch to match master. Regarding the content, I am adding some comments inline. |
new_state = {key: value for key, value in self.get_state().items() if | ||
not key.startswith('_') and | ||
not key == 'layout' and | ||
not key == 'style'} |
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.
Maybe the state could be directly passed to __init__
as kwargs.
Also, rather than overriding the method for each widget type that may have subwidget, a good way to exclude things may be to exclude attributes that are of type Widget?
A list of synchronized keys is available in the keys
attribute.
Finally, I don't quite know if there may be other state in the object that is no synchronized with the front-end, and would need to be included in a copy operator...
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 putting the PR in "request changes" state because I am not sure how we should tackle this in terms of semantics.
Unfortunately, Python does not have a clear value semantics so we cannot guarantee that members all implement properly copy which may be a good argument for a shallow copy approach.
Note that the |
implements copy for widgets
implemented on base widget - and
Follows the following steps:
create a new widget from the reference class.
initialises the widget passing any required arguments.
reads all relevant attributes and sets them on the new widget
fixes #2352