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 #296610 : import from 2.x may create useless "Fig. bass" user text style #5454

Merged
merged 1 commit into from Nov 15, 2019
Merged

Fix #296610 : import from 2.x may create useless "Fig. bass" user text style #5454

merged 1 commit into from Nov 15, 2019

Conversation

mgavioli
Copy link
Contributor

@mgavioli mgavioli commented Nov 4, 2019

See the original issue for details and why this text style is superfluous.

The fix simply ignores the "Figured bass" text style while importing from 2.x; possibly customised F.b. parameters are in any case loaded form the relevant tags of the main <Style> section.

  • 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

@mgavioli
Copy link
Contributor Author

mgavioli commented Nov 5, 2019

No idea why AppVeyor fails; the log is hardly comprehensible: if shows a huge amount of errors all over the place having to do with the keyword constant. It cannot be a syntax error or Travis would fail too.

Anyone knows what is wrong? Could be an AppVeyor problem?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Nov 5, 2019

My guess is that IGNORE conflicts with some Windows specific marco.

It apparently is, windows.h including winbase.h. How about naming it IGNORED_STYLES? It does compile then on Windows, I've tried it.

@mgavioli
Copy link
Contributor Author

mgavioli commented Nov 5, 2019

Thanks @Jojo-Schmitz ! Updated.

EDIT: And it worked!

TEXT_STYLES
// special, no-contents, styles used while importing older scores
TEXT_STYLES, // used for user-defined styles
IGNORED_STYLES // used for styles no longer relevant (mainly Figured bass text style)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be for "Repeat Text" too? ISTR your scores using those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. I am not entirely sure yet that the "RepeatText" does not come from some customisation of mines. And this PR and the issue it refers to are already well defined in title, topic, code, etc. So, maybe another PR when I will be sure it is an MS problem and not mine.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're probably right, it is is separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, MuseScore 2 had 3 different Repeat text styles (Repeat Text Left, Repeat Text Right and Repeat Text), MuseScore 3 has only 2 (Repeat Text Left, Repeat Text Right), so it is not your fault, other that that you customized that 'excess' Repeat Text.
So ignoring it may indeed be the way to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is nowhere to put it, I believe the way is to ignore it. Yes, I customised it, but eventually it was not really usefully, so I totally forgot about it!

Maybe it would be better to drop this PR and push a new one ignoring both?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just add it to this here "while at it also ignore the no longer existing text style 'Repeat Text'", which as far as I can tell is just one additional line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with "Repeat Text" too.

…t style.

See the [original issue](https://musescore.org/en/node/296610) for details and why this uext style is useless.

The fix simply ignores the "Figured bass" text style while importing from 2.x; possibly customised F.b. parameters are in any case loaded form the relevant tags of the main <Style> section.

While at it, this PR also ignores the "Repeat Text" style, which was a generic varinat for "Repeat Text Left" and Repeat Text Right" and no longer exists in ver. 3.x.
@dmitrio95 dmitrio95 merged commit 5f73d26 into musescore:master Nov 15, 2019
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

3 participants