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: 303027 - Add System lines #5965

Merged

Conversation

njvdberg
Copy link
Contributor

@njvdberg njvdberg commented Apr 17, 2020

Resolves: https://musescore.org/en/node/303027

This PR add System Text Lines. System Text Lines are like Text Lines but are attached to a system instead of staff. System Text Lines are implemented as an extension of the existing TextLine. In the Inspector the TextLine will show an new button, System Text Line. With this
button a TextLine can changed into a SystemTextLine and vice versa.

To be consistent with the StaffText and SystemText, in the palette both original TextLine and the new SystemTextLine will appear as two items.
To support different styles for both text lines, TextLine now supports two different styles. Depending on the value of systemFlag() the correct style is chosen.

In the MSCX/MSCZ file the new SystemTextFile is saved a TextLine with attribute system=1. A system attribute has the value 0, or is missing, indicates a TextLine.

In MusicXML, SystemTextLine's are exported as normal TextLine's. Also, when a score containing SystemTextLine's is loaded into an older 3.* version, these SystemTextLine's are seen as normal TextLine's.

This PR replaces #5925

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from da40c39 to b1d03c1 Compare April 19, 2020 07:57
@anatoly-os anatoly-os added this to the MuseScore 3.5 Beta milestone Apr 29, 2020
@anatoly-os anatoly-os removed this from the MuseScore 3.5 Beta milestone Jun 22, 2020
@RobFog
Copy link

RobFog commented Jun 22, 2020

Rebase needed.

@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from b1d03c1 to 1b146bd Compare June 23, 2020 13:16
@njvdberg njvdberg changed the base branch from master to 3.x June 23, 2020 13:16
@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from 1b146bd to e0f3bc6 Compare June 23, 2020 13:18
@njvdberg
Copy link
Contributor Author

Rebase done.

@Jojo-Schmitz
Copy link
Contributor

Another rebase needed...

@njvdberg
Copy link
Contributor Author

Rebase done (getting experienced 😉)

@Jojo-Schmitz
Copy link
Contributor

looks like the mtest fail is not your fault, but a network issue

@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from 85911cd to 211dc59 Compare November 12, 2020 12:40
@njvdberg
Copy link
Contributor Author

looks like the mtest fail is not your fault, but a network issue

Push again solved the problem. We be nice if all issues were that easy 😃

sp->append(textLine, QT_TRANSLATE_NOOP("Palette", "Text line"));
sp->append(textLine, QT_TRANSLATE_NOOP("Palette", "Staff Text line"));

TextLine* systemTextLine = new TextLine(gscore, true);
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering where "systemTextLine" will be deleted from heap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nowhere, this is and stays in the palette. Similar as the other elements here.

@@ -1466,9 +1466,15 @@ PalettePanel* MuseScore::newLinesPalettePanel()

TextLine* textLine = new TextLine(gscore);
textLine->setLen(w);
textLine->setBeginText("VII");
Copy link
Member

Choose a reason for hiding this comment

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

Just curious
Why was it here? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

For a Capo/Barre ? I think it should stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a Roman Chord identifier in a trill line doesn't make much sense, does it?

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Nov 17, 2020

Choose a reason for hiding this comment

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

It is not a trill lin, it is after the trill lines...

It is (supposed to be an example for) a Capo line, the Roman number supposed to denote the fret you should place the capo at (here: fret 7), as far as I can tell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I misread the location.
In the old textline (which was on the staff only), there was this text VII. But now we have the same element which can be both be system and non-system but they have their own entry on the palette. So the text VII is changed to Staff to indicate a staff textline. The new system textline will have the text System. The old VII had no real meaning.

libmscore/style.cpp Outdated Show resolved Hide resolved
//---------------------------------------------------------

static const ElementStyle systemTextLineStyle {
// { Sid::systemTextLineSystemFlag, Pid::SYSTEM_FLAG },
Copy link
Member

Choose a reason for hiding this comment

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

Does this really need to be commented out? I thought you used it in several places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flag is set at another place since the textline can now be both system and non-system.

textLine->setEndHookType(HookType::HOOK_90);
sp->append(textLine, QT_TRANSLATE_NOOP("Palette", "Text line"));
sp->append(textLine, QT_TRANSLATE_NOOP("Palette", "Staff Text line"));
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Nov 17, 2020

Choose a reason for hiding this comment

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

Not sure how to name it then though. It certainly is (an example for) a staff based text line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested, this PR includes the same textline twice on the palette, once as a "Staff Text Line" (which was the original Text Line) and a new "System Text Line" which is a few lines lower. Leaving the original name unchanged could cause confusion.

@vpereverzev
Copy link
Member

@njvdberg there are some issues with mtests here

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 1, 2020

The mtest score mtest/libmscore/parts/part-textlines.mscx needs to get adjusted.
And a rebase is needed once again too.

@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from 66231b6 to 8750dfe Compare December 1, 2020 15:48
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 1, 2020

5 vtest 'issues' with trill lines being offset slightly, guess those are expected?
before:
bravura-8-ref
after:
bravura-8-1
diff:
bravura-8-diff

Similar for Gonville, Emmentaler (or Edwin?) and MuseJazz

This PR adds System Text Lines. System Text Lines are like Text Lines but are attached to a
system instead of staff. System Text Lines are implemented as an extention of the existing
TextLine. In the Inspector The TextLine will show an new button, System Text Line. With this
button a TextLine can changed into a SystemTextLine and vice versa.

To be consistent with the StaffText and SystemText, in the palette both original TextLine and
the new SystemTextLine will appear as two items.

To support different styles for both text lines, TextLine now supports two different styles.
Depending on the value of systemFlag() the correct style is chosen.

In the MSCX/MSCZ file the new SystemTextFile is saved a TextLine with attribute "system=1".
If system attribute has the value 0, or is missing, indicates a TextLine.

In MusicXML, SystemTextLine's are exported as normal TextLine's. Also, when a score containing
SystemTextLine's is loaded into an older 3.* version, these SystemTextLine's are seen as
normal TextLine's.
@njvdberg njvdberg force-pushed the issue-303027-textline-systemflag branch from 8750dfe to 8ecbeec Compare December 3, 2020 16:43
@njvdberg
Copy link
Contributor Author

njvdberg commented Dec 3, 2020

These vtest violations are not expected. And even wrong 😉.
The reason was this PR overwrote the style setting Sid::trillMinDistance which was changed in the meantime.

@Jojo-Schmitz
Copy link
Contributor

Ah, good to have them then ;-)

@vpereverzev vpereverzev merged commit a2f4f1b into musescore:3.x Dec 8, 2020
MuseScoreCore::mscoreCore->updateInspector();
return;
}
initStyle();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this? It causes any change to one property to reset all other properties to style defaults.

igorkorsukov added a commit to igorkorsukov/MuseScore that referenced this pull request Feb 8, 2021
vpereverzev pushed a commit that referenced this pull request Feb 8, 2021
@njvdberg njvdberg deleted the issue-303027-textline-systemflag branch March 10, 2021 12:49
lvinken referenced this pull request Feb 22, 2022
Export the volta text to the MusicXML ending element, allowing custom text such as "1-3".

Backport of #10623, plus 2 more mtest fixes
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.

6 participants