Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add menu option and hotkeys to move controllers/effects #7139

Merged
merged 13 commits into from
May 23, 2024

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Mar 11, 2024

Supersedes #6154 and closes #5773.

from the original PR body.

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

@zonkmachine
Copy link
Member

Tested. Context menu and hotkeys works fine. 👍

@zonkmachine
Copy link
Member

Is this pr based on #6154 ?

@Rossmaxx
Copy link
Contributor Author

@zonkmachine yes. I asked the original author and took over the PR because they seem to have lost interest.

@zonkmachine
Copy link
Member

I asked the original author and took over the PR because they seem to have lost interest.

I don't see his name in the commit logs. I may have missed that but you should credit the original author in the commit history. I don't know what is the best way to do it but sometimes its just easier to push a bunch of changes to the original PR. Or you can keep the original commit with the original author intact and push it to a new branch with your added stuff to follow. Or you make sure to add a message in the final commit message "Based on original work by: ...".

@Rossmaxx
Copy link
Contributor Author

I actually copy pasted the diff so you won't see his name. So what's the best way to credit him then?

@zonkmachine
Copy link
Member

I actually copy pasted the diff so you won't see his name. So what's the best way to credit him then?

I wonder too so I bumped #dev-support.

Ensure that `build` and `target` are directories in `.gitignore`.
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Apr 13, 2024

I did something to try credit @ejaaskel per request from zonk. For some reason, looks like @michaelgregorius is getting some credit too due to a mess up i did with git (for a PR he already wrote).

@michaelgregorius
Copy link
Contributor

@Rossmaxx, the best way to keep everything intact would be something along the following lines:

  1. Add https://github.com/ejaaskel/lmms.git as a remote to your (local) repository.
  2. Checkout the branch movable-controllers by @ejaaskel.
  3. Optional: Merge the current master into the branch if necessary.
  4. Add your changes on top.
  5. Push the branch to your remote at GitHub.
  6. Create a new pull request from your remote.

This way GitHub should see all the involved commits and credit @ejaaskel appropriately.

@zonkmachine
Copy link
Member

Optional: Merge the current master into the branch if necessary.

And squash it down to one commit probably.

@michaelgregorius
Copy link
Contributor

Optional: Merge the current master into the branch if necessary.

And squash it down to one commit probably.

This can also be done during the final merge by using the option "Squash and merge" in GitHub. At least that's what I am using after being told to do so. 😅

@Rossmaxx
Copy link
Contributor Author

Noted

@Rossmaxx
Copy link
Contributor Author

@michaelgregorius i fixed it using an interactive rebase.

@Rossmaxx
Copy link
Contributor Author

@michaelgregorius can i get an approval here?

@michaelgregorius
Copy link
Contributor

@Rossmaxx, with regards to the functionality everything is working for me. However, it would be nice if there was an indication of what element is currently selected so that the users know which element will be affected by the action.

I think the code might also get simpler if there was a concept of a selected element because some of the signals would not be needed in that case. The shortcuts would be defined for the widget that presents the list and if activated the widget would search for the selected element and work on that. With the current code a list element knows how to move itself (by emitting a signal which is caught by the parent) which is a little bit unintuitive IMO.

However, I guess that's not really in the scope of this PR.

@sakertooth
Copy link
Contributor

Sorry, forgot about this. I saw a couple of problems with this that I wanted to mention. Give me a second.

src/gui/ControllerView.cpp Outdated Show resolved Hide resolved
src/gui/EffectView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerRackView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerRackView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerRackView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerRackView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerView.cpp Outdated Show resolved Hide resolved
src/gui/ControllerView.cpp Outdated Show resolved Hide resolved
src/gui/EffectRackView.cpp Outdated Show resolved Hide resolved
src/gui/EffectRackView.cpp Outdated Show resolved Hide resolved
Rossmaxx and others added 5 commits May 23, 2024 10:50
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
@sakertooth
Copy link
Contributor

Had a feeling I would break build..., sorry, just one second.

@sakertooth
Copy link
Contributor

I'm going to push my fixes directly here since its easier (and safer) compared to reviewing it on Github again.

@sakertooth sakertooth changed the title Adds ablitiy and hotkeys to move controller rack modules up and down Add menu option and hotkeys to move controllers/effects May 23, 2024
@Rossmaxx Rossmaxx merged commit b803e92 into LMMS:master May 23, 2024
9 checks passed
@Rossmaxx Rossmaxx deleted the controller-updown branch May 23, 2024 12:10
sakertooth added a commit to sakertooth/lmms that referenced this pull request May 27, 2024
* added controller rack modules

* remove this-> from setFocusPolicy() and obsolete comment

Co-authored-by: saker <sakertooth@gmail.com>

* Use std::swap

Co-authored-by: saker <sakertooth@gmail.com>

* some more cleanup

Co-authored-by: saker <sakertooth@gmail.com>

* Replace slots with function pointers

Co-authored-by: saker <sakertooth@gmail.com>

* Apply fixes

---------

Co-authored-by: ejaaskel <esa.jaaskela@suomi24.fi>
Co-authored-by: saker <sakertooth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controller Rack - Ability to move controller modules up and down
5 participants