Disconnect trait change handlers from downstream ApplicationWindow widgets

[ievacerny]

Part of #258

Before destroying control of ApplicationWindow, trait change handlers from downstream widgets are disconnected.

The structure of these underlying widgets and their managers is fairly complex so it's hard to say if it is necessary to disconnect these trait change handlers and if it helps with garbage collection. I attempt to evaluate the risks and the benefits but there might be things that I've missed.

---

_Menu in menu_manager.py and _ToolBar in tool_bar_manager.py

They connect their methods to Python MenuManager and ToolBarManager object traits, but as far as I know that shouldn't prevent any of the objects from being garbage collected. And when either side is deleted, the connection should break. However, if for some reason those connections remain after an attempt to destroy the whole control hierarchy, they might cause problems because there are no safeguards in the handlers.

This PR removes on_trait_change connections, however, to do so it has to find these objects in the children of the control, since no proper reference is held. The code also only checks that the objects is an instance of expected Qt object, not the custom _Menu or _ToolBar because those are private classes. This is risky because there might be other QMenu and QToolBar objects that do not have expected disconnect_event_listeners method. One solution would be to wrap these in a try-except.

I'm open to suggestion on how to better approach this situation and if the potential benefits are worth the risk.

---

_MenuItem and _Tool in action_item.py

All handlers in action_item.py have protective if self.control is not None check so from code inspection it seems that leaving these handlers here shouldn't cause problems. But a disconnect structure was already implemented for _MenuItem in 8085548, which suggests that not removing the handlers was actually causing problems. Although it was only executed on refresh, not on destruction.

This PR connects that structure from _MenuItem to the main destroy method. The PR doesn't add a similar structure for _Tool because (1) its clear method behaves differently, (2) it would introduce more changes outside of disposal code. Instead trait change handlers of _Tool are disconnected in a slot that is connected to the destroyed signal of its control. I would say that the code added here is low risk because it disconnects on_trait_change from Python objects. And if the code from the section above doesn't work out, the disconnect in _MenuItem can also happen in a slot connected to destroyed signal.

There is a qt connection to _Tool.control.triggered signal that is not currently disconnected on destruction because there's no good place to do so without adding a lot of extra code. The slot does work with UI components and doesn't have a control is None check so it has some potential of causing problems. But I think a better (lower risk) approach would be to add this check to the slot rather than adding additional structure in order to disconnect it.

Note1: There are also 2 qt signal-slot connections that are connected to destroyed signal. This PR doesn't disconnect them because they shouldn't be disconnected.

Note2: _PaletteTool in action_item.py hasn't been touched because the support for it has not been implemented in qt.

[ievacerny]

This is risky because there might be other QMenu and QToolBar objects that do not have expected disconnect_event_listeners method. One solution would be to wrap these in a try-except.

And tests are failing on windows because of this... Added try-except but it still doesn't feel like a very good solution.

[codecov-commenter]

Codecov Report

Merging #541 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   36.99%   37.12%   +0.13%     
==========================================
  Files         470      470              
  Lines       26027    26066      +39     
  Branches     3961     3964       +3     
==========================================
+ Hits         9629     9678      +49     
+ Misses      15977    15968       -9     
+ Partials      421      420       -1     

Impacted Files	Coverage Δ
pyface/ui/qt4/action/action_item.py	46.46% <100.00%> (+5.15%)	⬆️
pyface/ui/qt4/action/menu_manager.py	70.29% <100.00%> (+7.50%)	⬆️
pyface/ui/qt4/action/tool_bar_manager.py	84.28% <100.00%> (+3.92%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ba6c2...815a400. Read the comment docs.

[kitchoi]

Thank you for the assessment! Some ideas...

[ievacerny]

Thank you for the review @kitchoi

Following the suggestions I changed the approach:

---

_Menu in menu_manager.py and _ToolBar in tool_bar_manager.py

MenuManager and ToolBarManager now keep a list of created _Menu and _ToolBar objects and use that list to call their _remove_event_listeners methods (name changed).

One concern I have with this is context and temporary menus (e.g. in fields.py) might stick around because of those references, but I've done some testing and I haven't seen any changes in behaviour. I would still labels this as a high risk change as I'm not convinced that it's not going to introduce bugs.

---

_MenuItem and _Tool in action_item.py

I've implemented a structure similar to the one in _MenuItem with _Tool but without going through clear method. Again, I've done some testing and everything seems to work fine but I'm not convinced that keeping these references won't cause issues, so I'm labelling this as high risk as well.

[kitchoi]

Thank you very much! This looks pretty reasonable to me. Just some naming nits.

[kitchoi]

LGTM