lmms
lmms copied to clipboard
LMMS
Add ability to move Controllers and hotkeys for moving Controllers & Effects
This commit adds UI changes, signals, and handlers required for moving controllers up and down in a controller rack. The feature was requested in the issue #5773. The PR also adds hotkeys for moving controllers and effects in their respective racks, with a combination of ALT + Up/Down.
The implementation is similar to the one used with EffectRacks. Quite small change, good for a first issue to solve (as it was mentioned in the issue itself)
- Added move up and move down buttons to context menu
- Added signals that'll message the requested move
- Added functions in the ControllerRackView that'll handle those signals
- Added QActions for the hotkeys to move Controllers & Effects
Tested on W10-64, works well. Just an observation that I believe comes from "history". When you open the popup menu over Effects Chain the title of the window is the "selected" FX. When open it in the Controller Rack a generic title "LFO Controller" is shown. If possible and simple, it would be better to show the name of the controller both for consistency and so you know exactly which controller you are moving.
@superpaik Thanks for trying it out! And I can see what you mean with your comment, I'll check out how the name could be added to the LFO popup menu
@superpaik The fix for the contextMenu name seemed to be fairly straightforward, hopefully it's a good solution. Besides that, I had to rebase the branch because I had messed up the git author info on my VM, should be all good now.
It works OK. IMO, it's more useful than before and it's in line with Effects Chain.
@ejaaskel I've been doing some code research and I think that it's very straightforward to add the ability to move Controllers with keyboard shortcuts as well (like in the FX mixer). I don't have a setup (yet) to integrate code with github, so if you are willing (and think it's a useful functionality) to modify this PR to add this new behaviour I can share the code I have tested.
@superpaik Definitely, that sounds like a good thing to add as well. Do you have the code in a fork for cherry-picking, or do you want to share it some other way?
Sorry, I have the code locally. You only need to define a new function (in the ControllerView) "keyPressEvent" and handle the Alt+Up and Alt+Down keys to call your functions. I attach the files here with the changes. The new code is identified with //@NewCode. Double-check that the code follow LMMS code standards, though. And what's interesting is that the same code works on the effectRack as well (EffectView.cpp and EffectView.h). Maybe you can create another PR to handle that. But please, do it only if you have time and feel like it. Otherwise, no worries. ControllerView.zip
@superpaik sounds good, and after a quick look the code seems to make sense as well. I'll look into integrating the changes the next week when I have more time
Perhaps it's possible to set a keyboard shortcut using QAction or QMenu->addAction without tampering with keyPressEvent?
@allejok96 Yeah, sounds sensible to not compare every keypress event in the UI element. I'll check it as well next week when going through this suggestion in a bit more detail.
@allejok96 @superpaik There should now be Alt + Up/Down hotkey for moving both Controllers and Effects in their respective racks. I made it using QActions in the end. I also rebased the branch on top of master again
Neat! Both work ok on W10.
In order for the reviewers to validate and merge the code, I think you should change the title and description of the PR in order to reflect the new scenario. Or even better, make a separate PR for the Effect rack. Thanks!
I edited the title and message to be up-to-date. If these changes still need to be separated into two separate PRs then I can of course do that, but to me they seem to work on so similar topics they could still be in one PR
What's the state of this pr? if it's abandoned, do you mind me taking this over?
Apologies for the lack of activity here. @Rossmaxx Yes, feel free to take over.
I have opened #7139. If you don't mind, can you close this PR in favour of that one?