Menu ▾ ▴

#4938 Refactor handling of supported MIDI control changes

Verified
H T LilyPond
FIxed_2_19_47 (1)
Maintainability
2016-11-04
2016-07-20
H T LilyPond
No

This issue separates the code refactoring originally done for Issue 4745 from the enhancements implemented as part of that issue. This patch does not change any existing functionality (nor introduce any new features), its purpose is to only improve the maintainability of the current implementation by simplifying the interfaces, removing duplication of code, and cleaning up some terminology.

(Background: Issue 4745 is about the ability to annotate arbitrary MIDI CCs in LilyPond input. That issue still remains open due to still unresolved design questions raised during the review about the proper syntax and implementation for the annotations. Regardless of the eventual fate of the enhancement, I'd hate to see this code refactoring being lost, however, so that's why the current issue was created.)

http://codereview.appspot.com/309720043

Discussion

  • H T LilyPond

    H T LilyPond - 2016-07-20
    • summary: Add Audio_item and Midi_item subclasses for (MIDI control, value) changes --> Refactor handling of supported MIDI control changes
    • Description has changed:

    Diff:
    • Needs: -->
    • Type: Enhancement --> Maintainability
     

  • H T LilyPond

    H T LilyPond - 2016-07-20
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -1,20 +1,5 @@
-Add Audio_item and Midi_item subclasses for (MIDI control, value) changes
+This issue separates the code refactoring originally done for Issue 4745 from the enhancements implemented as part of that issue.  This patch does not change any existing functionality (nor introduce any new features), its purpose is to only improve the maintainability of the current implementation by simplifying the interfaces, removing duplication of code, and cleaning up some terminology.

-
-Refactor handling of MIDI control changes
-
-Handle the MIDI control value initialization from context properties
-(Staff_performer::new_audio_staff), control value changes
-(Midi_control_function_performer::announce_function_value_change), and
-value conversion for output
-(Midi_control_function_value_change::to_string) in the new
-Midi_control_change_announcer class.
-
-All MIDI control changes are now encoded using
-{Audio,Midi}_control_change items.  This change makes the old
-{Audio,Midi}_control_function_value_change classes obsolete.
-
-
-Rename Midi_control_function_performer to Midi_control_change_performer
+(Background: Issue 4745 is about the ability to annotate arbitrary MIDI CC changes in LilyPond input.  That issue still remains open due to still unresolved design questions raised during the review about the proper syntax and implementation for the annotations.  Regardless of the eventual fate of the enhancement, I'd hate to see this code refactoring being lost, however, so that's why the current issue was created.)

 http://codereview.appspot.com/309720043
     

  • H T LilyPond

    H T LilyPond - 2016-07-21
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -1,5 +1,5 @@
 This issue separates the code refactoring originally done for Issue 4745 from the enhancements implemented as part of that issue.  This patch does not change any existing functionality (nor introduce any new features), its purpose is to only improve the maintainability of the current implementation by simplifying the interfaces, removing duplication of code, and cleaning up some terminology.

-(Background: Issue 4745 is about the ability to annotate arbitrary MIDI CC changes in LilyPond input.  That issue still remains open due to still unresolved design questions raised during the review about the proper syntax and implementation for the annotations.  Regardless of the eventual fate of the enhancement, I'd hate to see this code refactoring being lost, however, so that's why the current issue was created.)
+(Background: Issue 4745 is about the ability to annotate arbitrary MIDI CCs in LilyPond input.  That issue still remains open due to still unresolved design questions raised during the review about the proper syntax and implementation for the annotations.  Regardless of the eventual fate of the enhancement, I'd hate to see this code refactoring being lost, however, so that's why the current issue was created.)

 http://codereview.appspot.com/309720043
     

  • Anonymous

    Anonymous - 2016-07-21
    • Patch: new --> review
     

  • Anonymous

    Anonymous - 2016-07-21

    passes make, make check and a full make doc.

     

  • Anonymous

    Anonymous - 2016-07-24
    • Patch: review --> countdown
     

  • Anonymous

    Anonymous - 2016-07-24

    Patch on countdown for July 27th.

     

  • Anonymous

    Anonymous - 2016-07-27
    • Patch: countdown --> push
     

  • Anonymous

    Anonymous - 2016-07-27

    Patch counted down - please push

     

    • H T LilyPond

      H T LilyPond - 2016-07-27

      The code changes for this issue comprise three patches, which should preferably be committed separately to best preserve the incremental nature of the changes. (I've tested that the code still compiles after each individual patch.) Thanks in advance for your help with pushing these.

      (Fortunately, these changes are the last ones that I currently have in my queue except for Issue 4920, with which, I think, I'm going to wait until the implementation stabilizes since during the review some other developers voiced their intentions to make code changes which could alter the behavior from what is currently documented in the patch issue.)

       

      Last edit: H T LilyPond 2016-07-27
      0001-Issue-4938-1-3-Add-Audio_item-and-Midi_item-subclass.patch
      0002-Issue-4938-2-3-Refactor-handling-of-MIDI-control-cha.patch
      0003-Issue-4938-3-3-Rename-Midi_control_function_performe.patch

  • Anonymous

    Anonymous - 2016-07-27
    • labels: --> FIxed_2_19_47
    • status: Started --> Fixed
    • Patch: push -->
     

  • Anonymous

    Anonymous - 2016-07-27

    author Heikki Tauriainen g034737@welho.com
    Thu, 7 Jan 2016 21:02:07 +0100 (22:02 +0200)
    committer James Lowe pkx166h@gmail.com
    Wed, 27 Jul 2016 22:54:06 +0100 (22:54 +0100)
    commit 528d28e079419b3a45098b42b8b2b4eb9f1b15b1

    author Heikki Tauriainen g034737@welho.com
    Thu, 7 Jan 2016 21:34:56 +0100 (22:34 +0200)
    committer James Lowe pkx166h@gmail.com
    Wed, 27 Jul 2016 22:54:15 +0100 (22:54 +0100)
    commit 14de2f189e75424bd0b04f4577386b4f0dde1a6e

    and

    author Heikki Tauriainen g034737@welho.com
    Wed, 20 Jul 2016 11:06:56 +0100 (13:06 +0300)
    committer James Lowe pkx166h@gmail.com
    Wed, 27 Jul 2016 22:54:23 +0100 (22:54 +0100)
    commit 23bb09401ef1d8c5f5256b8f7a6c002365b1d88c

    Many thanks as always Hiekki.

     

  • Federico Bruni

    Federico Bruni - 2016-11-04
    • status: Fixed --> Verified