PianoRoll: fix getKey and remove dead code

[allejok96]

• Fixes: when right clicking on a piano key close to the lower edge of the screen, "Mark semitone" will mark the wrong key because it uses the upper edge of the context menu as reference.
• Fixes: getKey() is off by 2 pixels
• Introduces: yCoordOfKey() which is the reverse of getKey()
• Removes: section of code that can never be reached

[JohannesLorenz]

Only 2 editorials.

Checked:

• Code logic
• Code style
• Partial testing: I checked that the dead code cannot be reached

Suggesting to test the other points of the OP.

[allejok96]

Suggesting to test the other points of the OP

I glanced through the various mouse*Event() and I didn't see any obvious dead ends.

[JohannesLorenz]

Rework check OK, only grammar issues.

This PR can be tested.

[bratpeki]

• when right clicking on a piano key close to the lower edge of the screen, "Mark semitone" will mark the wrong key because it uses the upper edge of the context menu as reference.

I can recreate this!

---

The PR does indeed fix this! Approving as a tester.

[bratpeki]

Looked at the code. Again, I'm just starting out reviewing, but this looks great!

Gonna just sum up my understanding of the code and ask @allejok96 to confirm:

• Relative height is the y-coordinate relative to the visible bit of Piano Roll
• Absolute height is the y-coordinate relative to the whole Piano Roll
• getKey returns the key value (ex. A4 = 69) for the absolute height passed to it
• yCoordOfKey is the inverse function of getKey, per se

As for my questions, I'm gonna make them in the code viewer.

[bratpeki]

Why was the + 1 there in the first place?

[allejok96]

My guess is it compensated for the - 1 a few lines earlier.

[bratpeki]

Actually, other than that, I've no other questions, the comments are terrific!

[allejok96]

@bratpeki thank's for the testing. You're right about the relative and absolute values, although I just call them "distances from bottom" and not "coordinates" since Qt always measures coordinates from the upper left corner of the widget.

So with that in mind getKey takes a Y coordinate as input, and yCoordOfKey returns the Y coordinate of the top of key (both measured from top of editor).

[bratpeki]

Thanks for the clarification! @regulus79 can we get a brief look at this and then merge?

[regulus79]

I have not tested this, nor can I confirm that the code block removed cannot be reached. But, just from a brief look, the changes look fine. If other people have tested it, I'm fine with it.

[bratpeki]

@allejok96: Yeah, could you delve a bit into that bit of code that can't be reached? How are you sure that such a big chunk of code is unreachable?

[allejok96]

@bratpeki This is the outer shell of the if statement, starting on line 2459

// If mouse is below timeline OR user is performing an action
if( me->y() > PR_TOP_MARGIN || m_action != Action::None )
{
    ...
}
else
{
    // If left button is pressed AND select mode is active AND is performing select action
    if( me->buttons() & Qt::LeftButton &&				
					m_editMode == EditMode::Select &&
					m_action == Action::SelectNotes )
	{
	    // This cannot be reached since if action == SelectNotes it will always match the first statement
    }
}

[bratpeki]

Thanks for the clarification, @allejok96.

I see @regulus79 also thinks this is okay, @messmerd is okay with the code changes, @JohannesLorenz too. There have been no changes since my approval, so @regulus79 can we get this merged?