Feature: PianoRoll Razor

[cyber-bridge]

Cut notes into pieces.

• Minimum cut (step) size will depend on set quantification.
• While in razor mode; hold SHIFT while cutting to stay in razor mode. Without SHIFT the razor mode will exit on first cut.
• Press ESC or Right-Mouse to exit razor mode (or click outside of the PianoRoll).

TODO:

• The cut-line (red line) is not very visible, maybe change it to a pixmap so it also can be theme-able?
• I'm not sure if I did the edit mode/action code as how it's expected to be done, but this will come up in the review right?
• Code style

[Spekular]

I won't do further review right now since there's a PR freeze, but I'd prefer if shift wasn't used in this way as it'd be inconsistent with the sample knife tool (#5524)

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.82%2Bgebee510-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.82%2Bgebee51097-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.82%2Bgebee51097-mingw-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12613-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bgebee510-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12613?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://12614-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bgebee51097-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12614?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12615-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bgebee51097-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12615?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "63bd4f1f3587c9f640db7eeaa4006cdc8678054e"}

[cyber-bridge]

That's all right. And sure behaviour may be changed, will look into how the sample knife does it. For consistency I'm also fine with renaming razor to knife.

[ryuukumar]

I've done some testing, and there's only really one thing that stood out to me that could require some changing, behaviour wise:

While holding down shift and using the razor tool, after shift is released, the cursor remains in razor mode until used once more. Say I've held Shift and sliced a note, after leaving the Shift key, I would either need to manually need to move back to draw mode, or slice a note once more before the cursor moves back to draw mode. A preferable behaviour would be to move back to the original mode as soon as the Shift key is released. (This can be achieved by setting up a keyReleaseEvent function.)

I went over the code as well, and on a first glance there are no outstanding issues. There are some tiny style nitpicks that I could suggest to be changed.

Regarding the pixmap, I think we should try to avoid this, because it may not be consistent when the vertical zoom is changed. However, I think at least the color must be theme-able.

I'll leave a comment shortly with those style changes, but apart from that I think the only noteworthy thing is the behaviour change discussed at the beginning.

[IanCaio]

Just something I noticed reading the code a while ago, still didn't manage to really review the code but will do soon, just have to finish other pending ones first.

[IanCaio]

So I did an initial review today (all written in a piece of paper 😆) and there are some things that I believe should be changed before we could merge this. In order of importance:

1. I think the razor action should be extracted to a method instead of being placed inside the mouse event. I have a suggestion on how it could be done and will explain it below.
2. getMouseTickPos looks very confusing and lacks commenting. It also has a misleading name, since from what I understood of the method it returns a pixel position that is later converted again to a time position on the mouse event (I think the pixel position is the one being quantized, which is a bit odd IMO). I'm all for extracting the conversion of mouse positions to TimePos, since it happens in lots of place in the code base. But if this extraction is done I think it should be done globally, maybe even a PR exclusively for that.
3. Replacing NULL with nullptr
4. Rewriting the lambda function with [this] instead of [=]
5. Some minor code style issues

So here's what I had in mind for the extracting the method:

• Create a method called Pattern::splitNotes(NoteVector notes, TimePos pos, bool quantize). It will only receive a single note from the Razor action, but I'm proposing that it receives a note vector as an argument anticipating the continuation of Knife Tool for Sample Clips (Again) #5524 , where patterns will be able to be split as well, so it would be convenient to be able to split lots of notes in the same position.
• The PianoRoll::mousePressEvent would then just calculate the time position based on the mouse position and call Pattern::splitNotes.
• Remove getMouseTickPos altogether.

I've offered in other PRs to write diffs implementing my suggestions so the author could apply them more easily and I could do the same for this one too.

[IanCaio]

