fix #97931: system initial barline type lost on reload

[MarcSabatella]

We were writing the value as an int but reading itexpecting a string. Since regular barlines read and write the type as a string, I changed the code here to be consistent with that.

[lasconic]

Replacing writeProperty by xml.tag doesn't really go in the right direction. No way to make it works without doing this?

[MarcSabatella]

One way I can see would be to define a new P_TYPE - perhaps called "BARLINE_STYLE" - and use this as the type for this property. Then we would modify the switch statement in Xml::tag() to convert this to a string much as is done for some of the other property types. but since we don't do this for the regular barline type, I didn't see a reaosn to introduce it now for system barline type.

What's wrong with using xml.tag() directly as I have done? Are you concerned about losing the automatic check against propertyDefault?

[lasconic]

Using xml.tag put one more "sysInitBarLineType" in the wild and by loosing consistency is error prone. After all that one of the goal of using the property system. Let me dig further the issue.

[lasconic]

Saving it as a string and BARLINE_TYPE as a P_TYPE seems the way to go indeed. We can change the read to readProperties probably. If we can reuse it for normal barline, even better !
We should also take care to compatibility by at least not crashing or doing weird stuff if we read an int. You do it?

[MarcSabatella]

OK, that makes sense, and I can do this. I am not sure about changing the code for for normal barlines, though. We currently use different tags for this: "sysInitBarLineType" versus "subtype". And right now, BarLine::write() always writes the "subtype" tag, even for NORMAL. I think we don't normally try to write normal barlines in the first place, but prlbably we do in some special cases, and changing to writeProperty() would mean we no longer write the tag. Maybe it's not an issue, but still, it seems a little bit dangerous tp be messing with that unnecessarily. I'll look further, though.

[MarcSabatella]

1. I think I have added the P_TYPE::BARLINE_TYPE correctly, but I have to say I am more concerned I got something wrong than I was about my original fix.

2. I guess the advantage of using properties in this way is that centralize the places that need to know about the specifics of the tags and their values. But looking at the code, I'm not so impressed with how this works in practice. In getProperty(), most of the properties that are represented internally as enums but written as strings are handled via a bunch of explicit tests for specific literals, and this still needs to be synchronized with the corresponding code in Xml::tag(). For the new BARLINE_TYPE, I decided to leverage the existing code to use the table of names rather than explicit tests against string literals in the body of the code. So in theory, it's a bit safer than the other types, but it still seems kind of messy to me. I guess that's just how it is, though.

3. Regular barlines use P_ID::SUBTYPE for this, which is also shared by other elements as an "int", so I can't immediately convert that to using the new P_TYPE::BARLINE_TYPE. Doing it right would, I think, would mean making a new P_ID::BARLINE_TYPE property (of type P_TYPE::BARLINE_TYPE) and converting both regular barlines and the system initial barline to use this property instead of P_ID::SUBTYPE and P_ID::SYSTEM_INITIAL_BARLINE_TYPE respectively. I guess it could still use the "subtype" tag, which would represent an incompatible change as far as the system initial barline is concerned, but since that doens't work currently anyhow, it would be no real loss. Or we could start using a new tag for this since "subtype" is normally read/written as a string rather than an int, but then we need break compatibility. No doubt this is all possible, but it would require a larger change to the code than I think is warranted right now just to fix this one issue with system initial barlines. Maybe that should wait until we do a version number bump.

4. Because this new property type is only used here, I didn't think it made sense to handle it in Element::readProperties(), but there is no Measure::readProperties(), so I'm still reading the tag literally in Measure::read(), but at least I can use getProperty().

5. I'm not squashing this yet in case after review it is decided to just keep the original simpler change, but of course I can do so once I am sure I've got it right.

[lasconic]

Could you add a comment to explain it used to be written as a int?

[MarcSabatella]

I didn't see this before, slrry. Should I still add the comment?

[lasconic]

I missed it too. Let me add it.

[lasconic]

It's fine for me.

Regarding 3)
I believe normal barline don't use P_ID::SUBTYPE. In the future, they could be converted to P_ID::BARLINE_TYPE (with name == "subtype") of type P_TYPE::BARLINE_TYPE. I don't see how that would raise a compatibility problem. What do I miss?

I agree on 4)
and I believe you can squash.

[MarcSabatella]

Normal barlines do use P_ID::SUBTYPE in at least some places - that is, getProperty(), setProperty(), and propertyDefault() do implement it, and Barline::drop() uses undoChangeProperty() with that parameter. But the read and write are hard-coded just as I had originally done here, and do not use the property.

I believe you are right, there is no compatibility if we create yet another P_ID that re-uses the new P_TYPE I just added. I was thinking of somehow using the same P_ID for both. But even just using the same P_TYPE would allow us to leverage most of the same code, so we should be able to make that change any time we wanted.

Oh, and it's squashed now.