Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix droppable instance editor #1733

Merged
merged 7 commits into from Jul 21, 2021
Merged

Fix droppable instance editor #1733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fab3308
add regression test
aaronayres35 Jul 19, 2021
905c84a
fix bug by moving set up of DropEventFilter down in init method. Als…
aaronayres35 Jul 19, 2021
1c822a5
flake8
aaronayres35 Jul 19, 2021
6c0e717
dont add an unused observer, instead just don't remove it if it wasn'…
aaronayres35 Jul 19, 2021
6b2a42d
Apply suggestions from code review
aaronayres35 Jul 21, 2021
277426c
fix observer unhooking conditionals
aaronayres35 Jul 21, 2021
4d096e8
add news fragment
aaronayres35 Jul 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/releases/upcoming/1733.bugfix.rst
View file
Open in desktop
@@ -0,0 +1 @@
Fix droppable InstanceEditor (#1733)
35 changes: 22 additions & 13 deletions traitsui/qt4/instance_editor.py
View file
Open in desktop
Expand Up @@ -122,11 +122,6 @@ def init(self, parent):
self._choice.setReadOnly(True)
self.set_tooltip(self._choice)

if droppable:
# Install EventFilter on control to handle DND events.
drop_event_filter = _DropEventFilter(self.control)
self.control.installEventFilter(drop_event_filter)

orientation = OrientationMap[factory.orientation]
if orientation is None:
orientation = self.orientation
Expand Down Expand Up @@ -155,6 +150,11 @@ def init(self, parent):
layout.setContentsMargins(0, 0, 0, 0)
self.create_editor(parent, layout)

if droppable:
# Install EventFilter on control to handle DND events.
drop_event_filter = _DropEventFilter(self.control)
self.control.installEventFilter(drop_event_filter)

# Synchronize the 'view' to use:
# fixme: A normal assignment can cause a crash (for unknown reasons) in
# some cases, so we make sure that no notifications are generated:
Expand Down Expand Up @@ -289,7 +289,10 @@ def update_editor(self):
else:
choice.setText(name)
else:
choice.setCurrentIndex(-1)
# choice can also be a QLineEdit in which case we just leave
# text as empty
if isinstance(choice, QtGui.QComboBox):
choice.setCurrentIndex(-1)


def resynch_editor(self):
Expand Down Expand Up @@ -357,18 +360,24 @@ def dispose(self):
self._ui.dispose()

if self._choice is not None:
if self._object is not None:
self._object.observe(
# _choice can also be a QLineEdit in which case we never set up
# this observer.
if isinstance(self._choice, QtGui.QComboBox):
if self._object is not None:
self._object.observe(
self.rebuild_items,
self._name + ".items",
remove=True,
dispatch="ui"
)

self.factory.observe(
self.rebuild_items,
self._name + ".items",
"values.items",
remove=True,
dispatch="ui"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if factory.name != "":
self._object.observe(
self.rebuild_items, self._name + ".items", dispatch="ui"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


self.factory.observe(
self.rebuild_items, "values.items", remove=True, dispatch="ui"
)

super().dispose()

def error(self, excp):
Expand Down
37 changes: 37 additions & 0 deletions traitsui/tests/editors/test_instance_editor.py
View file
Open in desktop
Expand Up @@ -97,6 +97,29 @@ def _modify_inst_list(self, event):
Item('change_options'),
buttons=["OK"],
)
non_editable_droppable_view = View(
Item(
"inst",
editor=InstanceEditor(
editable=False,
droppable=True,
),
style='custom',
),
buttons=["OK"],
)
non_editable_droppable_selectable_view = View(
Item(
"inst",
editor=InstanceEditor(
name='inst_list',
editable=False,
droppable=True,
),
style='custom',
),
buttons=["OK"],
)
modal_view = View(
Item("inst", style="simple", editor=InstanceEditor(kind="modal"))
)
Expand Down Expand Up @@ -389,3 +412,17 @@ def test_none_selected(self):
self.assertIsNone(obj.inst)
text = instance.inspect(SelectedText())
self.assertEqual(text, '')

# regression test for enthought/traitsui#1478
def test_droppable(self):
obj = ObjectWithInstance()
obj_with_list = ObjectWithList()
tester = UITester()

with tester.create_ui(obj, {'view': non_editable_droppable_view}):
pass

with tester.create_ui(
obj_with_list, {'view': non_editable_droppable_selectable_view}
):
pass