lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Adds ablitiy and hotkeys to move controller rack modules up and down

Open Rossmaxx opened this issue 11 months ago • 13 comments

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 https://github.com/LMMS/lmms/issues/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

Rossmaxx avatar Mar 11 '24 07:03 Rossmaxx

Tested. Context menu and hotkeys works fine. :+1:

zonkmachine avatar Apr 12 '24 15:04 zonkmachine

Is this pr based on https://github.com/LMMS/lmms/pull/6154 ?

zonkmachine avatar Apr 12 '24 16:04 zonkmachine

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

Rossmaxx avatar Apr 13 '24 05:04 Rossmaxx

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: ...".

zonkmachine avatar Apr 13 '24 07:04 zonkmachine

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

Rossmaxx avatar Apr 13 '24 07:04 Rossmaxx

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.

zonkmachine avatar Apr 13 '24 08:04 zonkmachine

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).

Rossmaxx avatar Apr 13 '24 15:04 Rossmaxx

@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.

michaelgregorius avatar Apr 13 '24 15:04 michaelgregorius

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

And squash it down to one commit probably.

zonkmachine avatar Apr 13 '24 15:04 zonkmachine

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. :sweat_smile:

michaelgregorius avatar Apr 13 '24 16:04 michaelgregorius

Noted

Rossmaxx avatar Apr 14 '24 00:04 Rossmaxx

@michaelgregorius i fixed it using an interactive rebase.

Rossmaxx avatar Apr 14 '24 03:04 Rossmaxx

@michaelgregorius can i get an approval here?

Rossmaxx avatar May 22 '24 09:05 Rossmaxx

@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.

michaelgregorius avatar May 22 '24 18:05 michaelgregorius

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

sakertooth avatar May 22 '24 19:05 sakertooth

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

sakertooth avatar May 23 '24 09:05 sakertooth

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

sakertooth avatar May 23 '24 10:05 sakertooth