New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed recording of sustained midi notes #3710

Merged
merged 3 commits into from Jul 26, 2017

Conversation

Projects
None yet
4 participants
@serdnab
Contributor

serdnab commented Jul 20, 2017

Fixes #1700.
This recovers the behavior done by 4b340f7, and later lost.
When a midinoteff comes from midi, and sustain pedal is pressed, the noteplayhandle pointer don't get nullified, so later, when sustain pedal is released, it can catch that pointer to emit the signal that recording of note is finished.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 20, 2017

Member

Should the target branch be master? #1700 is in 1.2.0 milestone.

Member

PhysSong commented Jul 20, 2017

Should the target branch be master? #1700 is in 1.2.0 milestone.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 20, 2017

Member

Should the target branch be master? #1700 is in 1.2.0 milestone.

Yes, or we backport to stable-1.2.

@serdnab I've tested this and it works fine. It can't be tested completely because of #3537.
As commented in the review you can cut down the number of if()'s.
Would it be rude to suggest you to have a look at #3537 too?

Member

zonkmachine commented Jul 20, 2017

Should the target branch be master? #1700 is in 1.2.0 milestone.

Yes, or we backport to stable-1.2.

@serdnab I've tested this and it works fine. It can't be tested completely because of #3537.
As commented in the review you can cut down the number of if()'s.
Would it be rude to suggest you to have a look at #3537 too?

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Jul 21, 2017

Contributor

Made the shortening of the if's.

Would it be rude to suggest you to have a look at #3537 too?

I will take a look at it.

Contributor

serdnab commented Jul 21, 2017

Made the shortening of the if's.

Would it be rude to suggest you to have a look at #3537 too?

I will take a look at it.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 21, 2017

Member

I will take a look at it.

Awesome!

I restarted the Travis build. It sometimes will fail for reasons that are not related to the changes made.

Member

zonkmachine commented Jul 21, 2017

I will take a look at it.

Awesome!

I restarted the Travis build. It sometimes will fail for reasons that are not related to the changes made.

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Jul 21, 2017

Contributor

Addressed the remarks.
I realized that the "else" is not necessary, if not isSustainPedalPressed(), m_sustainPedalPressed is already false.

Contributor

serdnab commented Jul 21, 2017

Addressed the remarks.
I realized that the "else" is not necessary, if not isSustainPedalPressed(), m_sustainPedalPressed is already false.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 21, 2017

Member

I realized that the "else" is not necessary, if not isSustainPedalPressed(), m_sustainPedalPressed is already false.

So, drop the test and just let there be: else

Member

zonkmachine commented Jul 21, 2017

I realized that the "else" is not necessary, if not isSustainPedalPressed(), m_sustainPedalPressed is already false.

So, drop the test and just let there be: else

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Jul 23, 2017

Contributor

I believe you misunderstood me.
The if(isSustainPedalPressed()) is necessary because m_sustainPedalPressed can be true or false, what is not necessary is a possible "else" for the if(isSustainPedalPressed()) clause.

Contributor

serdnab commented Jul 23, 2017

I believe you misunderstood me.
The if(isSustainPedalPressed()) is necessary because m_sustainPedalPressed can be true or false, what is not necessary is a possible "else" for the if(isSustainPedalPressed()) clause.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 24, 2017

Member

I believe you misunderstood me.

I believe you're right there... :)
Looking at this again.

what is not necessary is a possible "else" for the if(isSustainPedalPressed()) clause.

This is true, but the if( event.controllerValue() > MidiMaxControllerValue/2 ) above it still needs the else. You're testing a state, but this is the event that sets the state. Maybe you can show how you're thinking in code instead?

Member

zonkmachine commented Jul 24, 2017

I believe you misunderstood me.

I believe you're right there... :)
Looking at this again.

what is not necessary is a possible "else" for the if(isSustainPedalPressed()) clause.

This is true, but the if( event.controllerValue() > MidiMaxControllerValue/2 ) above it still needs the else. You're testing a state, but this is the event that sets the state. Maybe you can show how you're thinking in code instead?

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 24, 2017

Member

@serdnab Or you can drop else if you put current else if clause first, though current code looks better. I suggest keeping this code.

Member

PhysSong commented Jul 24, 2017

@serdnab Or you can drop else if you put current else if clause first, though current code looks better. I suggest keeping this code.

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Jul 24, 2017

Contributor

I am not explaining well, then you are not understanding.
Anyway, the code I intend is the current one.

Contributor

serdnab commented Jul 24, 2017

I am not explaining well, then you are not understanding.
Anyway, the code I intend is the current one.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 24, 2017

Member

The only difference I see is that with dropping the test you have a built in safe guard in case of an unexpected state. When we get a pedal off command when m_sustainPedalPressed is false. This may never happen and/or may not have any side effects at all. I don't know if I'm being picky here. If other developers are cool with this I vote merge.

Member

zonkmachine commented Jul 24, 2017

The only difference I see is that with dropping the test you have a built in safe guard in case of an unexpected state. When we get a pedal off command when m_sustainPedalPressed is false. This may never happen and/or may not have any side effects at all. I don't know if I'm being picky here. If other developers are cool with this I vote merge.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 24, 2017

Member

@zonkmachine I suggest merging without more change.

Member

PhysSong commented Jul 24, 2017

@zonkmachine I suggest merging without more change.

@zonkmachine zonkmachine merged commit 2464a57 into LMMS:master Jul 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 26, 2017

Member

OK. Squashed and merged. Thank you so much Andrés for fixing a long standing bug!

Member

zonkmachine commented Jul 26, 2017

OK. Squashed and merged. Thank you so much Andrés for fixing a long standing bug!

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong
Member

PhysSong commented Jul 26, 2017

@zonkmachine backport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment