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.

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

Extend / document secret TreeNode api for passing tuples into add list trait #1527

Merged
merged 7 commits into from
Mar 16, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Feb 25, 2021

closes #1502

This is part 2 from this comment: #1518 (comment) which follows up the changes made in #1524.

Previously there was already a secret part of the TreeNode api in which users could pass in a 2-tuple as an element in the list supplied for the add trait of a TreeNode. The first element of the tuple would be used as the klass to be added, and the second was a prompt boolean, which when true would call edit_traits on the new object before fully adding it to the tree to prompt the user to change traits if they want to. I did not see this actually documented anywhere, but from the code I believe that was the idea (and testing locally that is what happens).
In this PR I simply extend that secret api to now also potentially accept a 3-tuple, where the last tuple element in a factory callable that when called produces an instance of klass. I did not enforce, but I imagine in such cases there is a high probability of us wanting prompt=True. I've documented the old and new secret options on the add trait so that it shows up in the api docs. Further I've added a demo showcasing the functionality. The added demo is exactly the example code given in the original issue, modified to use this new functionality.

The specific tuple checking is a little weird and maybe we want to refactor / re-engineer how we go about doing this. This was just a first stab at a solution that made use of something already pretty much in place in the api, so it was pretty easy to introduce.

Another important thing to note: I made the change to now be loud in _menu_new_node if factory() fails because users have a means for doing the right thing now. We may not want to do this though to avoid breaking existing code / forcing them to do the new thing. i.e. before they have a HasRequiredTraits class and trying to add a new node does nothing (doesn't fail though) to now trying to do that leads to failure. Forcing them to pass a 3-tuple with factory now.
Actually as I am writing this I am realizing it is probably too early to start being loud there... But also if we are not loud people may never notice the new functionality and never switch...?
I am happy to include or remove this change as a reviewer sees fit, I am not sure what the correct approach is. I imagine we are going to want to stay quiet about it at least for now.

Also, this PR still needs a test! I am unsure of how to best do this as UI Tester still doesn't support TreeEditor and IDK if I should be explicitly testing the individual private methods that were changed... 🤔

@aaronayres35
Copy link
Contributor Author

aaronayres35 commented Feb 25, 2021

Very strange seemingly unrelated CI failure...:

======================================================================
FAIL: test_qt_process_events_process_all (traitsui.testing.tests.test_gui.TestProcessEventsRepeated)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traitsui\testing\tests\test_gui.py", line 154, in test_qt_process_events_process_all
    self.assertEqual(n_left_behind_events, 0, msg)
AssertionError: 8 != 0 : Expected 10 events processed on the objects and zero events left on the queue after running process_cascade_events. Found 2 processed with 8 left behind.

----------------------------------------------------------------------

ref: #951

Going to try re-running the jobs as 951 seems to indicate this is an occasionally occurring thing

Also I am seeing error messages in the logs that aren't resulting in test failures, but they appear related to the recent otc->observe PRs:

test_qt_tuple_editor (traitsui.tests.editors.test_tuple_editor.TestTupleEditor) ... Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<traitsui.key_bindings.KeyBindings object at 0x000001625E102468>, name='children', old=[<traitsui.key_bindings.KeyBindings object at 0x000001625D9A3AF0>], new=[])
Traceback (most recent call last):
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traits\observation\_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traits\observation\observe.py", line 26, in dispatch_same
    handler(event)
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traitsui\key_bindings.py", line 267, in _children_modified
    for item in event.added:
AttributeError: 'TraitChangeEvent' object has no attribute 'added'
ok
test_value_update (traitsui.tests.editors.test_tuple_editor.TestTupleEditor) ... Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<traitsui.key_bindings.KeyBindings object at 0x0000016259C16468>, name='children', old=[<traitsui.key_bindings.KeyBindings object at 0x0000016259C5CE60>], new=[])
Traceback (most recent call last):
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traits\observation\_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traits\observation\observe.py", line 26, in dispatch_same
    handler(event)
  File "D:\a\traitsui\traitsui\.edm\envs\traitsui-test-3.6-pyqt\lib\site-packages\traitsui\key_bindings.py", line 267, in _children_modified
    for item in event.added:
AttributeError: 'TraitChangeEvent' object has no attribute 'added'
ok

Comment on lines 1063 to 1065
klass, prompt, *factory_list = klass
if factory_list:
factory = factory_list[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just do an explicit if len(...) == 2: ... elif len(...) == 3: ... here. Clearer for people reading the code, I think. You could also pull the factory = None into the length 2 case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually event better, you could do factory = klass in the length 2 case here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, not also need to handle the case where we're just given a bare class.

@corranwebster
Copy link
Contributor

This seems reasonable, given that the whole thing is fairly hacky to start with.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

This is OK, modulo the is None check.

Co-authored-by: Corran Webster <cwebster@enthought.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants