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

Add option to show note values on notes in Piano Roll (#4466) #4467

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

michaelgregorius
Copy link
Contributor

Add the option to show note values on notes in the Piano Roll. This
functionality is currently coupled with the option "Enable note labels
in piano roll" which can be found in the main menu.

The notes are rendered at about 80% of the notes height. They are only
rendered if they fit on the whole note and if the font does not become
too tiny.

Enable the configuration of the note value text's color via the
stylesheets and set the value to white for both shipped themes.

Other changes:

  • Clean up some warnings about old school casts and implicit casts.

lmms-4466-notetext

Add the option to show note values on notes in the Piano Roll. This
functionality is currently coupled with the option "Enable note labels
in piano roll" which can be found in the main menu.

The notes are rendered at about 80% of the notes height. They are only
rendered if they fit on the whole note and if the font does not become
too tiny.

Enable the configuration of the note value text's color via the
stylesheets and set the value to white for both shipped themes.

Other changes:
* Clean up some warnings about old school casts and implicit casts.
Copy link
Member

@Wallacoloo Wallacoloo left a comment

Choose a reason for hiding this comment

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

Nice improvement! Fix the tabbing in the .css files, and then I'm happy with it :)

@@ -123,6 +123,7 @@ PianoRoll {
qproperty-backgroundShade: rgba( 255, 255, 255, 10 );
qproperty-noteModeColor: rgb( 255, 255, 255 );
qproperty-noteColor: rgb( 119, 199, 216 );
qproperty-noteTextColor: rgb( 255, 255, 255 );
Copy link
Member

Choose a reason for hiding this comment

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

Should probably use tab indentation instead of spaces to match the style of the surrounding lines (same with the change to data/themes/default/style.css).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 27bd8aa.

{
p.setPen(noteTextColor);
p.setFont(noteFont);
QPoint textStart(x + xOffset, y + (noteTextHeight + (noteHeight - noteTextHeight) / 2));
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, the calculation of the y-coordinate (baseline) could be expressed more succinctly as y + (noteHeight + noteTextHeight) / 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 27bd8aa.

Use tabs in the style sheets. Make the calculation of the Y offset more
concise.
@michaelgregorius
Copy link
Contributor Author

@Wallacoloo Thanks for the code review! I have implemented all of your proposals with commit 27bd8aa.

@musikBear
Copy link

But not for 'frozen' 1.2.0 -right. This is for 'later' - or?

@michaelgregorius
Copy link
Contributor Author

@musikBear This pull request is made for the master branch, i.e. the changes are not intended for the frozen 1.2.0 version.

@michaelgregorius michaelgregorius merged commit 8dab817 into LMMS:master Jul 10, 2018
@michaelgregorius michaelgregorius deleted the 4466-Notes-with-Text branch July 10, 2018 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants