Convert dynamic on_trait_change to observe part 2 #880

aaronayres35 · 2021-02-04T16:31:38Z

closes #637
closes #732

This PR makes the last round of updates to replace use of on_trait_change with observe. With the changes in this PR, on_trait_change is no longer present anywhere in the code base. This PR also makes an update to the documentation section on timers. Please confirm the changes made there are reasonable.

Note to reviewer, be cautious: some of the code undergoing changes in this PR is not covered by the test suite. I made some knowingly broken changes and ran the test suite only to find everything still passing.

This PR also drive by removes some commented out code from the code base.

… broken)

… does not even have an image trait so setting up observers for it doesn't make sense

aaronayres35 · 2021-02-04T16:54:32Z

pyface/ui/wx/grid/grid.py

+        smobs(self._on_model_content_changed, "content_changed", remove=True)
+        smobs(
+            self._on_model_structure_changed, "structure_changed", remove=True
+        )
+        smobs(self._on_row_sort, "row_sorted", remove=True)
+        smobs(self._on_column_sort, "column_sorted", remove=True)


although these are observers on the model not the grid itself I assume they should still be removed when the grid is disposed (notably because the handlers are methods on a mid disposal grid object)

aaronayres35 · 2021-02-04T17:03:22Z

pyface/ui/wx/preference/preference_dialog.py

@@ -134,7 +134,7 @@ def _create_tree(self, parent):
            content_provider=DefaultTreeContentProvider(),
        )

-        tree_viewer.on_trait_change(self._on_selection_changed, "selection")
+        tree_viewer.observe(self._on_selection_changed, "selection")


I can't comment on the line itself, but this file has from pyface.ui.wx.split_dialog import SplitDialog but there is no split_dialog module in pyface/ui/wx ...
And that imported SplitDialog is used as the base class for the PreferenceDialog class defined in this module.

aaronayres35 · 2021-02-04T17:37:59Z

pyface/ui/qt4/action/tool_bar_manager.py

@@ -136,7 +136,7 @@ def _qt4_add_tools(self, parent, tool_bar, controller):
                # Is a separator required?
                if previous_non_empty_group is not None and group.separator:
                    separator = tool_bar.addSeparator()
