Convert dynamic on_trait_change uses to observe #870

aaronayres35 · 2021-01-25T21:43:03Z

part of #732 and #637

… broken)

aaronayres35 · 2021-01-25T22:13:01Z

Im currently getting a strange segfault I am trying to debug. Somehow adding print(handler) directly above this line https://github.com/enthought/traits/blob/743531b45fa7321ede9b2009b1dcee47ed0172a4/traits/observation/_observer_change_notifier.py#L85
prevents the segfault (but tests still error)

aaronayres35 · 2021-01-25T22:18:08Z

pyface/ui/wx/action/action_item.py

+        action.observe(self._on_action_enabled_changed, "enabled", remove=True)
+        action.observe(self._on_action_visible_changed, "visible", remove=True)
+        action.observe(self._on_action_checked_changed, "checked", remove=True)
+        action.observe(self._on_action_name_changed, "name", remove=True)


For some reason we don't remove the listener for "image" here...? I left code to match it as is but this feels like a bug? Additionally for some of the occurrences below, we do not remove

Note for future ref: Maybe the segfault was related to this?

Yep, definitely should be removing anything that is added.

aaronayres35 · 2021-01-25T22:19:05Z

examples/explorer.py

        """ Called when the selection in the tree is changed. """
-
+        selection = event.new
        if len(selection) > 0:
            self._table_viewer.input = selection[0]


This example seems to not be run-able on qt or wx. I will look if there is already an issue and open one if not.

aaronayres35 · 2021-01-25T22:20:05Z

pyface/ui/qt4/action/menu_manager.py

+        self._manager.observe(self._on_enabled_changed, "enabled")
+        self._manager.observe(self._on_visible_changed, "visible")
+        self._manager.observe(self._on_name_changed, "name")
+        self._manager.observe(self._on_image_changed, "image")


The manager/its subclasses do not explicitly define an "image" trait

…thing is broken)" This reverts commit 297a448.

aaronayres35 · 2021-01-26T19:37:31Z

I reverted the changes to menu_manager.py for now as I do not understand/want to deal with the segfault in this PR. The segfault was occurring in test_action_item.py. This PR does make changes to action_item.py, and I'm uncertain if they were related to the segfault. I don't believe they are and thus am leaving those changes in this PR (Tests should all still be passing). However, if a reviewer sees something I am not please make it known.

Also, looking at this PR in its current state, it is already getting decently large so I am tempted to just arbitrarily stop here and flag it for review... Then I will leave the remaining changes for a second PR.
Maybe I will go through and look for any other very simply updates that can be made to tack on here.

aaronayres35 · 2021-01-26T20:53:55Z

Aha! looks like segfault is still occurring on CI, but I no longer am able to reproduce locally. I suspect the changes in action_item.py are responsible. I will revert that commit for this PR as well and investigate later on

This reverts commit b27e19e.

rahulporuri

LGTM with a bunch of style recommendations.

examples/application/python_editor/python_editor_application.py

examples/grid.py

examples/node_tree.py

examples/tree.py

examples/tree_viewer.py

pyface/ui/wx/wizard/wizard.py

pyface/wizard/chained_wizard_controller.py

pyface/wizard/i_wizard.py

pyface/wizard/wizard_controller.py

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

aaronayres35 · 2021-02-01T15:23:57Z

ref: #876

aaronayres35 · 2021-02-01T15:48:48Z

Latest round of changes has CI failing. Looking into all the places where we need to explicitly pass event=None now specifically at timers and their callbacks

aaronayres35 · 2021-02-01T15:58:41Z

@rahulporuri I tried to fix the CI failures in the latest commit 🤞 . However, the fixes seem very fragile, and I am not confident I didn't miss anything. We may be introducing bugs that are flying under the radar because they are not covered in tests. In other words, in the latest commit, I was trying to identify any ways that handler.callback could be called so I could make sure it always was called with the correct event=None passed in. I believe CI should pass on this commit now / that I have found the ones related to CI failures but I am uncertain I have found all of them.

I understand style wise we want to go with just passing event, but it seems to me we need to make sure we aren't risking any bugs... If instead of all the changes in the latest commit I simply have the ConditionHandler's callback method specify event=None as a kwarg, CI will also pass and I am more confident existing code will not break...

What are your thoughts about this?

EDIT: see latest commit, where I just went ahead and did the "refactor" approach. I think this is probably the go here and in general. The only thing to note: when we do the refactor approach we will probably want to try to leave the current handler with the same name without any arguments as a standard method and then redefine a new handler that we then observe that has the event arg. This is because we may not always be certain of all the code paths that could be taken to call the handler standalone, but we will know where it is explicitly called with observe. Maybe thats not entirely true... In any case we need to be cautious to not break things!

This reverts commit 4d6e724.

rahulporuri · 2021-02-01T16:44:44Z

pyface/timer/tests/test_timer.py

+        timer = CallbackTimer(
+            callback=handler.callback, kwargs={'event': None}
+        )


it is better to simply update the ConditionHandler.callback method to set event=None instead of updating all of the callers

That makes sense to me, should I do it where I simply have event=None as a kwarg on callback then? Or is the most recent solution with _callback better?

I think just having it as a kwarg makes the most sense to me

I think just having it as a kwarg makes the most sense to me

Let's go with that approach - instead of the refactor.

This reverts commit 564e167.

aaronayres35 · 2021-02-01T17:09:58Z

@rahulporuri I was thinking of looking into making sure we remove and listeners we set up before we push this through. Should I do that, or make those fixes in a separate PR (since they didn't exist with otc before, so this PR should preserve behavior, but ultimately we will want them addressed).
I am fine either way, if you want me to try and tack those changes onto this PR, or just push this through and do them separately (in the second case I will be sure to open a new issue as well)

EDIT: on second thought I think I will do this separately. Many (most?/all?) of the cases are such that the code is transient, e.g. examples, and some tests. Still worth looking into, but I think I will leave them for a separate issue/PR

rahulporuri

still LGTM, assuming CI is happy

rahulporuri · 2021-02-01T18:01:09Z

EDIT: on second thought I think I will do this separately. Many (most?/all?) of the cases are such that the code is transient, e.g. examples, and some tests. Still worth looking into, but I think I will leave them for a separate issue/PR

that's fine by me but do open an issue

aaronayres35 added 11 commits January 25, 2021 13:14

update a few examples to use dynamic observe instead of otc

4f4c6fd

update other examples

3d06c59

update docstring of application.py to use observe in example

d6f3266

use dynamic observe in action/field_action.py

55dbdf8

update dock/dock_window.py

6351248

update a few test files to use dynamic observe not otc

be8d9e7

update tasks/action/task_window_toggle_group.py

620c057

update tasks/action/task_window_toggle_group.py

4f6f9d4

updates for both wx and qt action_item.py

b27e19e

updates for tree/node_tree_model.py

b598f06

first attempt at updating ui/qt4/action/menu_manager.py (something is…

297a448

… broken)

aaronayres35 commented Jan 25, 2021

View reviewed changes

Revert "first attempt at updating ui/qt4/action/menu_manager.py (some…

8cb3609

…thing is broken)" This reverts commit 297a448.

aaronayres35 added 11 commits January 26, 2021 13:43

remove print statement from debugging

47643e8

update test_gui.py

0a6e7bd

update qt4 gui_test_assistant

5dbc777

update wizard_controller.py and chained_wizard_controller.py

97b43d2

update i_wizard.py

93704b2

update qt wizard_page.py

e8052f7

update wx switcher and lazy_switcher and remove some commented out code

9dfb95d

update wx and qt workbench_window_layout

e16b54f

update workbench.py

96b2dd1

typo

ef2b10d

update wx expandable_panel.py

1167355

Revert "updates for both wx and qt action_item.py"

617622e

This reverts commit b27e19e.

aaronayres35 closed this Jan 27, 2021

aaronayres35 reopened this Jan 27, 2021

aaronayres35 changed the title ~~[WIP]Convert dynamic OTC uses to observe~~ Convert dynamic OTC uses to observe Jan 27, 2021

aaronayres35 requested a review from rahulporuri January 27, 2021 13:29

rahulporuri approved these changes Feb 1, 2021

View reviewed changes

aaronayres35 and others added 4 commits February 1, 2021 09:11

Apply suggestions from code review

3d0dc93

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

add comment about use of match and listening to all traits

1a92a5b

missed one in last commit

15e4af7

pass event=None to handler as it is called standalone

4507f4c

try to maake ci green

4d6e724

aaronayres35 added 2 commits February 1, 2021 10:16

Revert "try to maake ci green"

c65f01d

This reverts commit 4d6e724.

maybe this approach is bettter

564e167

rahulporuri reviewed Feb 1, 2021

View reviewed changes

rahulporuri changed the title ~~Convert dynamic OTC uses to observe~~ Convert dynamic on_trait_change uses to observe Feb 1, 2021

aaronayres35 added 2 commits February 1, 2021 11:01

Revert "maybe this approach is bettter"

da2bc10

This reverts commit 564e167.

keep it simple

9552c9b

rahulporuri approved these changes Feb 1, 2021

View reviewed changes

typo

0f84c8a

aaronayres35 merged commit 254a3ae into master Feb 1, 2021

aaronayres35 deleted the replace-dynamic-otc-observe-simple branch February 1, 2021 17:37

aaronayres35 mentioned this pull request Feb 1, 2021

Make sure all trait listeners get unhoooked #877

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert dynamic on_trait_change uses to observe #870

Convert dynamic on_trait_change uses to observe #870

aaronayres35 commented Jan 25, 2021 •

edited

aaronayres35 commented Jan 25, 2021

aaronayres35 Jan 25, 2021

aaronayres35 Jan 26, 2021

corranwebster Jan 27, 2021

aaronayres35 Jan 25, 2021

aaronayres35 Jan 25, 2021

aaronayres35 commented Jan 26, 2021

aaronayres35 commented Jan 26, 2021

rahulporuri left a comment

aaronayres35 commented Feb 1, 2021

aaronayres35 commented Feb 1, 2021

aaronayres35 commented Feb 1, 2021 •

edited

rahulporuri Feb 1, 2021

aaronayres35 Feb 1, 2021

rahulporuri Feb 1, 2021

aaronayres35 commented Feb 1, 2021 •

edited

rahulporuri left a comment

rahulporuri commented Feb 1, 2021

Convert dynamic on_trait_change uses to observe #870

Convert dynamic on_trait_change uses to observe #870

Conversation

aaronayres35 commented Jan 25, 2021 • edited

aaronayres35 commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronayres35 commented Jan 26, 2021

aaronayres35 commented Jan 26, 2021

rahulporuri left a comment

Choose a reason for hiding this comment

aaronayres35 commented Feb 1, 2021

aaronayres35 commented Feb 1, 2021

aaronayres35 commented Feb 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronayres35 commented Feb 1, 2021 • edited

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri commented Feb 1, 2021

aaronayres35 commented Jan 25, 2021 •

edited

aaronayres35 commented Feb 1, 2021 •

edited

aaronayres35 commented Feb 1, 2021 •

edited