Clean up PianoRoll selection

[allejok96]

In order of appearance:

• Convenience functions: getTick yCoordOfKey xCoordOfTick (taken from paintEvent)
• m_select* members store start/end instead of start/length
• Timeline selection made to work like Song editor - first draw box, then apply (see video). draws a box
• If code wants to check for ongoing selection, check m_action, don't care about m_editMode. This makes removeSelection(), unnecessary.
• Bugfix: Do not end selection if Ctrl is released. This fixes bug when ctrl is released, then pressed again.
• Bugfix: Box selection always includes the key where selection started (see video).
• Bugfix: getKey() was 2 pixels off, and keys below the bottom line was a whole key off.
• selectAll() simply selects all notes, instead of drawing a box around them (see video)

select.mp4

[Spekular]

• Timeline selection made to work like Song editor - first draw box, then apply (see video).

Is there any benefit to this change? The current behavior provides clearer feedback on what's selected.

• Bugfix: Do not end selection if Ctrl is released. This fixes bug when ctrl is released, then pressed again.

I would consider this expected behavior.

• Bugfix: Box selection always includes the key where selection started (see video).

I tend to click near the horizontal lines when box selecting and expect the box to select notes either above or below the line depending on my selection endpoint. If there's any bug here I'd say it's the fact that selecting near the bottom line of a note includes that note in the selection, rather than snapping the selection edge to the closest grid line.

Rest looks good to me.

[allejok96]

Is there any benefit to this change?

• Consistency, above all.
• Discoverability. Shows the user what this action does, even if there are no notes.
• If we ever implement Esc as a way to cancel a box select, this way will leave the old selection unaltered.

The current behavior provides clearer feedback on what's selected.

Yes, I like the visual feedback too. But either way it should be consistent with box select (and with song editor)

I would consider this expected behavior.

I disagree. Actions where a modifier key needs to be pressed before the mouse button should only finish when the mouse button that started it is released. (In contrast to things like unqunatize, which can be toggled on/off during drag). Pressing Esc, another mouse button or key should, if anything, cancel the action. Just look at file managers, GIMP, Blender and LMMS knob automation.

I tend to click near the horizontal lines when box selecting

This is a interesting and good point. The only exception would be when clicking to select a single note. Maybe there is some way to combine these two? I will give it a look.

[Spekular]

Is there any benefit to this change?

• Consistency, above all.

Piano roll box select is the one that should be changed then. Both selection modes in the song editor (box and timeline) select during the action, as does (currently) the timeline select in piano roll.

• Discoverability. Shows the user what this action does, even if there are no notes.

Can be accomplished without removing the selection.

• If we ever implement Esc as a way to cancel a box select, this way will leave the old selection unaltered.

Ought to be possible either way.

I would consider this expected behavior.

I disagree[,] look at file managers, GIMP, Blender and LMMS knob automation.

I can't say I've ever thought to release ctrl when doing a ctrl+drag action, but you seem to be correct. Carry on then :)

This is a interesting and good point. The only exception would be when clicking to select a single note.

I would do this by dragging from slightly above it to slightly below. If the selection top & bottom always snaps to the nearest note edge this works and can be rationalized as consistent w/ the song editor. There's also a "select all notes on this key" option when right clicking piano keys.

[Veratil]

Not needed, but preferred.

[Veratil]

I've sat on replying to this until now since I was thinking about the changes (since I've been focused on the piano roll more than any other dev lately).

• I don't think we need to break out the xCoordOfTick and yCoordOfKey since they are used only in paintEvent, but there are similar class functions in AutomationEditor and StepRecorderWidget so I'm okay with it
• I think some of the getTick calculations aren't using the same formula, but pulling it into a function is a good update
• I like the timeline selection showing the lines 👍
• I was with Spekular on the Ctrl key release because I was in the same thought that you held it down all the time, but if it doesn't actually do that in the other programs then I'm cool with the enhancement/bugfix
• Not showing the selection box with selectAll is probably better anyway since it's suppose to select all 😂
• "bugfix" of selecting by whole key instead of "top" of key is better IMO
• Did you test all the different ways to select notes after removing the m_editMode check? There are some weird code paths so just want to make sure

Other than that, looks okay to me.

[allejok96]

Good thoughts @Veratil.

xCoordOfTick and yCoordOfKey ... are used only in paintEvent

After realizing this I was about to revert them. But then again they might prove useful in the future for drawNoteRect or other stuff extracted from pain[t]Event. The equation is easy to get wrong (which has proven to be the case). So since you say it's ok, I'll keep them.

Did you test all the different ways to select notes after removing the m_editMode check?

No, I will have to double-check the code paths.

[allejok96]

Piano roll box select is the one that should be changed then.

@Spekular, this was more complicated than expected, due to toggle-selection with shift. We'd have to do something like note->oldSelected(). Better make that another PR. Then we could add Esc support to that one.

[allejok96]

• Styling
• Timeline both selects notes and shows the blue lines
• Code paths checked, no problem. Although discovered that a whole section in mouseMoveEvent is unreachable (see FIXME).
• Went loco on computeSelectedNotes. Did not change the logic, appart from removing special treatment of 0 length notes, as they don't exist afaik.

---

EDIT: this part is reverted

• Selection snaps from (key) grid line to grid line, but if it is just one key, it always picks the key where the mouse is. So holding ctrl and clicking a note will always select that note.
  • I'm not a big fan of the logic behind this, but this was the third and best solution I came up with:
  • mousePressEvent stores the initial click position in m_selectStartTick and m_selectStartKeyLine
  • mouseMoveEvent handles all the dirty logic, rounding and stuff
  • The m_selection* members store the nicely computed and ready-to-use selection bounds

[Spekular]

Code wise things LGTM, but I want to test drive this before approving so it doesn't do anything unexpected. O build bot where art thou?

[allejok96]

I would do this by dragging from slightly above it to slightly below. If the selection top & bottom always snaps to the nearest note edge this works and can be rationalized as consistent w/ the song editor.

Just tested this code again and I disagree. If I start a selection with the mouse hovering a note, that note should always be selected, no matter which grid line I was closest to. See problem:

select.mp4

This actually matches the behavior of Song editor, because it selects whatever the selection box touches. The major difference is that the selection box does not snap to the grid in Song editor.

[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! 🎩

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.182+g7f6999799-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.182+g7f6999799-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)

Linux

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

macOS

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

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f90ef64c-7064-4dc9-b4bc-978968ef22a6/artifacts/0/lmms-1.3.0-alpha.1.182+g7f6999799-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16148?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/f89bd314-ee72-4149-ad29-c03599897dc1/artifacts/0/lmms-1.3.0-alpha.1.182+g7f6999799-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16147?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/kqmnrqdvsop1a9fk/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43201305"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/hui7a6re3lg5jxdf/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43201305"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/74cc832a-0b82-4121-9be8-68bdba6aca9c/artifacts/0/lmms-1.3.0-alpha.1.182+g7f6999799-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16145?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d56bc8e1-3587-4679-ad04-58aa49444e6b/artifacts/0/lmms-1.3.0-alpha.1.182+g7f6999799-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16146?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "f7be357809f77435e90c06ee981d8d48fb60dfb4"}