-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
Tested. Context menu and hotkeys works fine. 👍 |
Is this pr based on #6154 ? |
@zonkmachine yes. 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: ...". |
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`.
15d3d92
to
e69d0f7
Compare
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, the best way to keep everything intact would be something along the following lines:
This way GitHub should see all the involved commits and credit @ejaaskel appropriately. |
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. 😅 |
Noted |
63ecf1b
to
6a85189
Compare
@michaelgregorius i fixed it using an interactive rebase. |
@michaelgregorius can i get an approval here? |
@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. |
Sorry, forgot about this. I saw a couple of problems with this that I wanted to mention. Give me a second. |
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
Had a feeling I would break build..., sorry, just one second. |
I'm going to push my fixes directly here since its easier (and safer) compared to reviewing it on Github again. |
* 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>
Supersedes #6154 and closes #5773.
from the original PR body.