Fix droppable instance editor #1733
Conversation
… fix an observe error by adding an observer in the droppable but not selectable case
| with tester.create_ui(obj, {'view': non_editable_droppable_view}) as ui: | ||
| pass | ||
|
|
||
| with tester.create_ui( | ||
| obj_with_list, {'view': non_editable_droppable_selectable_view} | ||
| ) as ui: | ||
| pass |
There was a problem hiding this comment.
We previously both of these caused a failure / crash.
| Item( | ||
| "inst", | ||
| editor=InstanceEditor( | ||
| editable=False, | ||
| droppable=True | ||
| ), | ||
| style='custom', | ||
| ), | ||
| buttons=["OK"], | ||
| ) |
There was a problem hiding this comment.
I have no idea why one would want to do this... but it is a possible configuration one could make. This would be when you are "editing" a single instance, but the single instance itself is not editable, so all that is displayed in a QLineEdit displaying the current instance. The instance can be changed by dragging and dropping a different instance. The user can not select a different instance as theoretically there is no list to select from.
traitsui/qt4/instance_editor.py
Outdated
| self.factory.observe( | ||
| self.rebuild_items, "values.items", dispatch="ui" | ||
| ) |
There was a problem hiding this comment.
The more I think about this it doesn't really make sense. This was added because without it we were getting a NotifierNotFound error in the non_editable_droppable_view case. However, the only time this occurs is the use case described here: https://github.com/enthought/traitsui/pull/1733/files#r672368160
In which case values.items should never change. (There isn't a list of instances, just the one!).
Thinking about this more I think it probably makes more sense to check for this case when we arer removing the listener (and not try to remove it), rather than just adding it here to avoid the failure.
This code path is rather confusing. Using _choice in this case doesn't really make sense. At least not to me anyway, as we are not really choosing an instance from a list of options...
|
@aaronayres35 is this PR ready for review? |
| "values.items", | ||
| remove=True, | ||
| dispatch="ui" | ||
| ) |
There was a problem hiding this comment.
Do we also want to remove the other observer hooked up if factory.name != ""? Ref
traitsui/traitsui/qt4/instance_editor.py
Lines 109 to 112 in 6c0e717
There was a problem hiding this comment.
I believe this is already getting unhooked above
EDIT: However looking closer, the conditionals for hooking it up don't seem to match the conditionals for unhooking. unhooking we check for self._object, which only gets set in this code:
if factory.name != "":
self._object, self._name, self._value = self.parse_extended_name(
factory.name
)
so that at least is effecitvely the same though. It is set up in the if selectable block though so that unhooking could probably be wrapped in an if isinstance(self._choice, QtGui.QComboBox): as well...
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
|
@rahulporuri does the latest change with unhooking the observer still look okay? |
|
still LGTM but this PR doesn't have a news fragment |
closes #1478
Checklist