I worked on the suggestions I pointed out yesterday, they are ready to be pushed if you think they are good changes. You can see them on the diff of this gist (ignore the deletion of Pattern::getPattern, it showed there because your master branch is a bit outdated and I didn't rebase it before creating the gist).

The logic of the tool remains the same. I extracted the functionality to a method on the Pattern class (that accepts a vector of notes, making the road ready for the splitting of TCOs in the future), I removed the getMouseTickPos method, added a updateRazorPos method and a m_razorTickPos variable to hold the current razor position for both the drawing and the method call. I also made it so CTRL disables the quantization and gives the user a unquantized split.

Let me know if I should push them to the PR!

[cyber-bridge]

@russiankumar thanks for the testing and review! Yes the shift-release thing is a issue, will address this after the changes from @IanCaio; as for the suggested style changes. On the pixmap; that's a valid point. :-)

…

___ @IanCaio: whoa that looks good! I totally agree with these changes, that's thinking ahead. BTW I wasn't aware of that `[=]` is deprecated, thanks for teaching me :-) How would pushing to this PR work? Would you create a PR on my `razor` branch? And should I rebase to upstream master first?

[IanCaio]

@IanCaio: whoa that looks good! I totally agree with these changes, that's thinking ahead. BTW I wasn't aware of that [=] is deprecated, thanks for teaching me :-) How would pushing to this PR work? Would you create a PR on my razor branch? And should I rebase to upstream master first?

Actually, I have expressed myself a bit wrong: [=] is not deprecated, but the implicit capture of *this while using [=] is. Not on the C++ standards we are using, but from C++20 forwards (https://en.cppreference.com/w/cpp/language/lambda#Lambda_capture). It doesn't cause an issue right now, but it's a small change to make it easier to transition to more recent standards later. I discovered this by accident reviewing a PR from Dom 😆

Well, if you didn't uncheck a box that allows maintainers to update the PR when you submitted it I should be able to push directly to the branch on your fork. I'll try to do that later tonight, if it doesn't work I can send you the diff so you can apply it!

[cyber-bridge]

@IanCaio *thumbs-up* Well I can't remember explicitly checking a box on creation of any PR, so it should work then; otherwise no problem either. :-)

[IanCaio]

@IanCaio thumbs-up Well I can't remember explicitly checking a box on creation of any PR, so it should work then; otherwise no problem either. :-)

I just pushed the changes, I did three small changes after that gist: First one was just improving a comment, second one was removing the if (note->selected()) { newNote.setSelected(true); } statement from splitNotes, because since I'm using the copy constructor the m_selected variable is copied as well, so it's unnecessary. Third one was removing && noteUnderMouse() from the mousePressEvent conditional. That avoids us calling the method twice in a row, the only difference in the behavior being that now if you click somewhere that doesn't have a note (without holding Shift) you'll exit Razor mode. I think it's even a good behavior for a faster workflow, might even make it unnecessary to have a ESC shortcut since clicking away is faster.

The behavior that @russiankumar mentioned is still the same, it's not a bug but more of an UX improvement. I think I didn't mind it that much because ESC or the Right mouse click could be used to quit the razor action after releasing shift. But it might be as small of a change as adding this to the keyReleaseEvent:

case Qt::key_Shift:
    if (m_editMode == ModeEditRazor)
    {
        cancelRazorAction();
    }
    break;

But then, if you press and release shift without doing any splitting it will also exit the Razor mode. The only way to avoid this is to have a way of knowing when a split was made, likely through a variable, but then I think this adds a bit too much for a small UX improvement. If the behavior above isn't acceptable I think I'd rather not change it at all, as there are quick ways to exit the Razor mode already.

[ryuukumar]

But then, if you press and release shift without doing any splitting it will also exit the Razor mode.

To be honest, I hadn't really thought about this. Implementing the method you had discussed (using a variable) is certainly doable, but I will leave this to the author whether to implement this or not. As you said, there are other methods to exit, the only reason I brought this up is because it is slightly unintuitive.

Apart from that and the few suggestions I made, I think that'll be it from my side.

[IanCaio]

To be honest, I hadn't really thought about this. Implementing the method you had discussed (using a variable) is certainly doable, but I will leave this to the author whether to implement this or not. As you said, there are other methods to exit, the only reason I brought this up is because it is slightly unintuitive.

I gave it a second thought, think it wouldn't add too much. Maybe a member variable called bool m_firstRazorSplit which would be set to true every time the razor mode is activated and set to false on the mousePressEvent that resulted in a split. Then the keyReleaseEvent would just have to check if (!m_firstRazorSplit) { cancelRazorAction(); }

[cyber-bridge]

@IanCaio that seems to do the job :-)

