enthought / enthought/traitsui

Awkward editor disposal when editors are defined by subclassing another

Open
#884 1 comment 0 reactions 0 assignees View on GitHub
type: refactor
Dominant language
Python
Stars
306
Forks
99
PR merge metrics
No merged PRs in 30d

Description

This pattern is seen in a number of editors: a concrete editor is defined by subclassing another concrete editor. By concrete editors, I mean a subclass of `traitsui.editor.Editor` that is intended to be instantiated directly to be used in `traitsui.ui.UI._editors`.

For example, there is a concrete editor `traitsui.qt4.text_editor.SimpleEditor`, which is subclassed by another concrete editor `traitsui.qt4.text_editor.CustomEditor`.

This makes implementing the `dispose` method awkward when trying to clean up widgets upon UI disposal.

e.g.
`traitsui.qt4.button_editor.CustomEditor` subclasses `traitsui.qt4.button_editor.SimpleEditor` in order to reuse the `update_object` and `update_editor` defined there.

`SimpleEditor` has `init` and `dispose` methods. However `CustomEditor` only overrides `SimpleEditor`'s `init`, and does not implement a `dispose`.

The `SimpleEditor.dispose` is in fact incorrect: It needs to remove the on_trait_change handlers put up in `SimpleEditor.init`. However, such removal is NOT needed by `CustomEditor`. Consequently, one may need to override `dispose` in `CustomEditor`, but then it will also need to call `traitsui.editor.Editor.dispose`. Simply calling `super().dispose()` will end up calling `SimpleEditor.dispose` which will try to remove change handlers that don't exist.

Proposed solutions (they are not mutually exclusive):
- Avoid subclassing editors that are intended to be a leaf class: Refactor the common logic to a base class that is subclassed by the concrete editors.
- Move the logic in `traitsui.editor.Editor.dispose` out to another maybe-private method so that it is always run without relying on subclass to always call it. Its docstring says it is intended to be overridden to provide *additional clean-up*. If the base class implementations always needs to be run, it might make sense for the base class to ensure that happens.
- More tools for disposal: Maybe we need a method for adding cleanup steps in a stack, similar to `unittest.TestCase.addCleanup`. `unittest.TestCase.setUp` will be analogous to `traitsui.editor.Editor.init`. `unittest.TestCase.tearDown` is analogous to `traitsui.editor.Editor.dispose`.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.