PianoRoll: Draw note names on all white keys, highlight white keys #2285

Merged
merged 1 commit into from Sep 16, 2015

Projects

None yet

9 participants

@michaelgregorius
Contributor

Draws note labels on all (white) keys when selecting to do so in the
preferences ("Enable note labels in piano roll"). The old rather messy
implementation that drew them all over the place has been removed.

Furthermore, the lanes of the white keys are now slightly highlighted in
the piano roll.

pianorollchanges

@michaelgregorius
Contributor

Related to issue #2280.

@tresf
Member
tresf commented Aug 22, 2015

Very nice!

@midi-pascal
Contributor

👍 Very clear and readable.

@tresf
Member
tresf commented Aug 22, 2015

Prior to this, the C-notes were easier to locate. Can we consider keeping the C-notes quick to locate (bold perhaps)?

isWhiteKey could be shrunk a bit by converting to isBlackKey since 5 sharps/flats are less lines than 8.

To be more musically accurate, isSemitone would be a good replacement for isBlackKey as a theme might make the black keys orange, gray or brown in the future.

The only other criticism I have is that the switch/case blocks could use an extra indent on the case statements (or at least that's how I'm used to reading them).

Hope you don't mind the constructive criticism. 👍

@michaelgregorius
Contributor

I think drawing the C notes bold could look a bit odd because they might stick out too much. What do you think about drawing all other notes in a lighter color?
pianorollchanges-proposal
In this version I have also changed the horizontal lines of the notes to be drawn in a lighter color (less alpha) except for the lines of the C notes. That way it becomes mre obvious where a new octave starts.

I have no problem with implementing it in the isBlackKey form. We could still implement the isWhiteKey method simply as !isBlackKey(). Concerning the proposal to use isSemitone: it seems that a semitone denotes an interval so that might not be very concise either. I think something like isKeyOfTheCScale might also work.

I can change the switch/case statement, no problem.

Last but not least, thanks for the criticism! Constructive criticism is always welcome. 😃

@tresf
Member
tresf commented Aug 22, 2015

Top notch. Agree with all points. isBlackKey works then. 👍

@tresf
Member
tresf commented Aug 22, 2015

P.S. It is quite amazing what a difference the line can make in that scale. Good choice there!

@AndiEcker AndiEcker commented on the diff Aug 22, 2015
src/gui/editors/PianoRoll.cpp
+ switch (keyCode)
+ {
+ case 0:
+ case 5:
+ yCorrectionForNoteLabels = -4;
+ break;
+ case 2:
+ case 7:
+ case 9:
+ yCorrectionForNoteLabels = -2;
+ break;
+ case 4:
+ case 11:
+ yCorrectionForNoteLabels = 2;
+ break;
+ }
@AndiEcker
AndiEcker Aug 22, 2015

why not use this switch block instead of the one in the new function isWhiteKey() (aka isBlackKey()) and then also replace the if statement underneath with if ( yCorrectionForNoteLabels != 0 )?

@michaelgregorius
michaelgregorius Aug 23, 2015 Contributor

@AndiEcker Thanks for the review! I was thinking about that as well but then decided to not merge these functions because they are concerned with different things. The function isWhiteKey is concerned with determining whether a key number corresponds to a note in the C major scale. Therefore it's rather generic and might also be used in other context in the class as well. The commented code block on the other hand is very specific as it is used in the context of rendering the note labels on the keys.

Using something like yCorrectionForNoteLabels != 0 would be to implicit for my taste and might also break as soon as someone decides to also render the note labels for the black keys which then might have a non zero correction value as well.

@AndiEcker
AndiEcker Aug 25, 2015

Thanks for you explanations!!! I now fully agree with you - your approach is resulting in cleaner code (although more code lines;).

@michaelgregorius
michaelgregorius Aug 25, 2015 Contributor

your approach is resulting in cleaner code (although more code lines;).

Yes, there are always compromises one has to make when coding. 😉

@Umcaruje
Member

I just don't like the idea of having the white keys highlighted by default. If I'd want to have them all highlighted, I'd mark the C major scale in the scale settings. Maybe just highlight all of the C keys (if the goal is better orientation in the piano-roll)?

This is what it looks when you highlight the C major scale:

@michaelgregorius
Contributor

@Umcaruje I did not know that functionality before. I agree that always highlighting the white keys leads to rather messy results when that feature is enabled especially with a scale that's different from C major. The following picture shows a highlighted C harmonic minor scale with the C scale enabled as well:
pianorollchanges-harmonic-patched
And this is the same as shown in master, i.e. without the changes of this pull request:
pianorollchanges-harmonic-master
Although I have to admit that due to the color scale that's used for the different intervals I find the current implementation a bit busy as well. It's also a bit unfortunate that both the notes and the highlighting uses a blue color.

