Convert on_trait_change decorators to observe decorators #864

aaronayres35 · 2021-01-22T21:55:40Z

part of #732
also part of #637

This PR incrementally (effectively 1 file per commit) replaces all uses of the @on_trait_change decorator with the equivalent form of @observe.

Additionally, it removes any explicit use of setting comparison_mode as that is no longer needed with traits 6.2

Since traits 6.2 eggs are not on edm yet, I suspect CI will fail(? or at least give the comparison mode warnings), but very soon that will no longer occur. Looks like we also still have CI running with traits6.0 so it does fail
Looks like travis is also having failures that seem to stem from changes made to i_data_view_widget. Those changes were mostly to do with comparison_mode and I did not see them locally, so I'm suspecting they will be resolved with traits 6.2 (🤞 )

…nd tasks_application.py because it calls a super on one of the handlers so they needed to be updated togeter. Also specify comparaison mode to avoid warning (is this correct?)

…eeded with traits 6.2

…d, be careful)

…e changes may be untested)

…alled independently of change handling so I pass None as the event)

…onding test

aaronayres35 · 2021-01-22T21:57:06Z

pyface/ui/qt4/tasks/dock_pane.py

-        self._set_dock_title()
-        self._set_floating()
-        self._set_visible()
+        self._set_dock_features(None)


When calling the change handlers independently (without a change actually occurring) they now need to take an event argument. If it is unused, as it is for these handlers I simply passed in None.

Sorry for snooping... don't know if you have considered defining _set_dock_features like this?

def _set_dock_features(self, _=None): ...

Then the signature of the function is more explicit about that fact that the argument is not used.

That makes more sense, thanks!

aaronayres35 · 2021-01-22T21:59:03Z

pyface/ui/qt4/workbench/tests/test_workbench_window_layout.py

        # This should not throw
-        layout._qt4_active_editor_changed(None, None)
+        layout._qt4_active_editor_changed(DummyEvent(new=None))


similar to https://github.com/enthought/pyface/pull/864/files#r562936818 only here the handler was called with arguments. the handler must take a single event argument, and in the previous case we needed the new argument for the old signature to be None. So here I simply created a dummy class to serve the same purpose. This is a bit of a smell though

aaronayres35 · 2021-01-22T22:04:38Z

pyface/workbench/workbench_window.py

@@ -849,16 +849,15 @@ def _views_items_changed(self, event):

    # Dynamic ----

-    @on_trait_change("layout.editor_closed")
-    def _on_editor_closed(self, editor):
+    @observe("layout:editor_closed")


I believe the . from before was a bug and should have been a :. It seems from the handler itself that it was assumed editor was in fact an IEditor instance as we ultimately call self.editors.index(editor). Without this change I was not seeming test failuires, but exceptions were being raised in test_workbench_window.py saying a MagicMock instance was not in self.editors.

This made me learn a new behavior of otc. Previously, when we set workbench_window.layout = mock.MagicMock(spec=WorkbenchWindowLayout) this handler was invoked. However, by modifying the signature I saw that name was actually editor_closed not layout even though layout was changing. This caused the editor argument to come up as Undefined when running the test thus avoiding the self.editors.index(editor) line. When we make the switch, event.name is now layout not editor_changed and so event.new != Undefined as one might have suspected based on what happened before.

I'm not sure if this is explicitly documented in the old otc docs? Or if this is a bug? In any case I believe the new code is correct

aaronayres35 · 2021-01-22T22:09:48Z

pyface/action/action_item.py

        """ Handle the destruction of the wrapper. """
-        if name == "control" and new is None:
-            self._wrappers.remove(object)
+        if event.new is None:


there are a decent number of cases like this, where previously we used . not : but then only do something when the second trait is the one changed?

With these types of changes the tests were all still passing and I believe behavior is preserved (code is cleaner IMO as well), but if we want to play it safe or if a reviewer sees why these might not be equivalent, I am happy to adjust

rahulporuri

Marking this a request changes because there was one bug (missing event argument to method decorated with observe).

Apart from that, this PR LGTM. But, I'd prefer it if we can test these changes against some other project e.g. traitsui or an internal project to see if any errors are raised.

examples/application/python_editor/python_editor_task.py

examples/tasks/advanced/example_task.py

examples/tasks/basic/example_task.py

pyface/ui/qt4/tasks/editor_area_pane.py

pyface/ui/qt4/tasks/split_editor_area_pane.py

pyface/ui/qt4/workbench/workbench_window_layout.py

pyface/ui/wx/tasks/dock_pane.py

pyface/ui/wx/tasks/editor_area_pane.py

rahulporuri · 2021-01-25T05:29:08Z

