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.

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

Conversation

@karmux
Copy link
Contributor

@karmux karmux commented Jun 4, 2017

Steps to reproduce:

  1. Open LMMS
  2. Open Piano-Roll
  3. Click on Q
  4. LMMS crashes.

This change fixes it.
Link to issue: #3608.

@@ -3918,31 +3918,33 @@ int PianoRoll::quantization() const

void PianoRoll::quantizeNotes()
{
NoteVector notes = getSelectedNotes();
if( m_pattern != NULL ) {

This comment has been minimized.

@michaelgregorius

michaelgregorius Jun 4, 2017
Contributor

While this definitively works I'd propose to use the same style like in PianoRoll::cutSelectedNotes():

if( ! hasValidPattern() )
{
	return;
}

This make it more obvious what's tested and also keeps the indentation level of the method down.

@Umcaruje Umcaruje added this to the 1.2.0 milestone Jun 6, 2017
@Umcaruje Umcaruje added this to In Progress in Release 1.2.0 RC4 Jun 6, 2017
@@ -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,

This comment has been minimized.

@zonkmachine

zonkmachine Jun 7, 2017
Member

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.

This comment has been minimized.

@tresf

tresf Jun 7, 2017
Member

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

This comment has been minimized.

@karmux

karmux Jun 7, 2017
Author Contributor

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

Copy link
Member

@zonkmachine zonkmachine left a comment

Tested, works!
This PR has now been updated to conform with @michaelgregorius suggested changes. I suggest merge if there are no other concerns.

@@ -3918,31 +3918,33 @@ int PianoRoll::quantization() const

void PianoRoll::quantizeNotes()
{
NoteVector notes = getSelectedNotes();
if( hasValidPattern() ) {

This comment has been minimized.

@michaelgregorius

michaelgregorius Jun 7, 2017
Contributor

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.

@@ -3918,33 +3918,36 @@ int PianoRoll::quantization() const

void PianoRoll::quantizeNotes()
{
if( hasValidPattern() ) {
NoteVector notes = getSelectedNotes();
if( ! hasValidPattern() )

This comment has been minimized.

@michaelgregorius

michaelgregorius Jun 7, 2017
Contributor

That's what I meant. Thanks!

This comment has been minimized.

@zonkmachine

zonkmachine Jun 7, 2017
Member

Clean and simple. Retested, merging!

@zonkmachine zonkmachine merged commit 5678981 into LMMS:stable-1.2 Jun 7, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine moved this from In Progress to Done in Release 1.2.0 RC4 Jun 7, 2017
@tresf
Copy link
Member

@tresf tresf commented Jun 8, 2017

FYI, this commit is all ready to merge to master in liushuyu/merge.

Copy link
Contributor

@grejppi grejppi left a comment

This is merely a cosmetic thing

NoteVector notes = getSelectedNotes();

if (notes.empty())
if( notes.empty() )

This comment has been minimized.

@grejppi

grejppi Jul 2, 2017
Contributor

This is a new function, and therefore the old coding style (wrt spaces inside parentheses) should not apply. I realize this is late, but still 😅

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.