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 shortcut for Chord Inversions #7718

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

regulus79
Copy link
Contributor

@regulus79 regulus79 commented Feb 17, 2025

This PR adds the ability to invert a selection of notes in the PianoRoll, where the top note moves down n octaves/bottom note moves up n octaves. This feature works on selections of notes too, moving all notes on the min/max key up/down. It also works on microtonal scales which may have different octave sizes.

image

Edit: The keyboard shortcuts have been reversed. Now Shift+U is for Upward inversion.

The icons probably need to be changed. Also I think the two options might take up too much space in the menu. I was thinking of adding a sub menu, but I'm not sure Qt allows that.

2025-02-16.19-43-23.mp4

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine and the functions works as intended. I've thrown complex note patterns at it and you can invert the patterns up/down and it seem to end up where you started. I don't think you can expect it to work that well for all combinations but it may well be the case. I don't think it has to though as there is always Ctrrl+Z. Job well done.
Please note that if you push the notes up/down far enough they will 'clip' at the top/bottom note and will no longer be a chord but a single note in the end (invert it up 10 octaves and down again...). This may or may not be desired but is not a part of this PR.

@szeli1
Copy link
Contributor

szeli1 commented Feb 25, 2025

I'm not sure Qt allows that.

If it is a context menu, then qt will allow it.

Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed and tested this and it works as intended. I'm not sure where this would be useful, but if there is a use for this, then this can be merged.

@regulus79
Copy link
Contributor Author

If it is a context menu, then qt will allow it.

It seems that it's not exactly a context menu(?) The actions are added directly to the tool button with noteToolsButton->addAction(), instead of adding them to a menu and adding the menu to the button. I tried making a sub menu and adding it to the button, but that didn't work. I think there is a function to add a whole menu object instead of adding action by action, which might work. I haven't tried that yet, but in any case that would probably be for a separate PR.

@tresf tresf requested a review from bratpeki February 26, 2025 03:31
@bratpeki
Copy link
Member

I don't know how I feel about this feature, mainly since Ctrl+UpArrow/Ctrl+DownArrow exist. I feel like it's bloating our context menu but not giving a lot in return.

I dunno, if users are pushing for it to be in LMMS, I'll gladly test it, but I personally am not a big fan.

@zonkmachine
Copy link
Member

I don't know how I feel about this feature, mainly since Ctrl+UpArrow/Ctrl+DownArrow exist

This again. How does these features compare?

I'll gladly test it, but I personally am not a big fan.

You haven't tested it?

I haven't tried that yet, but in any case that would probably be for a separate PR.

Separate PR is fine with me. Merge is fine by me.

@bratpeki
Copy link
Member

This again. How do these features compare?

One is a subset of the other, which is why I feel like we're adding in a feature that already "exists".

You haven't tested it?

No, not yet. I would like to give my opinion and talk about the feature before testing.

@regulus79
Copy link
Contributor Author

I don't know how I feel about this feature, mainly since Ctrl+UpArrow/Ctrl+DownArrow exist. I feel like it's bloating our context menu but not giving a lot in return. ... One is a subset of the other, which is why I feel like we're adding in a feature that already "exists".

Ctrl+UpArrow/Ctrl+DownArrow shifts the whole selection up/down an octave, while this feature shifts the chord up one note at a time. But I get what you mean, that you can get this same effect by selecting each note you want to move, and then using Ctrl+Up/Down. However, I found that to be quite time consuming when building chord progressions. Also, I personally think it's useful to be able to quickly invert chords to get a "new sound" out of them, which can be fun for getting new ideas for a chord progression.

But I do agree it's kinda bloating the context menu lol. I can do something about that in a separate PR if it becomes a large enough issue.

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.

4 participants