Audit fixes and uplift: repair dead controls, decouple from wx, add tests and CI - #1
Merged
Conversation
The repository had neither, so build artefacts and virtualenvs were untracked-but-noisy and the licensing terms were unstated.
Common.py acted as a shared 'import everything' bucket that every module pulled in with 'from Common import *', which made it impossible to tell where any given name came from. Imports are now explicit. The Matplotlib editor was written against wx only: it imported wx at module scope, subclassed traitsui.wx.editor.Editor and scheduled repaints with wx.CallAfter. It now resolves the active ETS toolkit at import time and builds either a Qt or a wx canvas, and repaints go through the neutral GUI.invoke_later. That makes Qt a viable backend, which matters because wxPython is by far the harder of the two to install. Also switches to matplotlib.figure.Figure so importing the app no longer drags in pyplot and its global backend state. Behaviour is otherwise unchanged; the bugs found during the audit are fixed in the commits that follow.
There was no dependency metadata of any kind, so the only install instructions were prose in the README. The GUI toolkit is exposed as mutually exclusive 'qt'/'wx' extras rather than a hard dependency, and Mayavi sits behind a '3d' extra because VTK is the heaviest and most install-fragile piece of the stack.
* markercolor was 'ColorTrait' without parentheses, so it bound as a
method rather than a trait and the colour picker was inert. Now
ColorTrait("blue").
* yintcept/gradient were Range(0.0, 5.0, 10.0) -- Range takes
(low, high, default), so the default sat outside its own bounds. The
traits initialised to an invalid 10.0 and assigning any in-range value
raised TraitError. Bounds widened to (0.0, 10.0) with a 5.0 default.
* listoptions listed 'Option 2' twice, so the third entry was unreachable.
* Both panel constructors accepted **kwargs and then called
HasTraits.__init__(self) without them, silently discarding any trait
values passed by the caller.
ApplicationMain.__init__ assigned the marker defaults but never chained to
HasTraits.__init__. Declaring the defaults on the traits themselves makes
the override unnecessary, so it is removed rather than repaired. 'main' is
now a declared trait on both panels instead of an undeclared attribute.
All three handlers were guarded by 'hasattr(self, "display_points")', but nothing in the codebase ever assigned display_points. The guard was therefore always false and the entire marker control group was inert. Plot state now lives on ApplicationMain: panels call main.plot(x, y), which records the Line2D it created, and apply_marker_style() pushes the three traits onto it. The handlers delegate to that. Fixing the guard alone would not have been enough. ColorTrait yields a toolkit-native QColor/wx.Colour and Matplotlib raises ValueError on both, so the old 'color=self.main.markercolor' plotting calls in the panel could not have worked either. pyguitemplate.colors.to_mpl_color() converts them. Also collapses _gradient_changed and _yintcept_changed, which were byte-identical, into replot(), and gives plotbutton the _plotbutton_fired handler it was missing -- pressing it did nothing.
app.py imported mayavi and tvtk at module scope, so a missing VTK made the entire application unimportable -- not just the 3D tab. VTK is the largest and most fragile dependency here and plenty of users of this template never touch the 3D view. The scene tab moves to its own module that probes for Mayavi once. When it is absent the tab becomes a placeholder carrying install instructions, and both variants expose the same plot_demo() so callers never branch on availability. When it is present the tab now renders a demo surface rather than opening blank.
SecondPanel declared 'view = View()' and nothing else, so the second tab rendered blank -- a template tab that demonstrated nothing. It is now a small signal generator: pick a waveform, set amplitude, frequency, noise and sample count, and it derives the series and pushes it to the shared display. That covers ground the first panel does not -- combining several traits into a derived result, @observe on a group of traits, enabled_when driven by another control, and writing to a user-chosen file with the failure path handled.
The project had no tests at all. 44 tests covering the display, both panels, the colour conversion and the optional scene tab, with a named regression test for each bug this branch fixes. Nothing opens a window -- the tests build the trait objects and inspect the Matplotlib figure -- so they run on CI with no display. Verified by reintroducing the original bugs, which fails 6 of them.
Runs on Python 3.9, 3.11 and 3.12 against the Qt extra, using the offscreen platform plugin so no display server is needed. Also applies ruff's formatting to the existing sources so the format check starts green, and excludes Markdown from ruff so README snippets are left as written.
The install instructions were the main problem: they led with Enthought's EPD distribution, which has been discontinued for years and was described as needing a 32-bit build and an academic licence. The rest of the page documented editing Common.py by hand to switch import styles -- a file that no longer exists. Replaced with real installation via the toolkit extras, a project layout map, a worked 'add your own panel' example, a troubleshooting table and testing instructions. The example panel is executed as part of preparing this change, so it is known to run. Screenshots are kept but labelled as predating the current version.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of the template followed by an uplift. Ten atomic commits, each independently reviewable.
Bugs found
All of these were verified against a live Traits/TraitsUI install, and each now has a named regression test.
The marker colour/style/size controls never worked. All three handlers were guarded by
hasattr(self, "display_points"), but nothing in the codebase ever assigneddisplay_points. The guard was permanently false, so the entire control group was inert.Fixing the guard alone would not have been enough.
ColorTraitreturns a toolkit-nativeQColor/wx.Colour, and Matplotlib raisesValueErroron both — so thecolor=self.main.markercolorplotting calls in the panel could not have worked either. Addedto_mpl_color()to convert.markercolor = ColorTraitwas missing its parentheses, so it bound as a method rather than a trait. The colour picker had nothing behind it.yintcept/gradientwereRange(0.0, 5.0, 10.0).Rangetakes(low, high, default), so both initialised to10.0— outside their own bounds — and assigning any in-range value raisedTraitError.listoptionslisted"Option 2"twice, making the third entry unreachable.plotbuttonhad no handler. Pressing "Plot Me!" did nothing.Both panel constructors accepted
**kwargsthen calledHasTraits.__init__(self)without them, silently discarding caller-supplied trait values.ApplicationMain.__init__never chained toHasTraits.__init__at all._rangeplotx_changedwas dead and broken — a handler for arangeplotxtrait that does not exist, whose body referenced four undefined names and contained a bare expression statement (y). Removed in the restructure commit, since it could never execute.SecondCalcwasview = View()— the second tab rendered blank.Other changes
Removed the hard wx dependency. The Matplotlib editor imported
wxat module scope, subclassedtraitsui.wx.editor.Editorand usedwx.CallAfter. It now resolves the active ETS toolkit and builds either a Qt or wx canvas. This matters because wxPython is much harder to install than Qt.Mayavi is optional.
app.pyimportedmayavi/tvtkat module scope, so a missing VTK made the whole application unimportable — not just the 3D tab. The scene tab now degrades to an install hint.Structure.
Common.pywas a shared "import everything" bucket consumed viafrom Common import *, making name origins untraceable. Code moved into apyguitemplatepackage with explicit imports.python main.pystill works.Second panel is now a signal generator: waveform, amplitude, frequency, noise, sample count, plus CSV export with the failure path handled.
Added
pyproject.toml(with mutually exclusiveqt/wxextras and an optional3d),.gitignore, MITLICENSE, 44 headless tests, and GitHub Actions running ruff and pytest on 3.9/3.11/3.12.README led with Enthought's EPD distribution — discontinued years ago, described as needing a 32-bit build and an academic licence — and told users to hand-edit
Common.py, which no longer exists. Rewritten with real install instructions, a layout map, a worked "add your own panel" example, and a troubleshooting table.Verification
ruff checkandruff format --checkclean.Tested against PySide6 6.11 / TraitsUI 8 / Matplotlib 3.11 on Python 3.12. The wx path is written but not executed — wxPython is not installed here, so it is verified by inspection only. The CI matrix on this PR is the first real run of the workflow.