added pianoview context menu for first/last/base, fixed small reset v…

[spechtstatt]

This implements a context menu for the instrument piano view for setting the base note and the first/last key as discussed in the issue #6224. The context menu is only available in the key section of the PianoView. The automation context menu behavior in the black stripe above is not changed.

"Set single key" sets the first and last key so just a single key is left:

The menu entries are disabled accordingly if the operation would just set the same value as already set.

Additionally the shape of the base note marker is changed from a rectangle to an arrow:

A small bugfix is also applied - the value of the base note an the first/last key was set by a setInitValue call instead of setValue (seems to be the same bug as described in #6213). So the reset value reflects now the correct initial value as seen here:

The getNoteString method was moved to the PianoView an renamed to getNoteStringByKey to make it possible to use it from both the PianoRoll and the PianoView.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.223+g1ef676296-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.223+g1ef676296-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.223+g1ef676296-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.223+g1ef676296-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/2d2dc455-0e31-4e84-abff-8673cc789e8a/artifacts/0/lmms-1.3.0-alpha.1.223+g1ef676296-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17756?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/9617ba9d-361b-408f-b850-1254c9d6950b/artifacts/0/lmms-1.3.0-alpha.1.223+g1ef676296-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17758?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/84e43705-19b7-4e6d-aa46-54b09346a233/artifacts/0/lmms-1.3.0-alpha.1.223+g1ef676296-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17757?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/445fb3nhkmu00sd4/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44129404"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ann470ibqes4g0pw/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44129404"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/cda2bec2-16fd-45ce-ba3b-ddb154d5a6dc/artifacts/0/lmms-1.3.0-alpha.1.223+g1ef676296-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17759?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "38ff344d693aa6ba84f4a82893b3a99e8ad57af1"}

[superpaik]

It works ok on W10-64b.
One behaviour that I don't know if it's intended. Start and end arrows can cross resulting in a "no" active keys on the keyboard. I don't see the use case of that behaviour.
On the GUI aspect, it's difficult to see the start or end arrow when it's over a note name. I don't know if it could be solved by adding a black (background colour) border to the arrow. It's not a big deal, though.

[allejok96]

I can understand why you didn't change the context menu of the black stripe, but let's brainstorm for a sec...

The black stripe is where you click to move the arrow. So context menu that moves the arrow should intuitively be there too (besides on the piano key).

There are other situations where a automatable control could benefit from special options in its context menu (for example the tempo box or time signature). I don't if there is a clean way to do this...

[spechtstatt]

Yes - it's where I started with the context menu first, but there is this functionality of getting the nearest marker for displaying the automation context menu.

This gives us two possibilities:

1. mix two different positions (marker and current key) in one context menu.

2. remove the "nearest" functionality and display the marker menu exactly (including key menu) and the key menu only in all other cases.

I also wanted to display the key which gets to be selected in the header - I did this for the marker menu header so you see the marker key.

So I tried 2) first, but I found it not very practical to use because it is relatively difficult to target the marker exactly with the right menu button. 1) on the other hand is difficult for displaying what is selected (marker) and what gets to be selected (key) and I wanted to avoid writing the key to each of the new entries (maybe a subheader would solve this)

Another thing is that the selection of a key on the piano roll is easier than on the black bar. Of course we could remove the black bar restriction and show a combined context menu everywhere.

So I get your argument, but I am not sure how to merge this two approaches (nearest marker and exact key including an intuitive selection) in one context menu in a clean way.

[spechtstatt]

@superpaik: thank you for the feedback.

Yes - I also discovered the "cross key" behaviour, but it was already present so I didn't touch it. But I can fix it in the course of this PR.

Regarding the black border: good idea. I will try if this improves the visibility.

[superpaik]

I understand the logic of having the context menu where the icon is displayed, but to me is better to have the context menu on the piano keys, because the action is "Set this exact key as... (base note, first, last key)". If context menu is on the black stripe that is no so clear.

[allejok96]

I think this PR is a big improvement. My concern is not that it is illogical with the new piano menu. Instead I think it may become so logical that users never realize that the black bar contains some extra features, because no one will ever click on it.

Now people will argue that if you want to automate or MIDI-control base note you will know about this feature already. Still, could we "force" new users to discover these features, and not turn it into just another dev only feature? I'm trying to think of ways to find a middle ground.

Problem that spawned this PR: Users can set base note but does not discover the note range setting. How about a TextFloat similar to the master volume? Print base note and note range. Whenever you click the black bar you'll see that there is a range setting possible.

Problem created by this PR: Users spoiled with piano menu, will never click the black bar and discover all of its interactivity. What about forcing users to click it by removing the base note option from the menu? A bit of a regression yes, but is it really that different? With the new arrow it will still be an improvement, more visually clear that the bar is some form of control.

[spechtstatt]

