#1555 Various failures when cycling dots and undoing it

[Yves Guillemot]

Very likely the following issues are various sides of the same bug.

Issue 1 :

1 - In notation editor, enter a quarter at the end of a bar
2 - Select it and press "Ctrl + ." (or the "Cycle dots" icon)
A eighth tied to the quarter is created at the beginning of the next bar
3 - Press "Ctrl + z" (or the undo icon)
The tie is removed, but the eighth note is still there

Issue 2 :

1 - In notation editor, enter a quarter at the end of a bar
2 - Select it and press "Ctrl + ." (or the "Cycle dots" icon)
A eighth tied to the quarter is created at the beginning of the next bar
3 - Press "Ctrl + ." again
A doted eighth is superimposed to the already existing eighth

Issue 3 :

1 - In notation editor, enter a quarter at the beginning of a bar
A quarter rest follows the note
2 - Select it and press "Ctrl + ." (or the "Cycle dots icon")
The note is doted and a eighth rest replaces the quarter rest
3 - Press "Ctrl + z" (or the undo icon)
The dot is removed, but the eighth rest is keeped

Etc...

[D. Michael McIntyre]

Two thoughts... First, the cycle dots command should probably check for available space and fail if the extended note would extend beyond the next bar. That's easier than generating new events and tracking them for the undo operation. Second, I wonder what happens if the cycle dots runs a normalize rests at the end, especially on an undo. That might be a tidy solution, but it might also mess up explicit rests someone has drawn, so it would be necessary to do some testing and see how it all shakes out.

[Philip Leishman]

I think my changes to BasicCommand pretty much fixed the undo redo issues here (issues 1 and 3).
That still leaves issue 2 - I think the problem is because tied notes are selected. Would a solution here be to ignore tied notes for the add dot command ?
Ideas ? Suggestions ?

[Yves Guillemot]

Thanks Philip,
I totally forgot I opened this ticket more than three years ago.

I confirm that issues 1 and 3 are fixed.

About issue 2, cycling dot on tied notes is a bit ambiguous. Should the dot be applied to both notes together or to the last one only ?
If a dot is add to both notes and the result is normalized, we get two quarters and a sixteenth tied and not any dot. That's not exactly as simple as should be "cycling dots".
To apply the dot only to the last note is not the right thing to do as the two notes are selected together.
So to do nothing when notes are tied is probably the better thing to do from the point of view of an user.

[Yves Guillemot]

I just tried to cycle dot on two tied quarters.
I got two untied dotted quarters.
This confirm my opinion that it's better to do nothing when notes are tied.

[Yves Guillemot]

BTW it explains what happens in issue 2: The first "Ctrl + ." creates tied notes and the second one remove the tie before being applied to each of the notes.

[Philip Leishman]

Right - that all sounds very logical. I have done a quick fix - no add dot for tied notes. See the merge request.
When testing this I got some random crashes.
Try creating a segment and adding several notes in the notation editor.
Still in the notation editor undo the additions.
I get a crash sooner or later.
I think it may be something to do with the new code around slotLookAtModifiers.

[Ted Felix]

Crash confirmed. Backtrace:

#0  0x00007fef93b38209 in Rosegarden::operator< (a=..., b=...)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/base/ViewElement.cpp:44
#1  0x00007fef93ab6329 in Rosegarden::ViewElementComparator::operator() (
    this=0x55c5fc6eaba8, e1=0x7fef8400b4e0, e2=0x55c5fcbcf1e0)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/base/ViewElement.h:97
#2  0x00007fef93ab67bb in std::_Rb_tree<Rosegarden::ViewElement*, Rosegarden::ViewElement*, std::_Identity<Rosegarden::ViewElement*>, Rosegarden::ViewElementComparator, std::allocator<Rosegarden::ViewElement*> >::equal_range (
    this=0x55c5fc6eaba8, __k=@0x7ffc6c048b30: 0x55c5fcbcf1e0)
    at /usr/include/c++/9/bits/stl_tree.h:1997
#3  0x00007fef93ab650c in std::multiset<Rosegarden::ViewElement*, Rosegarden::ViewElementComparator, std::allocator<Rosegarden::ViewElement*> >::equal_range (
    this=0x55c5fc6eaba8, __x=@0x7ffc6c048b30: 0x55c5fcbcf1e0)
    at /usr/include/c++/9/bits/stl_multiset.h:880
#4  0x00007fef93b38756 in Rosegarden::ViewElementList::findSingle (
    this=0x55c5fc6eaba0, el=0x55c5fcbcf1e0)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/base/ViewElement.cpp:135
#5  0x00007fef9384e4f9 in Rosegarden::NoteRestInserter::computeLocationAndPreview (this=0x55c5fc668360, e=0x55c5fc668488, play=false)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/editors/notation/NoteRestInserter.cpp:628
#6  0x00007fef9384c877 in Rosegarden::NoteRestInserter::handleMouseMove (
    this=0x55c5fc668360, e=0x55c5fc668488)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/editors/notation/NoteRestInserter.cpp:301
#7  0x00007fef9384c9b5 in Rosegarden::NoteRestInserter::slotLookAtModifiers (
    this=0x55c5fc668360)
    at /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/editors/notation/NoteRestInserter.cpp:327
#8  0x00007fef9385257b in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Rosegarden::NoteRestInserter::*)()>::call(void (Rosegarden::NoteRestInserter::*)(), Rosegarden::NoteRestInserter*, void**) (
    f=(void (Rosegarden::NoteRestInserter::*)(Rosegarden::NoteRestInserter * const)) 0x7fef9384c8fc <Rosegarden::NoteRestInserter::slotLookAtModifiers()>, o=0x55c5fc668360, arg=0x7ffc6c048ec0)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:152
#9  0x00007fef938524c4 in QtPrivate::FunctionPointer<void (Rosegarden::NoteRestInserter::*)()>::call<QtPrivate::List<>, void>(void (Rosegarden::NoteRestInserter::*)(), Rosegarden::NoteRestInserter*, void**) (
    f=(void (Rosegarden::NoteRestInserter::*)(Rosegarden::NoteRestInserter * const)) 0x7fef9384c8fc <Rosegarden::NoteRestInserter::slotLookAtModifiers()>, o=0x55c5fc668360, arg=0x7ffc6c048ec0)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:185

[Yves Guillemot]

I confirm the crash and its relation to the fix of ticket 1597.
I'll look at it ASAP.

[Ted Felix]

Merged Philip's fix for issue #2 as [6734a9]. Please test latest git.

[Yves Guillemot]

Crash related to 1597 should now be fixed.

I confirm the fix for issue 2.

There is something strange about this fix: if you try to cycle dots on tied notes the command do nothing but is registered in the undo list.
So if you tie two quarter then press five times "Ctrl + .", nothing happens but you have to press Ctrl-Z six times to remove the tie.

This is not really harmful nor unusual as the same thing happens if yo try, for example, to add beam to notes already beamed or remove beam to notes already unbeamed.
So the better is probably to leave it as it is.

[Philip Leishman]

Yes. You are right - that is annoying. But I know of no way for a command which decides it has nothing to do can remove itself from the command history.

[Philip Leishman]

But if NotationView could recognize this situation and disable the command .....

OK. Can you merge again ?

[Ted Felix]

Looks good. Merged in [313e08].

I am seeing another issue. The keyboard shortcuts in the pop-up menus are cut off to the right for me. I'll do some digging and see when this started.

[Philip Leishman]

Yes I see that too.
Most likely one of my qt6 changes :(

[Philip Leishman]

OK. I broke the ThornStyle :(
Fix is on the feature-qt6-preparation branch

[Ted Felix]

Looks great. Thanks. Pushed as [0db4a1].

[Philip Leishman]

One last fix - I extended the normalizeRests call to the end of the bar. This makes the dot removal look better.
Please merge again.

[Ted Felix]

Looks good. Thanks. Pushed as [0130e2].