I ran the testsuite locally (windows) with the pyqt5 toolkit and i did not see any test failures.

rahulporuri · 2021-01-25T07:14:03Z

Also, just to confirm, you'll be opening a separate PR to address the other uses of on_trait_change e.g. object.on_trait_change(...). Is that correct? I'm happy if that's the plan - given how big this PR already is.

aaronayres35 · 2021-01-25T14:15:12Z

Also, just to confirm, you'll be opening a separate PR to address the other uses of on_trait_change e.g. object.on_trait_change(...). Is that correct? I'm happy if that's the plan - given how big this PR already is.

yep that was the plan, and possibly first opening a separate PR to address the first bullet of #732.

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

…m/enthought/pyface into convert-otc-decorators-to-observe

rahulporuri · 2021-01-27T06:25:38Z

I restarted CI but Travis CI isn't completely happy. Looks like #865 needs to be merged first - and then this PR needs to be updated - to make CI happy.

aaronayres35 · 2021-01-27T13:11:09Z

CI is now passing with #865 merged. Going to checkout this branch and run traitsui test suite now

EDIT: Done, I did not see any new errors from this branch compared to master (I ran testsuite with pyqt5 and wx)

rahulporuri

LGTM.

I apologize for the large number of suggestions at the last minute. Almost all of them are _=None changes - it would be best to ensure that we are using this convention wherever it is relevant.

Please batch all of the suggestions into a single commit to prevent spamming this PR with 27 commits - one per suggestion made in this review :D

pyface/gui_application.py

pyface/tests/test_gui_application.py

pyface/ui/qt4/tasks/advanced_editor_area_pane.py

rahulporuri · 2021-01-28T11:11:02Z

@kitchoi @aaronayres35 _=None is not standard python and we have been recommended against using it - we could use event=None instead, if not just event.

Update : On further thought, let's just leave it as event instead of using event=None either. There is no need to complicate this further.

rahulporuri · 2021-01-28T11:48:43Z

@mdickinson ^

aaronayres35 · 2021-01-28T15:56:56Z

@kitchoi @aaronayres35 _=None is not standard python and we have been recommended against using it - we could use event=None instead, if not just event.

Update : On further thought, let's just leave it as event instead of using event=None either. There is no need to complicate this further.

There are some occasions where the methods are called explicitly, independently of any listening to changes. I take that the suggestion then (at least for now / on this PR) is to when we call those methods explicitly pass None as the event there rather than having it as a default? Long term we will probably want to transition to Robert's solution of redefining the methods that will be called separately as normal methods and then calling those within the listening handlers

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

rahulporuri

LGTM with two necessary code changes (undiscovered bugs) and a lot of style changes (from _=None to event).

pyface/ui/qt4/tasks/dock_pane.py

pyface/ui/qt4/tasks/editor_area_pane.py

pyface/ui/wx/tasks/editor_area_pane.py

pyface/workbench/action/view_menu_manager.py

pyface/workbench/debug/debug_view.py

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

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

I've opened #876 for dealing with the refactor/cleanup of cases where we call handlers standalone and pass event=None. I think I will make those fixes in a separate PR.

rahulporuri · 2021-02-01T16:41:39Z

CI is green and still LGTM

aaronayres35 added 24 commits January 22, 2021 11:47

update otc decorators to be observe decorators in guiapplication.py a…

28f954b

…nd tasks_application.py because it calls a super on one of the handlers so they needed to be updated togeter. Also specify comparaison mode to avoid warning (is this correct?)

use observe decorator in palce of otc decorator in action/action_item.py

8d69ddc

same for tasks/task_window.py

0fdd93d

fix update to task window (needed a : not a .)

c086a34

remove specification of identity comparison_mode as it is no longer n…

9089d33

…eeded with traits 6.2

decorator updates for tasks/action/dock_pane_toggle_group.py

cd3f943

update decorators in tasks/action/task_toggle_group.py (seems unteste…

32d27b6

…d, be careful)

same for task_window_toggle_group.py (also seems unttested? be careful)

479fc21

replace decorator in test_gui_application.py

a43f506

replace decorators in application_window.py

2f8f535

update decorrators for ui/qt4/tasks/advanced_editor_area_pane.py (not…

60ed83c

…e changes may be untested)

update decorators for ui/qt4/tasks/dock_pane.py (note handlers were c…

2bf7b38

…alled independently of change handling so I pass None as the event)

update decorators for both toolkits editor_area_pane.py files

84a401e

same for ui/qt4/tasks/split_editor_area_pane.py

00fda10

more of the same this time for wx dock_pane.py

5ac9cef

update decorators in qt workbench_window_layout.py and update corresp…

56ecaf3

…onding test

update wx application_window.py

3c7a809

update workbench_window.py

7f0b8ec

update decorators for workbench/action/perspective_menu_manager.py

664b17a

update decorators for workbench/action/set_active_perspective_action.py

590d911

update for workbench/action/user_perspective_action.py

e6d91c4

update for workbench/action/view_menu_manager.py

df03601

update for workbench/debug/debug_view.py

604368e

update to use observe decorators in examples

7b967f0

aaronayres35 commented Jan 22, 2021

View reviewed changes

aaronayres35 mentioned this pull request Jan 22, 2021

Replace on_trait_change with observe #732

Closed

6 tasks

rahulporuri suggested changes Jan 25, 2021

View reviewed changes

aaronayres35 and others added 4 commits January 25, 2021 08:25

if event isn't actually needed, define the argument as _=None instead

a8e1066

Apply suggestions from code review

ba0722a

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

Merge branch 'convert-otc-decorators-to-observe' of https://github.co…

b42fd79

…m/enthought/pyface into convert-otc-decorators-to-observe

more suggested changes for code readability

b9cd5c2

aaronayres35 closed this Jan 27, 2021

aaronayres35 reopened this Jan 27, 2021

rahulporuri self-requested a review January 27, 2021 13:47

rahulporuri approved these changes Jan 28, 2021

View reviewed changes

Apply suggestions from code review

8f9c2b3

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

rahulporuri approved these changes Feb 1, 2021

View reviewed changes

aaronayres35 and others added 3 commits February 1, 2021 08:54

Apply suggestions from code review

729be50

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

pass event=None where needed

e7c6ea4

missed a few

083a265

aaronayres35 mentioned this pull request Feb 1, 2021

Refactor change handlers which are called standalone #876

Open

mdickinson changed the title ~~Convert otc decorators to observe decorators~~ Convert on_trait_change decorators to observe decorators Feb 1, 2021

aaronayres35 merged commit a38976c into master Feb 1, 2021

aaronayres35 deleted the convert-otc-decorators-to-observe branch February 1, 2021 16:46

aaronayres35 mentioned this pull request Feb 1, 2021

Refactor listeners which are called standalone #878

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert on_trait_change decorators to observe decorators #864

Convert on_trait_change decorators to observe decorators #864

aaronayres35 commented Jan 22, 2021 •

edited

Loading

aaronayres35 Jan 22, 2021

kitchoi Jan 25, 2021

aaronayres35 Jan 25, 2021

aaronayres35 Jan 22, 2021

aaronayres35 Jan 22, 2021

aaronayres35 Jan 22, 2021

rahulporuri left a comment

rahulporuri commented Jan 25, 2021

rahulporuri commented Jan 25, 2021

aaronayres35 commented Jan 25, 2021

rahulporuri commented Jan 27, 2021 •

edited

Loading

aaronayres35 commented Jan 27, 2021 •

edited

Loading

rahulporuri left a comment

rahulporuri commented Jan 28, 2021 •

edited

Loading

rahulporuri commented Jan 28, 2021

aaronayres35 commented Jan 28, 2021

rahulporuri left a comment

aaronayres35 commented Feb 1, 2021

rahulporuri commented Feb 1, 2021

Convert on_trait_change decorators to observe decorators #864

Convert on_trait_change decorators to observe decorators #864

Conversation

aaronayres35 commented Jan 22, 2021 • edited Loading

aaronayres35 Jan 22, 2021

Choose a reason for hiding this comment

kitchoi Jan 25, 2021

Choose a reason for hiding this comment

aaronayres35 Jan 25, 2021

Choose a reason for hiding this comment

aaronayres35 Jan 22, 2021

Choose a reason for hiding this comment

aaronayres35 Jan 22, 2021

Choose a reason for hiding this comment

aaronayres35 Jan 22, 2021

Choose a reason for hiding this comment

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri commented Jan 25, 2021

rahulporuri commented Jan 25, 2021

aaronayres35 commented Jan 25, 2021

rahulporuri commented Jan 27, 2021 • edited Loading

aaronayres35 commented Jan 27, 2021 • edited Loading

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri commented Jan 28, 2021 • edited Loading

rahulporuri commented Jan 28, 2021

aaronayres35 commented Jan 28, 2021

rahulporuri left a comment

Choose a reason for hiding this comment

aaronayres35 commented Feb 1, 2021

rahulporuri commented Feb 1, 2021

aaronayres35 commented Jan 22, 2021 •

edited

Loading

rahulporuri commented Jan 27, 2021 •

edited

Loading

aaronayres35 commented Jan 27, 2021 •

edited

Loading

rahulporuri commented Jan 28, 2021 •

edited

Loading