What do you think about switching to a rather subtle highlighting like in the screenshots above?

Last but not least, I think that the scale highlighting feature is hidden too deep. I knew from @Umcaruje's screenshot that it somehow must be possible to enable the highlighting. When I then switched the scale combo box nothing happened. From a usability point of view that's not a good thing in my opinion. I had to use the code to find out that there is a context menu available for the keyboard keys and that it's somehow connected to the scale and chord combo boxes.

Having another combo box that lists all 12 notes would be more intuitive in my opinion. The initial state would be "C" and "None". If you then select a scale from the combo box, e.g. "major", it would highlight C major. I know that the piano roll is space constrained already. However, that might be solved by using movable tool bars in the piano roll dialog and perhaps giving it a menu as well because it provides lots of functionality.

@musikBear

@michaelgregorius Absolutely marvelous! This will help a lot of - no really every new-comer!
Only thing, does it get more readable, and perhaps also more discrete, if the octave numbers are omitted?
The C-note shows the octave, and then its really not necessary to show octave on every key?
Otherwise 👏 -This is actually one of the most asked for features from forum!

@michaelgregorius
Contributor

Here's what the C harmonic minor scale would look like with subtle highlighting and the default highlighting of the C major scale disabled:
pianorollchanges-harmonic-subtle
To me it looks a bit clearer than the colorful implementation. Any other opinions?

We could also consider making the highlighting color a property of the piano roll so that it can be styled via style sheets.

@michaelgregorius
Contributor

Some more images. 😉

I have played around with movable toolbars for the piano roll because I know that there have been complaints that it is hard to use on size constraint displays. This is what I came up with:
pianorollchanges-movabletoolsbars
I have put the actions into several toolbars which are grouped logically. As you can see there are five groups / tool bars: "Transport controls", "Note controls", "Copy paste controls", "Timeline controls" and "Zoom and note controls" (this group is a bit mixed). Each group can be turned off and on using the shown context menu.

What you see is the default layout of the toolbars which saves horizontal space by putting the "Zoom and note controls" below the other toolbars. The toolbars can be moved but their positions are not saved. I have also disabled floating toolbars, i.e. they will always stay inside the window (top, bottom, left, right).

Any opinions? Shall I commit and push the changes on top of this pull request?

@Umcaruje
Member

To me it looks a bit clearer than the colorful implementation. Any other opinions?

We could also consider making the highlighting color a property of the piano roll so that it can be styled via style sheets.

+1 on both those things.


I think the toolbar breakdown thing should really be a PR/Issue of its own, since we don't really want to have that many conversations on the same issue :)

@tresf
Member
tresf commented Aug 23, 2015

[...] movable toolbars [...]

😮

@michaelgregorius
Contributor

@Umcaruje Done! I have created #2286 to document the changes and #2287 to implement them.

@tresf This is all done using stuff that's already available from Qt! If you look at the code changes you will see that it was a rather low hanging fruit. There is some not so nice code concerning m_toolBar from the Editor class because I had to override some of the defaults being taken in the Editor implementation. I don't think it's a good idea that every Editor automatically has some transport controls but for now this does not seem to create any problems (except some headscratching when reading some of the code 😉).

@musikBear

To me it looks a bit clearer than the colorful implementation. Any other opinions?

I would like to have more contrast -How does it look, if you have low volume on the notes.

We could also consider making the highlighting color a property of the piano roll so that it can be styled via style sheets.

👍 ! - If thats made, then everyone could choose contrast/ colors as liked : Big support to this!

@michaelgregorius
Contributor

@musikBear Here it is with low volume notes:
pianorollchanges-lowvolumenotes
I think the notes are toned down a bit much anyway. One solution might be to make the interval between 0% and 100% notes smaller (and to keep it somewhere in the middle). Another solution might be to keep the border of the notes at a constant value and to only tone down the filled area.

@Spekular
Contributor
@michaelgregorius
Contributor

This is how it looks with the borders always using the same color. Some of the notes are panned, some are very low volume and some very high volume.
pianorollchanges-lowvolumenotes-borderalways

@Spekular
Contributor
@musikBear

@michaelgregorius I like that edge
I think the notes in your image are much better! big 👍

@michaelgregorius
Contributor

How about using a thinner and slightly shorter line?
pianorollchanges-new_end_line

@Spekular I did not change the volume and panning drawing code so it's the same style as before. I only made the outline color constant and altered the handles. No/problem/with/the/slashes! 😉

@Spekular
Contributor
@michaelgregorius
Contributor

Pushed the latest changes which correspond to the state shown in the last screenshot above.

@tresf
Member
tresf commented Sep 15, 2015

@michaelgregorius what's keeping us from squashing and committing this? I think this is terrific and a well needed addition.

