Convert on_trait_change decorators to observe #1519
Conversation
| @observe('guests.items') | ||
| def _guests_modified(self, event): | ||
| if isinstance(event.object, Hotel): | ||
| for guest in event.new: | ||
| guest.hotel = self | ||
| else: | ||
| for guest in event.added: | ||
| guest.hotel = self |
There was a problem hiding this comment.
This is similar to the commit immediately prior. It is unclear me how we want to handle cases where we were listening to container[] and now observe container.items, but the handler previously expeced to have added and removed. Changes to container will fire a TraitChangeEvent which has object, name, old, new, whereas changes to an item in the container will have a TraitListEvent or the like which allows us to access added/removed. Also this example currently is broken with the following error when you click "add guest":
ERROR:traits:Exception occurred in traits notification handler for object: <__main__.Hotel object at 0x7f8a298bb570>, trait: guests_items, old value: <undefined>, new value: TraitListEvent(index=4, removed=[], added=[<__main__.Guest object at 0x7f8a29a650f8>])
Traceback (most recent call last):
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traits/trait_notifiers.py", line 524, in _dispatch_change_event
self.dispatch(handler, *args)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traits/trait_notifiers.py", line 619, in dispatch
handler(*args)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/list_editor.py", line 559, in update_editor_item
ui, view_object, monitoring = self._create_page(object)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/list_editor.py", line 657, in _create_page
parent=self.control, view=factory.view, kind=factory.ui_kind
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traits/has_traits.py", line 1744, in edit_traits
args,
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/view.py", line 455, in ui
ui.ui(parent, kind)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/ui.py", line 237, in ui
self.rebuild(self, parent)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/toolkit.py", line 157, in ui_subpanel
ui_panel.ui_subpanel(ui, parent)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 72, in ui_subpanel
_ui_panel_for(ui, parent, True)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 78, in _ui_panel_for
ui.control = control = _Panel(ui, parent, is_subpanel).control
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 140, in __init__
self.control = panel(ui)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 269, in panel
panel = _GroupPanel(content[0], ui).control
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 627, in __init__
layout = self._add_items(content, inner)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/qt4/ui_panel.py", line 893, in _add_items
ui, object, name, item.tooltip, None
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/editors/range_editor.py", line 283, in simple_editor
ui
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/editors/range_editor.py", line 193, in _get_low_high
low = self.named_value(self.low_name, ui)
File "/Users/aayres/.edm/envs/traitsui-test-3.6-pyqt5/lib/python3.6/site-packages/traitsui/editor_factory.py", line 108, in named_value
value = getattr(value, name)
AttributeError: 'NoneType' object has no attribute 'min_temperature'
This seems to originate for the _add_guest_changed method below.
We should not merge this PR until this is fixed
There was a problem hiding this comment.
I wonder if it's better to split the listener into two separate methods - one for guests and another for guests:items - this way, we can handle things cleanly.
There was a problem hiding this comment.
I think this question should also be discussed on the internal slack channel - so that we can settle on a consensus for what the right thing to do is. IMO, the best thing is to separate the listeners.
There was a problem hiding this comment.
Will post a question shortly after I go through the other suggested changes
There was a problem hiding this comment.
I think this specific piece of code can stay as is. We don't need to separate it into two methods - which listen to guests vs guests:items. But, i think it's definitely useful to add a comment differentiating the two branches - it took me a little time to understand which branch handled what.
There was a problem hiding this comment.
I will add a comment. I am unsure exactly where the failure is coming from. It seems to originate in _add_guest_changed not the _guests_modified method that we changed here. However, this error is not observed (at least loudly) without these changes. It is possible something had been broken perviously but on trait change didn't make a fuss about it
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
traitsui/key_bindings.py
Outdated
| if isinstance(event.object, KeyBinding): | ||
| for item in event.new: | ||
| item.parent = self | ||
| else: | ||
| for item in event.added: | ||
| item.parent = self |
There was a problem hiding this comment.
same comment here - a short comment explaining the difference between the two branches would be useful here i think.
|
|
||
| def _add_guest_changed(self): | ||
| self.guests.append(Guest()) | ||
| self.guests.append(Guest(hotel=self)) |
There was a problem hiding this comment.
this fixes the previous error, but now it feels like listening to items above is pointless.... As I believe (for this demos sake) the only way guests.items will be changed is via the add_guest button.
The cause of this issue is still mysterious to me. It seems that when the Guest object is instantiated, we have the following trait definitions that need to be sorted out:
# The maximum temperature allowed by the guest's plan:
max_temperature = Property(depends_on='plan')
# The current room temperature as set by the guest:
temperature = Range('hotel.min_temperature', 'max_temperature')
however, at the time of instantiation the hotel is not defined (when we have the observe decorated listener above, but with on trait change before this worked fine).
To prevent the error mentioned #1519 (comment) I simply specify the hotel to be self at the time we try to create the Guest object. But that makes the need to set guest.hotel = self above completely redundant...
This all feels somewhat fundamental to the purpose of the demo, but I don't think my understanding of the the dynamic range is entirely there to fully make sense of this.
There was a problem hiding this comment.
@rahulporuri shall I remove the code for listening to items in the above listener now? Seems strange this example worked before but not with observe without this change on line 150
There was a problem hiding this comment.
i'm not sure it's worth the effort figuring out what the best way is to structure this example. let's leave it as is for now.
contributes to #1052
This PR replaces all uses of
@on_trait_changewith the equivalent form using observe