@allejok96 : couldn't it be that the context menu on the piano roll would motivate people to try if there is also a context menu on the black bar? Or that they just click there because they don't expect that there would be a different context menu?

And the question is generally if the new context menu can even solve the underlying problem that the first/last key including the automation menu is not very discoverable in the first place.

For the automation menu the markers itself may need a different style so it would be more obvious that they are automatable controls - more 3D - like the usual knobs.

[spechtstatt]

Some proposals:

• move the text to the keys to avoid the marker/text "collision" at all
• use a thin "slider" line
• use green to have a stronger indication that the marker is a kind of (automatable) control

The "full" version:

Or keep the white but with the thin "slider" line:

Or simply classical but keep the text in the keys:

[superpaik]

The first proposal from @spechtstatt it's the best to me, though I know it may be not easy to move note names to keys.
But if that is not possible, having the markers in green it's a good solution to show users that there is "something" in there.

[spechtstatt]

Funny thing: the classic theme has green markers already.

[spechtstatt]

I have applied the changes so far - looks like this for now:

I think it is more obvious now that these markers are a kind of control thing.

The text rotation was a nightmare though :-)

I also addressed the "crossing key" bug. It is not possible anymore to cross the first/last key with the mouse and also the key context menu disables the entry accordingly.

And I added also the missing shortcut identifier for the "Set single key" menu.

[spechtstatt]

By the way: not sure if another master rebase was a good idea?

[spechtstatt]

Should I change this PR to "ready for review" or are there any change requests regarding the implementation?

[allejok96]

For some reason this PR includes the mixer rename commit. As I understand it, the only way you can push after a rebase is with push --force, but it doesn't look like you've done that here.

[spechtstatt]

So is it bad to do a master rebase from the lmms repo on the personal feature branch?

I mean I could create a new branch and just sync my changes again (and add a new PR) if this is easier.

How do you normally handle it if you want to update a branch?

Because I am still not a git expert - far from it :-)

[allejok96]

I have no idea what's going on. It must be some github issue... git diff locally doesn't show all these changes, and merging went just fine. I have done rebases before without this happening. Still, were you required to git push --force ?

[allejok96]

Tested this

git checkout master
git pull upstream master
git checkout -b messyPR
git rebase spechstatt/feature/pianoview-context-menu
git push --set-upstream origin/messyPR

Tried making a PR. It included your commits followed by Broken datafile commit and the Clipboard rename commit (which shouldn't show as a part of the PR). So I removed those two and tried again:

git reset --hard 'HEAD^^'  # careful with this
git push --force

Now your commit was the last one. Tried making a PR and everything seemed fine.

[PhysSong]

So is it bad to do a master rebase from the lmms repo on the personal feature branch?

Not in this situation.

[spechtstatt]

I hope that I have fixed the commit issues - "files changed" looks now normal here.

I repaired it like @allejok96 was suggesting. I used "git reflog" to identify the head xx number before the whole mess started. Then I used this number for the git "reset --hard HEAD@{xx}" command followed by a "git push -f" call.

So this leads back to my question if I should set this PR "ready for review" now or if there are any further issues regarding my implementation?

[allejok96]

Generally very good, just a few things that can be cleaned out.

[allejok96]

should set this PR "ready for review"

Looks good. Go for it! Wait. I reviewed it anyway. 😄 doesn't matter you can change things later

[allejok96]

I think we could trim it down just a bit more... I had this test code lying around.

[allejok96]

Everything looks good, for what it's worth

[spechtstatt]

@allejok96: this PR still shows that a change is requested from your side but I am not able to find the change request and I am also not able to resolve it. Is there anything you can do about it?

[allejok96]

Ah sorry, LGTM

[DomClark]

Sorry for the massive delay in getting round to reviewing this again; allejok96 reminded me earlier today about it.

This basically all looks good to me now. I've left a few comments on things that could be cleaned up or tweaked, but nothing major.

[spechtstatt]

Does anyone have an idea how I can fix the mvsc errors?

[DomClark]

The MSVC builds were failing due to an upstream problem. It has now been fixed, so I've restarted them. They should hopefully pass without further changes.

[messmerd]

I tested this PR on Linux Mint 20.3, and everything seems to be working fine.

However, I have one suggestion:
In the PianoRoll::drawNoteRect method at line 1010, if you change it from this

if( n->selected() )
{
    col = QColor( selCol );
}
const int borderWidth = borders ? 1 : 0;

to this:

if (n->selected())
{
    col = QColor(selCol);
}
else if (!m_midiClip->instrumentTrack()->isKeyMapped(n->key()))
{
    const int gray = qGray(col.rgb());
    col.setRgb(gray, gray, gray, col.alpha());
}

const int borderWidth = borders ? 1 : 0;

It will draw disabled notes as grayscale when they are not selected.
I think it looks a lot better that way. It makes it clear to the user when notes are outside the first/last range.