Switch loop marker shortcuts, add context menu #6021
Conversation
🤖 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
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14078-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg451e91c-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14078?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://14079-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg451e91c8c-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14079?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14080-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg451e91c8c-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14080?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/tq6hjy9jv0l24cw5/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39690694"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/mi3cd7xqait418nk/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39690694"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14077-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg451e91c8c-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14077?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "d8d60910feaa400097e0dc2ddfb6922a7c2d286d"} |
|
I experimented a bit with rebinding support, but you'd either need to introduce severe restrictions or drastically rework the mouse event functions. In particular, the It's very easy to handle dragging the loop start, loop end, or playhead without it, just check mouse button and modifiers in the mouse move handler. Dragging with mean based logic should work without it if you always recalculate the closest point in the move event, but this could get hairy when the points approach each other. Selection needs state to remember where the selection started, and needs to emit selectionFinished when done. In the end I decided it wasn't worth it, since there's not much difference between e.g. using ctrl or shift for an operation. |
|
It works just fine, as stated in the PR description. Some thoughts about it.
|
|
I'd be very surprised to see the playhead quantized when moving. Even with mostly programmed stuff there are many elements that comes in slightly before grid lines (e.g. preshifted claps) that you can easily account for with an unquantized playhead. If you start working with recorded material things get even less perfectly aligned. As for moving the loop, I agree that it'd be useful but I don't think unmodified LMB should do anything except playhead movement. If that's desireable then IMHO we should make a vertical division, so there's a playhead lane and a loop marker lane. Of course, that takes up more vertical space since you want the lanes to remain a comfortable height. Maybe I need to give remapping some more thought. |
|
I think playhead position shouldn't be automatically quantized, especially when manually moved. Maybe Ctrl+Shift+LMB inside loop to move entire loop section? |
|
I see your point regarding not quantizing the playhead, but I think that in any other use case the default behaviour is quantizing and you have an option for not quantizing it (like in the loop marker, midi/beat/sample track end, etc.) so thats why I said it, but it is ok to me, because I haven't notice it until I tested this PR and thought about how it should behave. As for context, Ableton quantize the playhead by default and with Alt+LMB you can move it freely. As for moving the whole loop the option Ctrl+Shift+LMB, as qnebra said, it's already taken for Loop Start (fine). The option Spekular propose would be perfect, but I guess it would require a major code change for just this functionality (unless is used for future functionalities like song markers). I don't know if ALT+LMB is an option because of cross platform issues, but another suggestion would be "doble-click" and move it until next click. Another option, which I think doesn't require a new lane, is that when inside (and only when inside) the loop marker and the cursor is at the top of the lane (above the playhead arrow) change the cursor to an double-arrow so you can move the loop with single LMB (see image). Perhaps with low resolution screens this may be an issue. Anyway, they where just thoughts that came to my mind while testing. Don't be too worried. |
IanCaio
left a comment
IanCaio
left a comment
There was a problem hiding this comment.
Still need to give a more thoughtful review and test it, but at first glance it looks good. There are some formatting changes I can later put on a review for a batch commit if you want. In this one I only addressed this conditional that can be one-lined to match the previous one.
|
Ok, I came up with a binding system that I think strikes a nice balance between flexibility and amount of code. I've reworked the PR quite a bit and as such edited the main post. |
IanCaio
left a comment
IanCaio
left a comment
There was a problem hiding this comment.
Mostly style changes and some doubts/suggestions. The code looks good, behavior is nice given some time to get used to it.
I found the "DragLoop" action name a bit misleading at first. Until I downloaded the build and tested it I thought that action was supposed to drag both loop points simultaneously (thus dragging the loop). I'd maybe rename it to "DragSelectLoop", which sounds more like what it does.
src/gui/TimeLineWidget.cpp
Outdated
| } | ||
|
|
||
| // Calculate the clicked position as a TimePos, several actions need this | ||
| m_moveXOff = s_posMarkerPixmap->width() / 2; |
There was a problem hiding this comment.
If the action is MovePositionMarker, doesn't this override the m_moveXOff = event->x() - m_xOffset; statement if the offset is smaller than the marker width? As far as I understood the statement was added there to make a more precise playhead movement, but it seems like this line undoes it.
There was a problem hiding this comment.
It does, but as far as I can tell moving the playhead still moves perfectly fine. Frankly I have no idea what m_moveXOff is supposed to do or in what edge case it ends up not being half the pixmap width. I've tried clicking within the playhead triangle, and clicking so that it's by the start of the song, and everything works.
There was a problem hiding this comment.
I see, it's probably not noticeable because the width is very small. We should probably decide between both ways though. If it's being overridden without apparent issue, maybe we should just delete the lines that calculate the offset when it's smaller than the width of the marker.
There was a problem hiding this comment.
I should also note that when I checked git blame, this seems to have been here a very long time. I'm not sure who originally added it, but I doubt they're around to explain what it's for.
IanCaio
left a comment
IanCaio
left a comment
There was a problem hiding this comment.
Sorry it took a while. On a second review nothing stood out besides those small comments!
src/gui/TimeLineWidget.cpp
Outdated
| } | ||
|
|
||
| // Calculate the clicked position as a TimePos, several actions need this | ||
| m_moveXOff = s_posMarkerPixmap->width() / 2; |
There was a problem hiding this comment.
I see, it's probably not noticeable because the width is very small. We should probably decide between both ways though. If it's being overridden without apparent issue, maybe we should just delete the lines that calculate the offset when it's smaller than the width of the marker.
Co-authored-by: IanCaio <[email protected]>
IanCaio
left a comment
IanCaio
left a comment
There was a problem hiding this comment.
I'd still rather we remove the irrelevant line (compiler might even be optimizing it out), but since it wasn't added by this PR particularly and everything else was fixed, I'm giving my approval!
* Function for setting loop marker * Nudge loop points instead of moving them to beginning/end and fix bug when end is set to 0.
|
(To everyone but Spekular) I've worked out a way to map RMB click to context menu, and RMB drag to something else. Now what would that something be?
The most desired of these two should be mapped to RMB drag. The other one could be mapped to mid button. Some people have no mid button, but no big deal, you can still set left/right manually. Personally I'd love the ability to quickly draw a loop. But I have a mouse, so any case is fine by me. I also suggest we drop the "mean select" since it made redundant by having "select left" and "select right". |
|
Personally, I think that moving the loop would make a bit of sense - as the multitude of modifiers used in this area performing the same behaviour could potentially be a bit confusing; am curious to hear other opinions, however. I agree on the drop due to redundancy - for some reason, I was under the assumption that that was already completed within this PR due to its creation. |
* Separate drag/click actions * Simplify mouse binding mechanism, only show help text on drag * Variables holding enums * Default to NoAction
|
is there a way to test this release with a macOS 10.14+ package? haven't seen @LmmsBot get invoked at any time after the initial commit, and can't compile these changes locally atm |
If you scroll to the CI list and click "details" next to ci/circleci: macos, then "Artifacts", you should see a link to download a mac build. I've copied it here: https://15068-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.143%2Bgcea1048db-mac10.14.dmg |
|
Can someone rerun this build? I don't currently seem to be able to do so haha |
|
The "remapping" part of this is a half measure, so I'm closing it in favor of a later PR that won't try to future-proof. |
7 participants
See #5505 and #6014
This PR now halfway-implements remappable loop marker controls, with limitations:
Future enhancements
Proposed default shortcuts:
LMB: Move playhead
RMB: Open context menu
Ctrl + LMB: Group select
Ctrl + RMB: Mean select
Shift + LMB: Loop Start
Shift + RMB: Loop End
Holding both modifiers disables snapping. If both are held at the start of the action, the shift action is selected.