Skip to content
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.

Sign up for GitHub

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 LMMS crash when pressing Q in not existing piano roll. #3609

Merged
merged 3 commits into from
Jun 7, 2017
Merged

Fixed LMMS crash when pressing Q in not existing piano roll. #3609

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffd9f65
Fixed LMMS crash when pressing Q in not existing piano roll.
karmux Jun 4, 2017
b2776ad
Using hasValidPattern() and coding style fixes.
karmux Jun 4, 2017
2cbd4a0
Don't wrap whole function inside if clause.
karmux Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions src/gui/editors/PianoRoll.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ void PianoRoll::setBackgroundShade( const QColor & c )



void PianoRoll::drawNoteRect( QPainter & p, int x, int y,
void PianoRoll::drawNoteRect( QPainter & p, int x, int y,
int width, const Note * n, const QColor & noteCol,
Copy link
Member

@zonkmachine zonkmachine Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You have one requested fix in here together with 14 lines that are formatting changes like whitespace at the end etc. This makes it a bit hard to review. I suggest for the next time to commit changes like this separately and name the commit 'fixup' or 'whitespace' or something similar.

Copy link
Member

Choose a reason for hiding this comment

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

@zonkmachine agreed. Harder to cherry-pick too as the more lines that get modified increases the chances of a merge conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to switch off editor's automatic cleanups :)

const QColor & selCol, const int noteOpc, const bool borders )
{
Expand Down Expand Up @@ -1918,7 +1918,7 @@ void PianoRoll::mouseReleaseEvent( QMouseEvent * me )
{
// select the notes within the selection rectangle and
// then destroy the selection rectangle
computeSelectedNotes(
computeSelectedNotes(
me->modifiers() & Qt::ShiftModifier );
}
else if( m_action == ActionMoveNote )
Expand Down Expand Up @@ -2463,15 +2463,15 @@ void PianoRoll::dragNotes( int x, int y, bool alt, bool shift, bool ctrl )
}
}
}
}
}
else if (m_action == ActionResizeNote)
{
// When resizing notes:
// If shift is not pressed, resize the selected notes but do not rearrange them
// If shift is pressed we resize and rearrange only the selected notes
// If shift + ctrl then we also rearrange all posterior notes (sticky)
// If shift is pressed but only one note is selected, apply sticky

if (shift)
{
// Algorithm:
Expand All @@ -2491,8 +2491,8 @@ void PianoRoll::dragNotes( int x, int y, bool alt, bool shift, bool ctrl )
const Note *posteriorNote = nullptr;
for (const Note *note : notes)
{
if (note->selected() && (posteriorNote == nullptr ||
note->oldPos().getTicks() + note->oldLength().getTicks() >
if (note->selected() && (posteriorNote == nullptr ||
note->oldPos().getTicks() + note->oldLength().getTicks() >
posteriorNote->oldPos().getTicks() + posteriorNote->oldLength().getTicks()))
{
posteriorNote = note;
Expand All @@ -2512,9 +2512,9 @@ void PianoRoll::dragNotes( int x, int y, bool alt, bool shift, bool ctrl )
if(note->selected())
{
// scale relative start and end positions by scaleFactor
int newStart = stretchStartTick + scaleFactor *
int newStart = stretchStartTick + scaleFactor *
(note->oldPos().getTicks() - stretchStartTick);
int newEnd = stretchStartTick + scaleFactor *
int newEnd = stretchStartTick + scaleFactor *
(note->oldPos().getTicks()+note->oldLength().getTicks() - stretchStartTick);
// if not holding alt, quantize the offsets
if(!alt)
Expand All @@ -2533,7 +2533,7 @@ void PianoRoll::dragNotes( int x, int y, bool alt, bool shift, bool ctrl )
int newLength = qMax(1, newEnd-newStart);
if (note == posteriorNote)
{
posteriorDeltaThisFrame = (newStart+newLength) -
posteriorDeltaThisFrame = (newStart+newLength) -
(note->pos().getTicks() + note->length().getTicks());
}
note->setLength( MidiTime(newLength) );
Expand Down Expand Up @@ -3918,28 +3918,28 @@ int PianoRoll::quantization() const

void PianoRoll::quantizeNotes()
{
if( m_pattern != NULL ) {
if( hasValidPattern() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already better than before but I was also asking to use the following coding pattern:

if ( !hasValidPattern() )
{
  return;
}

NoteVector notes = getSelectedNotes();

// ...
// Everything that's inside the `if` statement follows here

This is an often used pattern when the scope of an if statement encompasses a whole method. By doing it like this you also get rid of one indentation level for the code that's inside the if clause which makes the code more readable.

NoteVector notes = getSelectedNotes();

if( notes.empty() )
{
for( Note* n : m_pattern->notes() )
{
notes.push_back(n);
notes.push_back( n );
}
}

for( Note* n : notes )
{
if (n->length() == MidiTime(0))
if( n->length() == MidiTime( 0 ) )
{
continue;
}

Note copy(*n);
m_pattern->removeNote(n);
copy.quantizePos(quantization());
m_pattern->addNote(copy);
m_pattern->removeNote( n );
copy.quantizePos( quantization() );
m_pattern->addNote( copy );
}

update();
Expand Down