[IanCaio]

LGTM!

[ryuukumar]

Did some more testing, and it more or less works as expected. LGTM to me!

@Spekular had a concern about inconsistency of the Shift key, has that been taken care of as well?

[cyber-bridge]

***@***.***(https://github.com/Spekular) had a concern about inconsistency of the Shift key, has that been taken care of as well?

No that is still TODO; as is making the cut line color themable, will get to this asap.

[IanCaio]

No that is still TODO; as is making the cut line color themable, will get to this asap.

I think we can merge with the current behavior of Shift, and then go about making it consistent with the Knife Split shift behavior on a separate PR after the reorg (adding it to the #5915 discussion list).

The marker color themeability probably should be added before merge though, it's not a big change after all and it helps not breaking themes during this period where we will be "halted" for the refactor.

[cyber-bridge]

Leaving SHIFT behaviour for now is OK with me, if anyone opposes: please leave a message :-) And what about renaming razor to knife? If we want to do this it's probably better before merge else it will break themes later since css names will also change then.

[IanCaio]

Leaving SHIFT behaviour for now is OK with me, if anyone opposes: please leave a message :-) And what about renaming razor to knife? If we want to do this it's probably better before merge else it will break themes later since css names will also change then.

I would vote to keep it as Razor. I like having separate names since the Knife tool affects TCOs and the Razor affects notes only. I'd even go further that maybe we don't need shift to behave the same on both, since it makes less sense to quantize the note from the right on the split as it makes when cutting samples. But even though I added discussing the shift behavior to the TODO list for after the reorg on the linked discussion.

Last changes adding the color property look good to me, I think you just missed adding it to the classic theme CSS file.

[Spekular]

I really think the name should be the same. Cut and paste is still cut and paste whether you're pasting notes or patterns (or text, or images, or...). Both editors already have draw and erase mode, it's just inconsistent to have "knife" in one editor and "razor" in another. No one is going to be confused as to why the knife tool in the song editor doesn't cut the entire pattern.

[ryuukumar]

I'd even go further that maybe we don't need shift to behave the same on both

I disagree, as it's better to have them consistent in terms of workflow. We had different shortcuts for the same operations in the piano roll and the song editor, and people vouched for them being made into the same, so I see no reason why we would keep these separate.

Regarding the name I'm okay with either, but I would prefer 'Knife'. There isn't really any consensus across DAWs (some call it cut, scissor, razor, knife, slice, etc.) but since we already have a Knife tool, I think it's best to stick to that name, for the reason Spekular mentioned.

[cyber-bridge]

OK I've renamed everything from razor to knife and used the edit_knife image from #5524

I also thought about the SHIFT issue, what I can do is remove the current SHIFT behavior completely and let it stay in knife mode until ESC or Right-Mouse is pressed, or other mode is selected. Staying in knife mode is consistent with the sample knife. It should be easy to do since it will only require removing some code, so it could be done before re-org.

Other thing is the shortcut to enter knife mode, it makes sense to me to change this from SHIFT+R to SHIFT+K.

[IanCaio]

I disagree, as it's better to have them consistent in terms of workflow.

I would argue that the workflow for splitting notes and splitting patterns/samples is inherently different, as they are very usually done with different purposes in mind. That was why I didn't think it was bad to have different names and slightly different behaviors for each tool. But it's two opinions in favor of keeping them consistent and I don't mind that much that it's changed to Knife, so sure.

@cyber-bridge Last changes look good! Just comment here when you push the shortcut + Shift behavior changes.

[cyber-bridge]

Changes made :-)

[IanCaio]

Changes made :-)

Nice, just checked on them, approval stands! All good

[ryuukumar]

Really sorry for the delay. Tested again, didn't find any shortcomings, so approval stands. Merging.