Add ability to move Controllers and hotkeys for moving Controllers & Effects #6154
Conversation
|
Tested on W10-64, works well. |
|
@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 |
ejaaskel
force-pushed
the
movable-controllers
branch
from
5d983c9 to
665b56e
Compare
|
@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. |
|
@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. |
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 LMMS#5773
Instead of showing generic LFO Controller in context menu, show the actual name of the controller. This change improves user experience.
ejaaskel
force-pushed
the
movable-controllers
branch
from
665b56e to
c662977
Compare
|
@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! |
ejaaskel
changed the title
|
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 |
|
To me is Ok. Thanks! |
|
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? |
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)