Skip to content

Conversation

@jsfillman
Copy link
Owner

@jsfillman jsfillman commented May 27, 2025

PR Type

Enhancement, Tests, Documentation


Description

  • Added dual 3-tap delay effect to DSP and UI:

    • 18 new OSC-controllable delay parameters (L/R, 3 taps each, feedback, mix, enable)
    • Integrated delay controls into PyQt6 UI with MIDI mapping and tabbed layout
    • Updated DSP to process stereo delays post-filter, pre-gain
  • Improved and expanded automated test coverage:

    • Added headless PyQt6 test support and basic import tests
    • Enhanced DSP path and error handling tests
    • Improved test structure and coverage for synth, UI, and OSC utilities
  • Added CI enhancements and documentation:

    • New GitHub Actions for coverage badge and Xvfb-based GUI testing
    • Added implementation doc for dual 3-tap delay
    • Updated README for new features and coverage

Changes walkthrough 📝

Relevant files
Enhancement
2 files
legato_synth.dsp
Add dual 3-tap stereo delay with OSC control to DSP           
+68/-1   
main_window.py
Add delay controls (UI, MIDI, OSC) and tabbed effects section
+514/-34
Tests
6 files
conftest.py
Add PyQt6 headless test setup and skip logic                         
+58/-0   
test_basic_imports.py
Add basic import and structure tests for all modules         
+64/-0   
test_dsp_init.py
Improve DSP path tests and error handling coverage             
+12/-13 
test_melody.py
Refactor for improved import and patching, cleanup             
+1/-14   
test_osc_client.py
Refactor import for linting and coverage                                 
+1/-1     
test_ramp_test.py
Refactor for improved import and patching, cleanup             
+1/-8     
Configuration changes
4 files
coverage-badge.yml
Add workflow for dynamic coverage badge generation             
+75/-0   
tests.yml
Enhance CI: add Xvfb, run all tests, GUI support                 
+9/-2     
pytest.ini
Remove coverage fail threshold, minor cleanup                       
+0/-1     
.flake8
Add Flake8 config for linting and formatting                         
+4/-0     
Documentation
2 files
dual-3tap-delay-implementation.md
Add implementation document for dual 3-tap delay                 
+242/-0 
README.md
Update for delay, coverage, and feature documentation       
+66/-73 
Formatting
7 files
melody.py
Add linting pragma for import order                                           
+1/-1     
murnau_ui.py
Add linting pragma for import order                                           
+3/-3     
ramp_test.py
Add linting pragma for import order                                           
+1/-1     
run_murnau.py
Add linting pragma for import order                                           
+3/-3     
setup.py
Minor formatting and whitespace cleanup                                   
+3/-1     
ramp_test.py
Improve print formatting for ramp test                                     
+3/-1     
widgets.py
Minor import and paint logic cleanup                                         
+2/-4     
Error handling
1 files
__init__.py
Add error handling for None DSP file name                               
+3/-0     
Additional files
3 files
.DS_Store [link]   
.DS_Store [link]   
.DS_Store [link]   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The send_osc method now has error handling, but the specific exception type isn't caught. Consider using more specific exception types (like ConnectionError) rather than catching all exceptions to avoid masking unexpected errors.

    try:
        # Add synth name to the address path
        full_address = f"/{self.synth_name}{address}"
        self.osc_client.send_message(full_address, float(value))
    except Exception as e:
        # Silently handle OSC connection errors to prevent UI crashes
        print(f"OSC send error: {e}")
    Feedback Implementation

    The delay implementation is missing proper feedback loops. The documentation mentions feedback but the actual DSP code only implements basic taps without feedback paths, which could limit the delay effect's capabilities.

    delay_L_process(input) = output_L
    with {
        // Three taps using @ operator for delays
        tap1_L = input : @(ms_to_samples(delay_L_tap1_time)) : *(delay_L_tap1_level);
        tap2_L = input : @(ms_to_samples(delay_L_tap2_time)) : *(delay_L_tap2_level);
        tap3_L = input : @(ms_to_samples(delay_L_tap3_time)) : *(delay_L_tap3_level);
    
        // Sum taps
        taps_sum_L = tap1_L + tap2_L + tap3_L;
    
        // Dry/wet mix
        output_L = select2(delay_L_enable,
            input,  // Bypass when disabled
            input * (1 - delay_L_mix) + taps_sum_L * delay_L_mix
        );
    };
    Conditional UI Focus

    The piano focus code now checks if the piano attribute exists, but it might be better to have a more structured approach to handle optional UI components throughout the codebase.

    # Set focus to piano keys (only if piano widget exists)
    if hasattr(self, "piano") and self.piano:
        self.piano.setFocus()

    @qodo-code-review
    Copy link

    qodo-code-review bot commented May 27, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve error handling

    Catching a broad Exception silently can hide critical bugs. While error handling
    is good, you should log the error properly rather than just printing it,
    especially since this is a critical communication function.

    src/murnau/ui/main_window.py [1233-1239]

     try:
         # Add synth name to the address path
         full_address = f"/{self.synth_name}{address}"
         self.osc_client.send_message(full_address, float(value))
     except Exception as e:
    -    # Silently handle OSC connection errors to prevent UI crashes
    -    print(f"OSC send error: {e}")
    +    # Log OSC connection errors to help with debugging
    +    import logging
    +    logging.error(f"OSC send error: {e}")
    +    # Optionally update UI to show connection status
    • Apply / Chat
    Suggestion importance[1-10]: 4

    __

    Why: Valid suggestion to use proper logging instead of print() for error handling. However, the improvement is minor since this is for debugging OSC communication errors, not critical application logic.

    Low
    Fix string formatting

    The f-string is incorrectly split into two parts with concatenation. This works
    but is inefficient and harder to maintain. Combine them into a single f-string
    for better readability and performance.

    src/murnau/synth/ramp_test.py [40-42]

     print(
    -    f"\nTesting ramp from {start_freq}Hz to {end_freq}Hz " f"over {ramp_time}s"
    +    f"\nTesting ramp from {start_freq}Hz to {end_freq}Hz over {ramp_time}s"
     )
    • Apply / Chat
    Suggestion importance[1-10]: 3

    __

    Why: Correct observation that the f-string concatenation is inefficient and less readable. The change would improve code style but has minimal functional impact.

    Low
    • Update

    @jsfillman
    Copy link
    Owner Author

    /ask "Tell me about the new Faust DSP code."

    @qodo-code-review
    Copy link

    Ask

    Tell me about the new Faust DSP code.

    Answer:

    The PR adds a dual 3-tap delay effect to the Faust DSP code in legato_synth.dsp. Here's a breakdown of the implementation:

    Delay Parameters (18 new OSC-controllable parameters)

    • Left Channel Controls:

      • delay_L_enable: Toggle to enable/disable left channel delay
      • Three taps with time (0-2000ms) and level (0-1) controls:
        • delay_L_tap1_time (default: 125ms) and delay_L_tap1_level (default: 0.3)
        • delay_L_tap2_time (default: 250ms) and delay_L_tap2_level (default: 0.2)
        • delay_L_tap3_time (default: 500ms) and delay_L_tap3_level (default: 0.1)
      • delay_L_feedback: Controls feedback amount (0-0.95, default: 0.2)
      • delay_L_mix: Dry/wet balance (0-1, default: 0.3)
    • Right Channel Controls:

      • Similar structure but with slightly different default times (150ms, 300ms, 600ms)
      • All parameters have _R suffix instead of _L

    Delay Implementation

    • Time Conversion: ms_to_samples function converts milliseconds to samples based on sample rate
    • Delay Processing: Two separate processing functions (delay_L_process and delay_R_process)
    • Tap Structure: Each channel uses three delay taps with the @ operator for sample-accurate delays
    • Mixing: Dry/wet mixing controlled by the mix parameter
    • Bypass: Clean bypass when delay is disabled using select2 function

    Signal Flow

    • The delays are inserted after the filter stage but before the gain stage
    • The final output is now delayed_sigL * gain, delayed_sigR * gain instead of directly using the filtered signals

    Safety Features

    • Feedback is limited to 0.95 maximum to prevent runaway feedback
    • All level parameters are clamped to 0-1 range

    This implementation provides a versatile stereo delay effect with independent control over each channel, allowing for complex rhythmic patterns and spatial effects.

    @jsfillman
    Copy link
    Owner Author

    /analyze

    @qodo-code-review
    Copy link

    qodo-code-review bot commented May 27, 2025

    PR Analysis 🔬

    • This screen contains a list of code components that were changed in this PR.
    • You can initiate specific actions for each component, by checking the relevant boxes.
    • After you check a box, the action will be performed automatically by PR-Agent.
    • Results will appear as a comment on the PR, typically after 30-60 seconds.
    fileChanged components
    __init__.py
    • Test
    • Docs
    • Improve
    • Similar
     
    get_dsp_path
    (function)
     
    +4/-1
     
    ramp_test.py
    • Test
    • Docs
    • Improve
    • Similar
     
    test_ramp
    (function)
     
    +4/-2
     
    main_window.py
    • Test
    • Docs
    • Improve
    • Similar
     
    __init__
    (method of MurnauUI)
     
    +6/-4
     
    • Test
    • Docs
    • Improve
    • Similar
     
    init_ui
    (method of MurnauUI)
     
    +85/-20
     
    • Test
    • Docs
    • Improve
    • Similar
     
    _create_delay_controls
    (method of MurnauUI)
     
    +264/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    init_parameters
    (method of MurnauUI)
     
    +23/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    stop_midi
    (method of MurnauUI)
     
    +22/-6
     
    • Test
    • Docs
    • Improve
    • Similar
     
    _handle_midi_cc
    (method of MurnauUI)
     
    +34/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_enable_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap1_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap1_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap2_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap2_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap3_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_tap3_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_feedback_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_L_mix_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_enable_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap1_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap1_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap2_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap2_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap3_time_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_tap3_level_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_feedback_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    on_delay_R_mix_change
    (method of MurnauUI)
     
    +3/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    send_osc
    (method of MurnauUI)
     
    +7/-3
     
    conftest.py
    • Test
    • Docs
    • Improve
    • Similar
     
    pytest_configure
    (function)
     
    +8/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    pytest_collection_modifyitems
    (function)
     
    +24/-0
     
    • Test
    • Docs
    • Improve
    • Similar
     
    qapp
    (function)
     
    +13/-0
     

    💡 Usage guide:

    Using static code analysis capabilities, the analyze tool scans the PR code changes and find the code components (methods, functions, classes) that changed in the PR.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR:

    /analyze
    

    Language that are currently supported: Python, Java, C++, JavaScript, TypeScript, C#.
    See more information about the tool in the docs.

    @qodo-code-review
    Copy link

    qodo-code-review bot commented May 27, 2025

    Generated docstring for '_create_delay_controls' 📚

      _create_delay_controls (method) [+264/-0]

      Component signature:

      def _create_delay_controls(self, layout):

      Docstring:

      """
      Create delay control widgets for the 3-Tap Delay effect section.
      
      This method builds the UI components for the stereo delay effect with separate
      controls for left and right channels. Each channel has three delay taps with
      adjustable time and level, plus feedback and mix controls.
      
      Parameters
      ----------
      layout : QVBoxLayout
          The parent layout to which the delay controls will be added.
          
      Notes
      -----
      The delay controls include:
      - Enable/disable toggles for each channel
      - Three delay taps per channel with adjustable time (0-2000ms) and level (0-1)
      - Feedback control (0-0.95) to determine the amount of signal fed back into the delay
      - Mix control (0-1) to balance between dry and wet signal
      
      Each control is connected to its corresponding handler method and has an assigned
      MIDI CC number for external control.
      """

    @jsfillman
    Copy link
    Owner Author

    /improve
    --pr_code_suggestions.num_code_suggestions_per_chunk="4"
    --pr_code_suggestions.commitable_code_suggestions=true

    Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
    @qodo-code-review
    Copy link

    qodo-code-review bot commented May 27, 2025

    CI Feedback 🧐

    (Feedback updated until commit cbe48ad)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: test (3.10)

    Failed stage: Run tests with coverage [❌]

    Failure summary:

    The action failed due to a syntax error in the file src/murnau/ui/main_window.py at line 948. The
    error message indicates that there is a missing except or finally block after a try block for the
    process_midi method. This syntax error prevented the tests from being collected and executed
    properly, causing three test files to fail during collection:

  • tests/test_dsp_init.py
  • tests/test_main_window.py
  • tests/test_osc_client.py

    Additionally, the test coverage was only 4.73%, which is below the required 20% threshold.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    682:  Cannot create RT messagebuffer thread: Operation not permitted (1)
    683:  Retrying messagebuffer thread without RT scheduling
    684:  Messagebuffer not realtime; consider enabling RT scheduling for user
    685:  no message buffer overruns
    686:  jackdmp 1.9.21
    687:  Copyright 2001-2005 Paul Davis and others.
    688:  Copyright 2004-2016 Grame.
    689:  Copyright 2016-2022 Filipe Coelho.
    690:  jackdmp comes with ABSOLUTELY NO WARRANTY
    691:  This is free software, and you are welcome to redistribute it
    692:  under certain conditions; see the file COPYING for details
    693:  JACK server starting in realtime mode with priority 10
    694:  self-connect-mode is "Don't restrict self connect requests"
    695:  Cannot lock down 107341340 byte memory area (Cannot allocate memory)
    696:  Cannot use real-time scheduling (RR/10) (1: Operation not permitted)
    697:  AcquireSelfRealTime error
    698:  ##[group]Run # Start virtual display for PyQt6 tests
    ...
    
    708:  Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.17/x64
    709:  Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.17/x64
    710:  Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.17/x64
    711:  LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.17/x64/lib
    712:  QT_QPA_PLATFORM: offscreen
    713:  DISPLAY: :99
    714:  ##[endgroup]
    715:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/coverage/report_core.py:110: CoverageWarning: Couldn't parse Python file '/home/runner/work/murnau/murnau/src/murnau/ui/main_window.py' (couldnt-parse)
    716:  coverage._warn(msg, slug="couldnt-parse")
    717:  ============================= test session starts ==============================
    718:  platform linux -- Python 3.10.17, pytest-8.3.5, pluggy-1.6.0
    719:  PyQt6 6.9.0 -- Qt runtime 6.9.0 -- Qt compiled 6.9.0
    720:  rootdir: /home/runner/work/murnau/murnau
    721:  configfile: pytest.ini
    722:  plugins: qt-4.4.0, mock-3.14.1, cov-6.1.1
    723:  collected 49 items / 3 errors
    724:  ERROR: Coverage failure: total of 5 is less than fail-under=20
    725:  ==================================== ERRORS ====================================
    726:  ___________________ ERROR collecting tests/test_dsp_init.py ____________________
    727:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/python.py:493: in importtestmodule
    ...
    
    736:  ???
    737:  <frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
    738:  ???
    739:  <frozen importlib._bootstrap>:688: in _load_unlocked
    740:  ???
    741:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/assertion/rewrite.py:185: in exec_module
    742:  exec(co, module.__dict__)
    743:  tests/test_dsp_init.py:10: in <module>
    744:  from src.murnau.dsp import get_dsp_path  # noqa: E402
    745:  src/murnau/__init__.py:13: in <module>
    746:  from .ui.main_window import MurnauUI
    747:  src/murnau/ui/__init__.py:3: in <module>
    748:  from .main_window import MurnauUI
    749:  E     File "/home/runner/work/murnau/murnau/src/murnau/ui/main_window.py", line 948
    750:  E       def process_midi(self):
    751:  E   SyntaxError: expected 'except' or 'finally' block
    752:  __________________ ERROR collecting tests/test_main_window.py __________________
    753:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/python.py:493: in importtestmodule
    ...
    
    762:  ???
    763:  <frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
    764:  ???
    765:  <frozen importlib._bootstrap>:688: in _load_unlocked
    766:  ???
    767:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/assertion/rewrite.py:185: in exec_module
    768:  exec(co, module.__dict__)
    769:  tests/test_main_window.py:13: in <module>
    770:  from src.murnau.ui.main_window import MurnauUI  # noqa: E402
    771:  src/murnau/__init__.py:13: in <module>
    772:  from .ui.main_window import MurnauUI
    773:  src/murnau/ui/__init__.py:3: in <module>
    774:  from .main_window import MurnauUI
    775:  E     File "/home/runner/work/murnau/murnau/src/murnau/ui/main_window.py", line 948
    776:  E       def process_midi(self):
    777:  E   SyntaxError: expected 'except' or 'finally' block
    778:  __________________ ERROR collecting tests/test_osc_client.py ___________________
    779:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/python.py:493: in importtestmodule
    ...
    
    788:  ???
    789:  <frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
    790:  ???
    791:  <frozen importlib._bootstrap>:688: in _load_unlocked
    792:  ???
    793:  /opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/site-packages/_pytest/assertion/rewrite.py:185: in exec_module
    794:  exec(co, module.__dict__)
    795:  tests/test_osc_client.py:10: in <module>
    796:  from src.murnau.utils.osc_client import OSCClient  # noqa: E402
    797:  src/murnau/__init__.py:13: in <module>
    798:  from .ui.main_window import MurnauUI
    799:  src/murnau/ui/__init__.py:3: in <module>
    800:  from .main_window import MurnauUI
    801:  E     File "/home/runner/work/murnau/murnau/src/murnau/ui/main_window.py", line 948
    802:  E       def process_midi(self):
    803:  E   SyntaxError: expected 'except' or 'finally' block
    804:  ================================ tests coverage ================================
    ...
    
    807:  --------------------------------------------------------------
    808:  src/murnau/__init__.py               7      1    86%   15
    809:  src/murnau/dsp/__init__.py           8      8     0%   11-31
    810:  src/murnau/synth/__init__.py         3      0   100%
    811:  src/murnau/synth/melody.py          43     34    21%   18, 31-43, 54-74, 92-119, 124, 128
    812:  src/murnau/synth/ramp_test.py       24     19    21%   20-57, 62, 66
    813:  src/murnau/ui/__init__.py            3      2    33%   4-6
    814:  src/murnau/ui/widgets.py           397    397     0%   3-790
    815:  src/murnau/utils/__init__.py         2      2     0%   3-5
    816:  src/murnau/utils/osc_client.py      20     20     0%   3-61
    817:  --------------------------------------------------------------
    818:  TOTAL                              507    483     5%
    819:  Coverage XML written to file coverage.xml
    820:  FAIL Required test coverage of 20% not reached. Total coverage: 4.73%
    821:  =========================== short test summary info ============================
    822:  ERROR tests/test_dsp_init.py
    823:  ERROR tests/test_main_window.py
    824:  ERROR tests/test_osc_client.py
    825:  !!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!!
    826:  ============================== 3 errors in 0.74s ===============================
    827:  ##[error]Process completed with exit code 2.
    828:  Post job cleanup.
    

    @jsfillman jsfillman self-assigned this May 27, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants