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

Added ripple delete #6692

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added ripple delete #6692

wants to merge 1 commit into from

Conversation

hberg137
Copy link

Pull request for ripple delete issue #6495

Please see the following video for demonstration/explanation.

@superpaik
Copy link
Contributor

When the selection of notes is not "continuous" (Shift+Ctrl+Mouse) the behavior is not consistent. Not sure what the behavior should be. Perhaps treat each "continuous" sub-group individually.

@hberg137
Copy link
Author

@superpaik I was not sure about this behavior either when developing it. I assumed that if a note (or notes) was within the range of the selected notes but not actually selected, as what it sounds like you are alluding to above, the unselected note(s) should not be shifted upon ripple deleting the selected notes. So, sort of ignoring those notes was an intentional choice since this behavior was not specified in the feature request.

I think your suggestion on what to do in this case would make sense, however implementing it may be a bit tricky in trying to detect notes in between the gaps of selected notes. I think I would have to partition the selected notes into different regions, detect the unselected notes between them, and then shift the unselected notes between them based on the length of the partitions. It seems feasible, but again, a bit tricky, especially since I would probably have to restructure some of the code I already wrote.

Do you want to keep the behavior as it currently is or do you want me to try and make this adjustment?

@superpaik
Copy link
Contributor

@hberg137 As I said, not so sure what the best behavior should be. I recognize it's a weird use-case scenario. If it was intended and as you had a discussion already, maybe the current is the best possible behavior.

@allejok96
Copy link
Contributor

if a note (or notes) was within the range of the selected notes but not actually selected it should not be shifted upon ripple deleting the selected notes

No please. This creates very weird results. I see two feasible alternatives...

  1. Delete everything from start to end. You may justify it by the way shift+select works in file managers, or the way shift+delete deletes the whole line in some text editors.

  2. Do nothing and show a warning like glue notes does. Less destruction but could be annoying in a case like this

bild

deleteSelectedNotes();
ke->accept();
break;
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move all this code into deleteSelectedNotes() instead, and just pass it something like move=true ?

if (selectedNotes.empty()) { break; }
TimePos delStartPos = selectedNotes.first()->pos();
TimePos delEndPos = selectedNotes.last()->endPos();
TimePos delEndLen = selectedNotes.last()->length();
Copy link
Contributor

Choose a reason for hiding this comment

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

A prior note may still end after the last note depending on its length. You may find boundsForNotes helpful.

It will still be tricky with drum notes, but I imagine something like this:

area to delete     |xxxxxx|            |xxxxxxxxx|
normal note         ######    ####      ####      ###
drum note              #                    #  

In that case you just need to check if the drum note starts on the end position... I also see that boundsForNotes doesn't handle drum notes well. That needs to be fixed somehow.

delEndPos = rightNotes.first()->pos();
}

shiftPos(rightNotes, delStartPos - delEndPos);
Copy link
Contributor

Choose a reason for hiding this comment

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

You may disable journalling before moving to prevent the double undo

m_midiClip->saveJournallingState(false);
shiftPos(...);
m_midiClip->restoreJournallingState();

@luzpaz
Copy link
Contributor

luzpaz commented Jul 2, 2023

Any progress on this ?

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.

None yet

4 participants