P.S. Administrator's note.... If you merge this yourself, please add it to the 1.2.0-RC1 release notes here:

https://github.com/LMMS/lmms/releases

Note: This 1.2.0-RC1 is a draft (and thus non-admins can't see it), and we must keep it that way or Travis will fire a build an put it on our homepage! 👍

@michaelgregorius
Contributor

@tresf, @Wallacoloo Is there something wrong with the master branch? I wanted to update my master branch to squash and merge this feature but the git pull resulted in a merge conflict in my repo although it should result in a fast forward.

Also, if I look at the master branch here on GitHub the state looks rather old. For example it only shows two commits for today and the next commits are from August 26. My merge from yesterday is completely missing. Is this the same for everyone else as well?

Edit: This is the current state shown in my fork:
masters-diverging
It states that my master is 62 commits ahead. If there is something wrong I'd guess that is has to do with the commits done today which should be the three commits that my master is behind.

@tresf Have you force pushed an older tree or something like that with the improved Japanese translation (72d13ed)?

@Wallacoloo
Member

@michaelgregorius better set aside a clone of your local repository in case something did go wrong - that way we can still restore it.

Perhaps something went wrong merging #2339? I noticed that @andrewrk was pushing his commits to a separate branch in the lmms repository, rather than a branch in his own repository, and every time he pushed a new commit I also had merge conflicts. Also, there's no commit message in github's log for that merge.

That's the only non-ordinary thing that's happened recently, although it doesn't seem like it would cause issues.

@tresf
Member
tresf commented Sep 15, 2015

@michaelgregorius @Wallacoloo I believe I caused this when I did a squash for the Japanese language translations. I'm not sure how this was possible, I only squashed the 4 commits. This is bad and I'll need help to fix this. Rewriting history is very bad on master and I knew this and I previewed my changes before pushing them but I think it wiped out history. F***.

@tresf
Member
tresf commented Sep 15, 2015

@michaelgregorius I may need you to recommit the lost commits. The stuff committed today can be cherry-picked back in if we can get the head back to where it is supposed to be. The problem with rewriting history on master is that the only remaining history of overwritten changes are those distributed to people like you. As @Wallacoloo said, copy your branch as it might be the best we have to fix this blunder. 🔫

@Wallacoloo
Member

@tresf I sync my master branch regularly. See here. I don't think the Japanese translation was the issue - that one is in my repo's history (from 3 days ago).

@tresf
Member
tresf commented Sep 15, 2015

I don't think the Japanese translation was the issue - that one is in my repo's history (from 3 days ago).

I squashed all of them with the new ones that came in today and I commented out the commits I didn't want to modify. Apparently it removed them from history, I should have left them in there as pick. Can you remove the offending commit and rebase master against yours? We can easily cherry-pick today's work if we can get the last 3 weeks back.

@michaelgregorius
Contributor

@tresf No problem. Shit happens. 😄 A copy should already be available here at GitHub in my fork (which is also a tad newer than @Wallacoloo's). A prime example for the advantages of distributed version control systems. 😃

So if no one has a newer version of master we could use this to reinstate everything to the state it was 24 hours ago. However, if someone has a newer version we should obviously use that one so that not too much work goes wasted. I will not push to my fork until this problem is solved so we should always have at least one good copy.

@Wallacoloo
Member

Github has a tool that can maybe be useful in finding the most up-to-date fork? https://github.com/LMMS/lmms/network

@Wallacoloo
Member

@michaelgregorius but yours is only 23 hours stale, so it may be easier to just replay any PRs merged today on top of your branch, rather than hunt for a newer one. We're unlikely to find one that's newer than that anyway, IMO.

@tresf
Member
tresf commented Sep 15, 2015

For the record... merged changes since blunder:

image

And what I believe to be lost: https://gist.github.com/tresf/2bf987cce317268b86c0#file-blunder-diff

I believe I have the latest on the Ubuntu machine that caused the issue... I'm going to try resetting back to before the problem commit and forcing... one moment...

@Wallacoloo
Member

Going through the closed PRs, sorted by recently updated, we should be good if we take @michaelgregorius's master, and cherry-pick the following commits onto it:

72d13ed
2aef279
232c65c

Although at the moment, @michaelgregorius is missing 5 commits from lmms' master, so that's at least 2 that are unaccounted for...

@tresf
Member
tresf commented Sep 15, 2015

Ok, done. Can you take a look now?

@tresf
Member
tresf commented Sep 15, 2015

@Wallacoloo yeah 72d13ed was the problem child, so I'm hesitant to touch that one. I'll just leave the unsquashed commits out there.

The other two can be cherry picked now, but first I'd like to get a sanity check.

@Wallacoloo
Member

@tresf see the diff on @michaelgregorius's fork. I think you can just cherry-pick the commits he has that you don't.

Looks like they're from PRs: #2297, #2339, #2310 and #2343.

@tresf
Member
tresf commented Sep 15, 2015

Ok. Done. how about now?

P.S. Instead of cherry-picking (I wasn't having luck one-by-one), I instead told git he was upstream and re-based against his.

@michaelgregorius
Contributor

@tresf Seems to be ok now. 😃

@Wallacoloo
Member

@tresf did you get 2aef279 and 232c65c (from #2285 (comment))? (I'd checkout lmms & grep the commit log, but I don't have those tools handy).

@tresf
Member
tresf commented Sep 15, 2015

@Wallacoloo no, not yet. Not sure how to grab them yet.

@tresf
Member
tresf commented Sep 15, 2015

Never-mind, you provided the correct hashes, I'll try those instead.

Edit No dice ambiguous argument '2aef279': unknown revision or path not in the working tree.

@Wallacoloo
Member

@tresf try reopening and re-merging #2045 and #2140, if the owners haven't deleted their branches.

@tresf
Member
tresf commented Sep 15, 2015

try reopening and re-merging #2045 and #2140, if the owners haven't deleted their branches.

I thought of that too... GitHub doesn't seem to have a reopen button....

@Wallacoloo
Member

@tresf then in your local repository, you'll have to add the associated remotes from those two PRs, fetch their branches, and then cherry-pick the two commits onto your master. Then push your master up to github (this shouldn't require and rewriting of history).

If you can't cleanly cherry-pick the commits, you'll have to merge the branches locally, and then push master to github.

@tresf
Member
tresf commented Sep 15, 2015

@Wallacoloo done for #2045 via 571e4fd but now a new garbage branch is showing up called revert-2045-patch-1 whatever that is.

For #2140, it's parent branch was deleted... not sure how to merge that one in.

@tresf
Member
tresf commented Sep 15, 2015

Ok... got it via dff7e3e

git fetch origin pull/2140/head:zonkytemp
git cherry-pick 2aef279

Wipes sweat from forehead. Now to delete this garbage branch I mysteriously created... :)

P.S. @michaelgregorius thanks for letting us use this PR for convo.

@andrewrk
Member

Perhaps something went wrong merging #2339? I noticed that @andrewrk was pushing his commits to a separate branch in the lmms repository, rather than a branch in his own repository, and every time he pushed a new commit I also had merge conflicts.

I certainly did not force push master. I force pushed the branch I was working on, which is common git practice. You had merge conflicts because I was keeping the pull request clean, so every time you needed to get new code from me you had to delete your branch (or reset --hard it) before pulling.

I would never do such a thing on master branch.

Also, there's no commit message in github's log for that merge.

It's right here: 799f830

@Wallacoloo
Member

@andrewrk I certainly did not mean to accuse you of anything (I apologize if I came off that way) - I was trying to find any possible source of the error. I tried to make it extra clear there was no blame there by saying "[Merging #2339] doesn't seem like it would cause issues."

It's right here: 799f830

Thankfully, yes - @tresf was able to recover everything. At the time, that commit and many others were not present in the lmms repository.

@tresf
Member
tresf commented Sep 15, 2015

I would never do such a thing on master branch.

I wish github had a non-smiling blushing emoticon right now...

@michaelgregorius michaelgregorius PianoRoll improvements (note on keys, subtle highlight, note rendering)
Draws note labels on all (white) keys when selecting to do so in the
preferences ("Enable note labels in piano roll"). The old rather messy
implementation that drew them all over the place has been removed.
When rendering note names on the keyboard keys the C notes are rendered
in a darker color than the other ones. Horizontal lines which do not
correspond to the C key are now also rendered in a more subtle way to
give more prominence to the start of an octave.

The user selected scale is now highlighted in a more subtle way.

The note borders are not toned down in relation to the volume anymore.
The handles on the right side of each note have been made slimmer and
shorter.
2ea7208
@michaelgregorius michaelgregorius merged commit b5f5844 into LMMS:master Sep 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelgregorius michaelgregorius deleted the michaelgregorius:piano-roll-improvements branch Sep 16, 2015
@tresf
Member
tresf commented Sep 16, 2015

Release notes: Draw note names on the white keys when "Enable note labels in piano roll" is selected (#2285)

👍

@andrewrk
Member

@andrewrk I certainly did not mean to accuse you of anything (I apologize if I came off that way) - I was trying to find any possible source of the error. I tried to make it extra clear there was no blame there by saying "[Merging #2339] doesn't seem like it would cause issues."

No worries. I just wanted to make it clear that I would not misuse collaborator privileges like that.

It's right here: 799f830
Thankfully, yes - @tresf was able to recover everything. At the time, that commit and many others were not present in the lmms repository.

Glad to hear the problem is resolved :-)

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