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 #141581: Jianpu linked staff display and saving to XML score file #3614

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ericfont
Copy link
Contributor

ericfont commented Apr 6, 2018

This is a rebase byan61's Jianpu PR #2957 from May last year. I had to resolve a few conflicts and made a few small changes just to make it compile and run. I'm simply posting it here so others can look at the design and think about ways it might be redesigned or improved upon.

@ericfont

This comment has been minimized.

Copy link
Owner

ericfont commented on libmscore/chord.cpp in 7be774a Apr 6, 2018

I don't like the fact that chord.cpp has been modified.

The added lines above are very strange because it causes a doubling of notes in the original chord such that in the new jianpu chord there are two new note elements (one regular note and one new jianpu note) for every note in the original. So in the resulting jianpu staff, I see and can select these two elements, when really there should only be one jianpu element (without a regular note):

jianpu-test

I don't know why the code is doing this additional note cloning.

nnote = fac->cloneNote(onote, link);
else
nnote = new Note(*onote, link);
add(nnote);

This comment has been minimized.

@ericfont

ericfont Apr 6, 2018

Contributor

That is also unusual because that JianpuChord class inherits from regular Chord anyway, so that already accounts for one copy of each note of the chord. But then this code adds the notes again, which seems redundant.

@lasconic lasconic referenced this pull request Apr 6, 2018

Closed

141581 jianpu #2957

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Apr 7, 2018

I'm not surprised this broke a lot of tests. There were quite a few changes made in the past year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment