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

fix #196771 : Regroup Rhythms deletes various elements #3552

Merged
merged 1 commit into from Mar 21, 2018

Conversation

@AntonioBL
Copy link
Contributor

commented Mar 17, 2018

No description provided.

@AntonioBL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

I basically cloned the function setNoteRest using chord clone, so that articulations, lyrics, pitch selling etc are preserved.
I have some doubts regarding the handling of slurs.
The effect of this PR on real time entry should be tested.

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

Travis doesn't like it? See https://travis-ci.org/musescore/MuseScore/jobs/354833260#L5330, it is in tst_rhythmicGrouping, about added endSpanner tags

@AntonioBL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2018

From what I see, the tests were wrong: if I insert a tie from scratch, it appends an "endSpanner" tag to the note on which it ends.
I am now looking into each single test to see if the endSpanner is in the right place.
I will try to add further tests for articulations.

@AntonioBL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2018

Now the tests should be ok. I will squash the commits if Travis gives a green light on the tests.

Actually, there are still problems in regrouping inside tuplets (but it was the same also in the previous implementation), and the one about slurs, but it should work in the other cases.

N.B: Because of this bug https://musescore.org/en/node/270433 in some cases regrouping when an articulation is present on the first note will give a crash (related to Chord::removeMarkings not behaving properly at the moment in 3.0-dev).

@AntonioBL AntonioBL force-pushed the AntonioBL:regrouprhythms branch from 258a483 to a067c06 Mar 18, 2018

@lasconic

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

@shoogle if you are around and have a minute (or an hour...) could you have a look ?

@shoogle

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

@lasconic, it looks good to me in master, but don't merge just yet as I need to test against the 2.2 branch because the code has diverged.

@shoogle

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

@AntonioBL, nice job! This seems to work fine for me with real-time input.

I submitted a new PR (#3565) which needs to be merged before this one. @AntonioBL, you will need to rebase your PR once mine is merged, but it is quite easy. See my comment on the other PR.

@lasconic

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

I merged @shoogle's PR.

@AntonioBL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

@shoogle : Thank you for the update.
I see in your comment you suggest replacing the conflicting parts of code with those of this PR: do you think the change in chord.cpp for overlapping voices, i.e. this line
ChordRest* nextCR = nextSeg->nextChordRest(track(), backwards);
is still necessary? From what I see, the problem of overlapping voices should be solved in the updated master with that do-while loop.

@shoogle

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

@AntonioBL, I added the do while loop before I was aware of that nextChordRest function exists. Since the function exists I think we might as well use it.

@AntonioBL AntonioBL force-pushed the AntonioBL:regrouprhythms branch from a067c06 to 5c165ec Mar 21, 2018

@AntonioBL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

Ok. Rebased the PR on the updated master.

@lasconic

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

Thank you guys !

@lasconic lasconic merged commit 2803c7d into musescore:master Mar 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AntonioBL AntonioBL deleted the AntonioBL:regrouprhythms branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.