[MU3] fix #303422: lyrics color is preserved when importing mxml + small bugfix and cleanup for isEven #6021

SKefalidis · 2020-05-02T10:34:38Z

Relates to: https://musescore.org/en/node/303422

I removed the _even variable which does not do anything other than creating a bug where there are lyrics that are neither odd nor even (as far as Lyrics::layout is concerned). Even Layout::isEven uses _no instead of _even.

This further breaks the behavior of the program when importing lyrics (since now no lyrics have the correct color when imported), but I would think that it is a step in the right direction :^).

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

SKefalidis · 2020-05-02T10:36:52Z

As far as the issue is concerned, I see 2 possible solutions. One is removing the call to styleChanged from the Layout method. As far as I can tell, no other class calls styleChanged from its Layout method, and this removal fixes the problem.
Edit: the first possible solution would be a significant regression as @mattmcclinch pointed out.
On the other hand, even with this fix, when the user changes the style of the score, the color of the lyrics gets reset. So the other (probably better solution) is to track down that bug which might solve this too.

Jojo-Schmitz · 2020-05-02T11:25:32Z

Strange enough Lyrics::isEven() uses _no % 1 whereas Lyrics::propertyDefault() and Lyrics::layout() use _no & 1. Should be the same thing, but the latter might be faster, so you might as well change that too, at least god for consistency.

SKefalidis · 2020-05-02T11:25:54Z

The travis failure was expected.

Jojo-Schmitz · 2020-05-02T11:27:33Z

I believe that Travis CI mtest failure is exactly what this change is supposed to fix? See https://travis-ci.org/github/musescore/MuseScore/jobs/682247601#L4340-L4362

The travis failure was expected.

Edit: I suspected that ;-)

So this PR does related to an earlier PR of your's, #5910, right?

SKefalidis · 2020-05-02T11:45:20Z

Yes, it does relate to that PR.
I looked around a bit more and I believe that removing the call to styleChanged is the way to go (I'll test it more and I will upload it). This should fix the related issue.
Also _no % 1 is also a bug if I am not mistaken (it is always equal to 0), so you are right, I'll change that.

Jojo-Schmitz · 2020-05-02T11:48:47Z

Outch, I didn't even notice that it is _no % 1 but read it for _no % 2, which probably was intended there. How could this have ever worked?
Ah, I see, that Lyrics::isEven() is not used anywhere...
Looks like a rather bad thinko or (not quite as bad a) typo from 4944400

Once you fixed Lyrics::isEven(), better use it too in those other places that currently directly work use _no & 1

mattmcclinch · 2020-05-02T13:27:02Z

The purpose of the _even flag is to determine whether or not the parity of the verse number has changed since the last time Lyrics::layout() was called. initTid() should only be called when the lyric changes from even to odd or vice versa. It certainly should not be called every time the lyric is laid out. The _even flag makes sure that it is only called when necessary. I do not see what removing this flag is supposed to fix.

SKefalidis · 2020-05-02T13:34:50Z

@mattmcclinch that is very interesting and useful (and your valuable information helps explain the regression described above). What it fixed is the inconsistent behavior between odd and even lyrics. It looks like the problem lies somewhere else.

Jojo-Schmitz · 2020-05-02T13:40:01Z

How about setNo(int n) not just setting _no, but also _even?
And also in Lyrics::setProperty() and Lyrics::readProperties().

SKefalidis · 2020-05-02T13:46:41Z

My guess is that the problem is that all lyrics are initialized using the first constructor (line 5189 of importmxmlpass2, maybe?), therefore the yare all considered odd. Then layout runs and the even ones become even and they lose their properties because of styleChanged.

Edit: Yes, it's most probably that, I'll push a fix soon (hopefully).

mattmcclinch · 2020-05-02T14:04:32Z

importexport/musicxml/importmxmlpass2.cpp

            }
      else {
            l->setNo(lyricNo);
+            l->setEven(lyricNo % 2);


Really? The % operator?

I would prefer to do the following l->setEven(l->isEven()) but doesn't that look even more confusing? Either way, hopefully, @Jojo-Schmitz s idea of setting _even inside setNo will work flawlessly. That will alleviate the need for setEven.

Why not use l->setEven(isEven(lyricsNo))?
Ah, isEven() doesn't take an argument.
But at least use lyricNo & 1
Still better would be when setNo() also sets odd/even

I believe that setting _even inside setNo() is a very bad idea. Unless you mean you are going to compare it against its previous value and call initTid() and set styleDidChange to true if the values differed. Probably best to just let this all happen in one place.

@mattmcclinch yes, that's what I was thinking to do.
Frankly, I don't love either option. I prefer setters than only set, but at the same time having to call a second function when setting the number of the lyric is asking for trouble.

Sorry, what are the two options that you don't love?

The first one is setting _even in setNo (as you explained it has downsides, and it might even be not feasible). The second one is having to call 2 functions setNo and setEven together.
They are decent, but not quite as clean as I would hope :^).

Ah, so the second option is what you are currently doing. This is, in my opinion, the correct way to go about this. In most cases, you would only have to call setNo() and not bother calling setEven() (because it wouldn't be the right thing to do), but here is a case where you need to call both.

SKefalidis · 2020-05-02T14:04:56Z

Finally, a solution that seems pretty safe and decent (thank you very much @mattmcclinch). I added a comment that explains the use of _even.

@Jojo-Schmitz I like your idea very much, but at first I would like to test if that breaks something else (like the capella import).

SKefalidis · 2020-05-02T21:20:31Z

Added comments, changed % 2 -> & 1. From what I've gathered this is the only place where we want both setNo and setEven, that's why I will leave them as separate functions. If, in the future, the same problem arises for some other import I believe that the comments will help clarify the solution.

Jojo-Schmitz · 2020-05-03T09:39:59Z

libmscore/lyrics.cpp


      bool styleDidChange = false;
-      if ((_no & 1) && !_even) {
+      if (isEven() && !_even) {


Hmm, now this does look confusing. One would need to look up what isEven() does to find that it does not work on _even, but on '_no'
I know I siggested to use isEven(), but I change my mind in the light of how this looks now ;-)

I feel like the real problem is _even. Should I change it to _wasEven?

But then void setEven(bool b) { _even = b; } seems strange

Jojo-Schmitz · 2020-05-03T09:40:06Z

libmscore/lyrics.cpp

            styleDidChange    = true;
            }
-      if (!(_no & 1) && _even) {
+      else if (!isEven() && _even){


Jojo-Schmitz · 2020-05-03T09:40:18Z

libmscore/lyrics.cpp

      switch (id) {
            case Pid::SUB_STYLE:
-                  return int((_no & 1) ? Tid::LYRICS_EVEN : Tid::LYRICS_ODD);
+                  return int(isEven() ? Tid::LYRICS_EVEN : Tid::LYRICS_ODD);


here though it is OK

SKefalidis · 2020-05-03T10:26:28Z

Should I also modify testLyricsColor.xml to cover this case (of multiple colored lines of lyrics)?
(@Jojo-Schmitz )

Jojo-Schmitz · 2020-05-03T11:24:31Z

Should I also modify testLyricsColor.xml to cover this case (of multiple colored lines of lyrics)?

Yes, I think so