Skip to content

Commit

Permalink
Merge pull request #2 from hhromic/master
Browse files Browse the repository at this point in the history 
Added missing SND_SEQ_EVENT_CLOCK event (MIDI Real Time Clock message) f...
  • Loading branch information
@garyscavone
garyscavone committed Jan 18, 2014
2 parents c80bfb6 + eb300bf commit 4034140
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions RtMidi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,10 @@ extern "C" void *alsaMidiHandler( void *ptr )
if ( !( data->ignoreFlags & 0x02 ) ) doDecode = true;
break;

case SND_SEQ_EVENT_CLOCK: // MIDI timing clock
if ( !( data->ignoreFlags & 0x02 ) ) doDecode = true;
break;

case SND_SEQ_EVENT_SENSING: // Active sensing
if ( !( data->ignoreFlags & 0x04 ) ) doDecode = true;
break;
Expand Down

4 comments on commit 4034140

@hhromic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !
Thanks for accepting my pull-request! However I noticed that now the SND_SEQ_EVENT_CLOCK event case is duplicated! I don't fully understand what happened, but browsing a bit in GitHub it seems that you renamed TICK to CLOCK before (commit c80bfb6) and then merged my PR (commit eb300bf) all at the same time, leading to this code duplication. Sorry for the mess!

Also I think you should mention this GitHub repository somewhere in the RtMidi main page, so the Debian (and other) package mantainers can link it to the Debian packages and apply these fixes quickly =).

Cheers and thanks for the good work,
Hugo.

@radarsat1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what happened, it sounds like something that should have been flagged as a conflict during merge.

Anyways, wanted to respond to your second comment -- I agree the link needs updating, but I don't agree that packagers should be encouraged to apply fixes between releases. Give RtMidi some time to be tested and released before the Debian packages are updated, this will avoid headaches.

@hhromic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again! After checking it closely, I think the following happened:

  • Gary commited (locally?) an OS-X dynamic device fix, commit c80bfb6) where he also renamed SND_SEQ_EVENT_TICK to SND_SEQ_EVENT_CLOCK
  • He merged my PR which ADDED the new SND_SEQ_EVENT_CLOCK case.

The above is not technically a conflict hence Git would not flag anything. I suspect Gary did that change because I wrote him an email before I knew the existence of this rtmidi repo (because it isn't in the main page hehe). Then I made the PR and during merge both changes slipped silently.

My suggestion: simply delete the duplicate code or, better IMO, restore the SND_SEQ_EVENT_TICK case as it was before together with my PR. I can make another PR fixing this again if you wish. Please let me know what do you prefer.

Regarding about notifying packagers, yes you are right, official packages should be tested more. Let make them do the job as usual when RtMidi gets to 2.0.2 =). Meanwhile I will clone this repo to work with.

Thanks again!
Hugo.

@drewish
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put in #9 based on @hhromic's suggestion.

Please sign in to comment.