-                    group.on_trait_change(
+                    group.observe(


note: I don't think these are getting removed

…ually hooked up before trying to remove them)

aaronayres35 · 2021-02-04T19:00:04Z

pyface/action/listening_action.py

+            if old:
+                old.observe(method, trait(name, optional=True), remove=True)
+            if new:
+                new.observe(method, trait(name, optional=True))


commenting on this to draw attention to a reviewer. Previously the observer was never getting hooked up if name was None. This caused pyface/action/tests/test_listening_action/TestListeningAction/test_destroy to fail because we ultimately tried to unhook it in the destroy call.
The previous commit fixed this issue as well, but this seemed like the more correct way to go about it.

~~i'm not entirely sure under what circumstances name is None will be True here.~~ Ohh. Wait. Never mind. if name will be False if name is an empty string - which is possible.

aaronayres35 · 2021-02-04T19:59:39Z

ref: #876, #877
this PR did not make these fixes on the fly either and so when addressing those issues this PR should be used as a reference to ensure that all cases are addressed.

rahulporuri

Almost LGTM

pyface/action/listening_action.py

rahulporuri · 2021-02-05T06:13:01Z

pyface/action/listening_action.py

+            if self.enabled_name:
+                self.object.observe(
+                    self._enabled_update,
+                    trait(self.enabled_name, optional=True),


i think this line warrants a comment - we already have a conditional so im not sure why we use the optional=True in the call to trait - and i'm not even sure what trait does.

ah i meant to delete the conditionals, they are no longer needed once we have the optional=True

rahulporuri · 2021-02-05T06:13:07Z

pyface/action/listening_action.py

+            if self.visible_name:
+                self.object.observe(
+                    self._visible_update,
+                    trait(self.visible_name, optional=True),


same comment as above.

rahulporuri · 2021-02-05T06:15:07Z

pyface/action/listening_action.py

+            if old:
+                old.observe(method, trait(name, optional=True), remove=True)
+            if new:
+                new.observe(method, trait(name, optional=True))


~~i'm not entirely sure under what circumstances name is None will be True here.~~ Ohh. Wait. Never mind. if name will be False if name is an empty string - which is possible.

pyface/ui/qt4/action/action_item.py

pyface/ui/wx/viewer/table_viewer.py

pyface/ui/wx/tree/tree.py

pyface/ui/wx/grid/grid.py

rahulporuri · 2021-02-05T07:42:42Z

pyface/ui/wx/grid/grid.py

-        # because wx is retarded we have to check this as well -- why
-        # the blazes can't they come up with one Q#$%@$#% API to manage
-        # selections??? makes me want to put the smack on somebody.


why'd you remove this 🤣

Co-authored-by: Poruri Sai Rahul <[email protected]>

rahulporuri

LGTM with a few more suggestions.

pyface/ui/wx/action/menu_manager.py

pyface/action/listening_action.py

Co-authored-by: Poruri Sai Rahul <[email protected]>

rahulporuri · 2021-02-05T13:35:28Z

Still LGTM. Merge once green!

kitchoi · 2021-02-10T18:25:11Z

pyface/action/listening_action.py

-                self._visible_update, self.visible_name, remove=True
+            self.object.observe(
+                self._visible_update,
+                trait(self.visible_name, optional=True),


trait here assumes the first argument is a trait name (see doc), so an extended name like "child.is_visible" is going to fail because "child.is_visible" as a trait name is not defined on object. The optional=True may mask that lost behaviour?

This was missed in the `on_trait_change` to `observe` changes introduced in #880 . The methods were previously receiving `obj` as an argument to the trait change handler which should have been replaced with `event.object` in #880.

aaronayres35 added 11 commits February 3, 2021 15:51

updates for both wx and qt action_item.py

6cb6a5c

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

348d9be

… broken)

this fixes the segfault but could break other things. _manager object…

cb8abb9

… does not even have an image trait so setting up observers for it doesn't make sense

correct fix

11bcb9c

update wx menu_manager.py

267cf0c

update wx table_viewer.py and grid.py

a861827

fix wx grid and table_viewer handler signatures

c497ec0

fix handler signaturess for wx menu_manager

23228ea

update wx trait_grid_model

8694cea

updates for wx grid.py

194c3fc

remove a few more observers in dispose which weren't removed previously

68d9a9c

aaronayres35 commented Feb 4, 2021

View reviewed changes

aaronayres35 added 2 commits February 4, 2021 10:58

remove commented out code

fa38e0b

update wx preference_dialog.py

6316309

aaronayres35 commented Feb 4, 2021

View reviewed changes

aaronayres35 added 4 commits February 4, 2021 11:05

update wx list_box.py

a972587

update wx tool_bar_manager.py

e3434f1

update wx tree.py

8fa8527

update qt tool_bar_manager

021359e

aaronayres35 commented Feb 4, 2021

View reviewed changes

aaronayres35 added 4 commits February 4, 2021 11:41

update toggle_view_visibility_action

0151354

update abstract_command_stack_action.py

5c17aee

update listening_action.py (note had to be careful observers were act…

a9c74b3

…ually hooked up before trying to remove them)

maybe this is the more correct thing to do?

77a6e6c

aaronayres35 commented Feb 4, 2021

View reviewed changes

aaronayres35 added 4 commits February 4, 2021 13:07

update i_field.py

ca4ca3e

update i_combo_field.py

0c77a63

update i_spin_field.py

8a4c0aa

update i_text_field.py

e9b92e1

aaronayres35 changed the title ~~[WIP] Convert dynamic on_trait_change to observe part 2~~ Convert dynamic on_trait_change to observe part 2 Feb 4, 2021

aaronayres35 requested a review from rahulporuri February 4, 2021 20:00

rahulporuri reviewed Feb 5, 2021

View reviewed changes

aaronayres35 and others added 4 commits February 5, 2021 06:44

Apply suggestions from code review

8a663b3

Co-authored-by: Poruri Sai Rahul <[email protected]>

remove unneeded conditionals

4e45c99

update section in docs

e79805b

more suggestions from code review

c581b0b

rahulporuri approved these changes Feb 5, 2021

View reviewed changes

Apply suggestions from code review

c51318d

Co-authored-by: Poruri Sai Rahul <[email protected]>

aaronayres35 merged commit 05eefd4 into master Feb 5, 2021

aaronayres35 deleted the replace-dynamic-otc-observe2 branch February 5, 2021 13:38

kitchoi reviewed Feb 10, 2021

View reviewed changes

aaronayres35 mentioned this pull request Feb 11, 2021

don't use trait() for extended trait names #892

Merged

aaronayres35 mentioned this pull request Feb 18, 2021

Replace use of depends_on in Traits Properties with observe enthought/traitsui#1520

Merged

rahulporuri mentioned this pull request Mar 26, 2021

Handle the events correctly #915

Merged

Convert dynamic on_trait_change to observe part 2 #880

Convert dynamic on_trait_change to observe part 2 #880

Uh oh!

Conversation

aaronayres35 commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 commented Feb 4, 2021

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulporuri commented Feb 5, 2021

Uh oh!

kitchoi Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaronayres35 commented Feb 4, 2021 •

edited

Loading

aaronayres35 Feb 4, 2021 •

edited

Loading

kitchoi Feb 10, 2021 •

edited

Loading