Skip to content

Refactor to centralise binding machinery and make dynamic layouts easier#42

Merged
WillJRoper merged 21 commits intomainfrom
centralise-bindings
Nov 12, 2025
Merged

Refactor to centralise binding machinery and make dynamic layouts easier#42
WillJRoper merged 21 commits intomainfrom
centralise-bindings

Conversation

@WillJRoper
Copy link
Owner

This PR brings all key-binding machinery into a single class to make maintenance easier. This change also introduces the ability to better control dynamic labelling and layout based on application state. While no new functionality is introduced, this future-proofs H5Forest.

WillJRoper and others added 17 commits November 12, 2025 13:12
Fixed critical bugs in the centralized binding system refactor:

Bug Fixes:
- Fixed histogram show binding not displaying plots by adding missing
  use_chunks parameter to HistogramPlotter._plot() signature
- Fixed histogram save binding not bringing up data entry by passing
  use_chunks parameter to plot_and_save() calls
- Both histogram functions now properly match the parent Plotter class
  interface for consistency

Test Updates:
- Updated all binding test files to work with new H5KeyBindings class
- Fixed 145+ test failures by adding proper H5Forest singleton mocking
- Added missing focus state attributes to test fixtures
- Updated test imports from old module paths to new structure
- Removed obsolete test file (test_h5forest_properties.py)
- All 530 unit tests now passing with 95% code coverage

Documentation:
- Added comprehensive developer guide for adding new bindings
- Documented the centralized H5KeyBindings architecture
- Provided step-by-step instructions and examples
- Added section to contributing.md

Code Quality:
- Fixed all pre-commit hook violations (line length, formatting)
- Ensured all code follows PEP 8 style guidelines
- Maintained 100% test pass rate
Added extensive test coverage to reach 100% code coverage (624 tests):

New Test Classes and Methods:
- TestGetCurrentHotkeys (12 tests): Tests hotkey display for all modes
  - Normal mode, expanded/collapsed attributes
  - Dataset, search, window, jump modes
  - Histogram mode (tree/config focused, with/without data)
  - Plot mode (tree/plot focused, with/without data)

- TestGetModeTitle (8 tests): Tests mode title display
  - All 7 application modes (normal, goto, dataset, window, plot, hist, search)
  - Edge case for unknown mode state

- TestH5ForestProperties (7 tests): Tests H5Forest property methods
  - flag_in_prompt, dataset_values_has_content
  - histogram_config_has_focus, plot_config_has_focus
  - current_column with multiline documents
  - current_position property
  - refresh() callback method

- TestDataAssignedProperties (5 tests): Tests data_assigned properties
  - ScatterPlotter with no data, x only, y only, both x and y
  - HistogramPlotter with and without data

- TestChunkedHistogramComputation (2 tests): Tests chunked data processing
  - Chunked histogram computation with linear scale
  - Chunked histogram computation with log scale

- TestDynamicLabelLayoutPadding (3 tests): Tests label padding logic
  - Row padding with uneven label distribution
  - Edge cases forcing while loop execution

Coverage Improvements:
- bindings.py: 82% → 100% (get_current_hotkeys, get_mode_title)
- h5_forest.py: 98% → 100% (properties and refresh method)
- plotting.py: 96% → 100% (chunked computation, data_assigned)
- utils.py: 99% → 100% (padding while loop)

All tests pass with 100% coverage (2,506/2,506 statements covered)
All pre-commit hooks pass (ruff lint + format)
@WillJRoper WillJRoper added the Hotkeys pertaining to hot key interactions label Nov 12, 2025
@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e254889) to head (b0f21d3).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main       #42      +/-   ##
===========================================
+ Coverage   99.57%   100.00%   +0.42%     
===========================================
  Files          21        23       +2     
  Lines        2350      2493     +143     
===========================================
+ Hits         2340      2493     +153     
+ Misses         10         0      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

WillJRoper and others added 4 commits November 12, 2025 20:27
Extended GitHub Actions workflow to test on all major platforms:

Workflow Updates (.github/workflows/test.yml):
- Added matrix strategy for OS: ubuntu-latest, windows-latest, macos-latest
- Set fail-fast: false to run all combinations even if one fails
- Added platform-specific dependency installation:
  * Ubuntu: apt-get install libhdf5-dev pkg-config
  * macOS: brew install hdf5 pkg-config
  * Windows: Uses h5py wheels (no system deps needed)
- Now runs 15 test configurations (3 OS × 5 Python versions)

Documentation Updates:
- Updated contributing.md to reflect Python 3.13 support
- Added cross-platform testing to CI feature list
- Updated from "Python 3.9-3.12" to "Python 3.9-3.13"

This ensures h5forest works correctly on all major platforms and
catches platform-specific issues early in development.
Fixed two platform-specific test issues discovered by CI:

Windows Path Handling (test_tree.py):
- Issue: Test failed on Windows with path separator mismatch
  Expected: 'simple'
  Got: 'D:\a\h5forest\h5forest\tests\fixtures\simple'
- Fix: Changed from manual string splitting with "/" to using
  os.path.basename() for cross-platform path handling
- Added import os to test_tree.py

macOS Threading Race Condition (test_lazy_search.py):
- Issue: Test failed on macOS with Python >3.12 due to fast thread
  completion. The index_building flag was set to False before the
  assertion could check it.
- Fix: Removed the race-prone assertion that checked index_building
  status immediately after thread start. The test now verifies:
  * Thread was created and started
  * Thread is a daemon thread
  * After joining, indexing completed successfully
- This is more robust and handles both fast and slow systems

All 624 tests now pass with 100% coverage maintained.
Tests should now pass on all platforms (Ubuntu, Windows, macOS).
@WillJRoper WillJRoper merged commit c1eef12 into main Nov 12, 2025
33 of 34 checks passed
@WillJRoper WillJRoper deleted the centralise-bindings branch November 12, 2025 20:46
WillJRoper added a commit that referenced this pull request Feb 9, 2026
Refactor to centralise binding machinery and make dynamic layouts easier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Hotkeys pertaining to hot key interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant