-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add quality of life features to the piano roll #8143
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
base: master
Are you sure you want to change the base?
Conversation
…s. Ctrl + and ctrl - will zoom in and out in the piano and automation editor
| } | ||
| break; | ||
|
|
||
| case Qt::Key_Equal: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do these as a QAction too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that, this just seemed like a more straight forward approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use QKeySequence::ZoomIn and ZoomOut. That solves the issue with different keyboard layouts and platforms. For example, on my keyboard 0=} is the same key, and ?+\ is another key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with the QKeySequence it does work for zooming out, but doesn't perform how you'd expect for zooming in. Ctrl + = is standard, but QKeySequence::ZoomIn expects Ctrl+Shift + =. Also no QKeySequence for Alt + = and Alt + -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, ok so QKeySequence::ZoomIn doesn't use Ctrl+= on English keyboards...? That's annoying. Check out how Wireshark handles the situation:
|
#7873 may be related, at least for the note duplication shortcut. I am curious, reading the code, it appears that I am also interested what the use-cases are for vertically flipping a selection of notes. Given how scales are usually not evenly spaced, flipping a set of notes would easily create some off-scale notes. Actually, maybe that would be kind of cool sometimes. The key bind zooming sounds cool for keyboard users. I think we have something similar with a mouse shortcut, but having a purely keyboard one is probably a good idea too. |
|
some thoughts:
|
Great ideas. |
Yes, duplicate notes does act like shift dragging. |
Ctrl+0 and alt+0 are already used for setting quantization and note durations, maybe this can be revised later
|
Flipping a melody vertically turns major into minor and vice versa, it's pretty cool that it even works |
|
I'd almost want to request horizontal flipping; I find myself doing that fairly often |
| const NoteVector selectedNotes = getSelectedNotes(); | ||
| const auto& notes = selectedNotes.empty() ? m_midiClip->notes() : selectedNotes; | ||
|
|
||
| m_midiClip->flipNotes(notes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that having Clip::flipNotes is neat, but I don't like that you can pass notes from any clip to it (yeah reverseNotes does it too). I'd rather see that the selection happens inside Clip::flipNotes. It's not a deal breaker though... I need a second opinion on this...
src/gui/editors/PianoRoll.cpp
Outdated
| m_midiClip->flipNotes(notes); | ||
|
|
||
| update(); | ||
| getGUI()->songEditor()->update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these update calls needed? Try without. At least the first one should not be needed since the editor connects to Clip::dataChanged
src/tracks/MidiClip.cpp
Outdated
| int min = notes.at(0)->key(); | ||
| int max = notes.at(0)->key(); | ||
|
|
||
| for (auto note : notes) | ||
| { | ||
| int key = note->key(); | ||
| if (key > max) { max = key; } | ||
| if (key < min) { min = key; } | ||
| } | ||
|
|
||
| int sum = min + max; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int min = notes.at(0)->key(); | |
| int max = notes.at(0)->key(); | |
| for (auto note : notes) | |
| { | |
| int key = note->key(); | |
| if (key > max) { max = key; } | |
| if (key < min) { min = key; } | |
| } | |
| int sum = min + max; | |
| auto bounds = boundsForNotes(notes) | |
| int sum = bounds->lowest + bounds->highest; |
src/gui/editors/PianoRoll.cpp
Outdated
| } | ||
| } | ||
|
|
||
| void PianoRoll::duplicateNotes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like regulus said, there's no need for duplicate since #7873 does that better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, duplicateNotes should be removed in place for #7873
#7941 (just nagging people) |
|
I removed note duplication in favor of #7873 (which should really be merged btw). Keyboard zooming still needs work, but should be a feature in my opinion. Vertical flipping is cool, I think it has a place, nice for easily changing from major to minor. |
allejok96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw you're using spaces for indentation. You should be using tabs only
| } | ||
|
|
||
| rearrangeAllNotes(); | ||
| emit dataChanged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| emit dataChanged(); | |
| emit dataChanged(); | |
| Engine::getSong()->setModified(); |
| int value = m_zoomingXModel.value(); | ||
| m_zoomingXModel.setValue(value + 1); | ||
| } | ||
| if (ke->modifiers() & Qt::AltModifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be else if otherwise ctrl+alt will zoom both

Note duplication
Ctrl-D
duplication.mp4
Note flipping
flipping.mp4
Key bind zooming
Ctrl- and Ctrl=
zooming.mp4