fix #301259: wrong text offset for non-default text styles #5947

MarcSabatella · 2020-04-16T00:08:32Z

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

See also #5728

The problem occurs in several different overrides to
getPropertyStyle() for various different text classes.
The function is supposed to determine
which Sid to use for Pid::OFFSET, based on placement above/below.
But that calculation should only be relevant
if the element is using the default text style for its type.
Otherwise it should use the offset in the current text style.

This code removes the overrides for getPropertyStyle() in each class,
instead modifying TextBase::getPropertyStyle() to check
if the element is using its default text style or not,
and then only if so does it use the placement to select a Sid.
Otherwise it uses the offset of the current text style.

Resolves: https://musescore.org/en/node/301259 See also musescore#5728 The problem occurs in several different overrides to getPropertyStyle() for various different text classes. The function is supposed to determine which Sid to use for Pid::OFFSET, based on placement above/below. But that calculation should only be relevant if the element is using the default text style for its type. Otherwise it should use the offset in the current text style. This code removes the overrides for getPropertyStyle() in each class, instead modifying TextBase::getPropertyStyle() to check if the element is using its default text style or not, and then only if so does it use the placement to select a Sid. Otherwise it uses the offset of the current text style.

MarcSabatella · 2020-04-16T00:10:27Z

I'm ambivalent about which approach I like better - this one or the one I started with, #5728. Once I realized I saw the switch statement in offsetTid() here, I started thinking, oh, I know, I'll make this a new member function I can override in the subclasses. Then I realized, I'm essentially back where I started.

Anyhow, either PR should work the same.

Spire42 · 2020-04-16T00:28:58Z

I haven't looked at this code yet, but FWIW, I almost always prefer a virtual function to a switch statement, as it results in much better encapsulation.

ecstrema · 2020-04-16T03:54:19Z

But the switch statement is more compact, and seems clearer to me.

It's basically the difference between an element that handles all its capacities, and a capacity that handles all elements. The first option is very clear when there is not too many elements, while the latter scales better.

njvdberg · 2020-04-16T05:49:11Z

I also prefer virtual function to a switch, it make it make clear which part of the code is responsible. Too often we see a mix of this responsibility which make the code difficult to understand.

MarcSabatella · 2020-04-16T16:04:15Z

I guess I also generally like to see virtual functions with overrides as opposed to a switch for things like this. But what bothered me about the original PR #5728 was the extent to which the existing virtual function getPropertyStyle() was really doing almost exactly the same thing in each of its overrides, and this is what suggested to me there was opportunity for improvement.

I think the best solution here is a rather more general one: I think instead of (or perhaps in addition to) a single Pid::OFFSET, there should be OFFSET_ABOVE and OFFSET_BELOW. Then the ElementStyle data for each of the relevant data types can associate these with the appropriate Sid. This would potentially apply to more than text - other elements like trills, hairpins, fermatas etc. have separate above/below position settings that have similar overrides for getPropertyStyle() to disambiguate which Sid to use for Pid::OFFSET based on placement. This would all be totally unnecessary if we had separate properties - all we'd need to do is associate the above/below styles and properties in ElementStyle.

But, frankly, I don't have the stomach for making a change like that. It affects more code paths than I care to think about, plus it opens up a can of worms about whether we then expose both properties in the Inspector, and whether we should actually implement the separate above/below positions for other elements too.

So my inclination right now is to actually recommend we go with my original PR #5728. Keeping the override for getPropertyStyle() is completely consistent with how it is handled for trills and other non-text elements with separate above/below position settings, so it no longer bothers me the way it did at first. And then if someone does want to take a look at adding new properties, it might be that much simpler to sort out what yo do since all the existing code is consistent.

I am leaving this PR open because really I don't care that much which approach gets chosen - I'm happy to leave that decision to @anatoly-os .

dmitrio95 · 2020-04-16T19:00:00Z

Virtual functions are a better way for a logic which depends on object's type, but here we have exactly the same logic which depends on whether Tid for this object has been changed. In this case, in my opinion, having multiple getPropertyStyle function overrides only leads to duplicating code without real benefit to code readability or maintainability. In this case only the "whether Tid has been changed" part needs to use virtual functions mechanism, and this is what defaultTid calculation tries to achieve here. Mapping of Tid to Sid for placement does not depend on object type at all, so making this a separate function is, in my opinion, a good thing to do (although I would rather make it a static member of TextBase class as it logically belongs to TextBase).

Therefore I personally would prefer to recommend this PR with some small adjustments over #5728.

dmitrio95 · 2020-04-16T19:02:28Z

libmscore/textbase.cpp

+//   offsetTid
+//---------------------------------------------------------
+
+Sid offsetTid(Tid tid, bool placeAbove)


To sum up what I have said above, this could probably be moved to TextBase class as a static member as it deals with text-related issues only...

If offsetTid() is to become a method of class TextBase, why make it static? Making it an object method would eliminate the need to pass tid and placeAbove as parameters, since these are properties of TextBase elements.

Also, wouldn't it make more sense to name this function offsetSid, since it returns an Sid and not a Tid? That was the name I originally gave it, and I do not understand why it was changed.

Weird, I would have sworn I just copied/pasted your code. But I guess I just took the body of the function and retyped the declaration, and missed it by that much. I have to admit, I got a certain kick out of having written in the original PR - in all seriousness - "we need to check Tid before mapping Pid to Sid." But yeah, it's easy to lose track of what's what.

Anyhow, the reason to make this a static member - if I'm not confusing myself further - is as per the comment from @dmitrio95 : the function really is more about Tid than element type.
This distinction was more or less the reservation I had with your original code. To be honest, my head is spinning a little.

Anyhow, I'm going to push an update shortly, I'll try to make sure it makes sense.

I do not see how that is a reason to not make it an object method. I am not talking about overriding virtual functions; I am talking about having the method use the object's own tid() and placeAbove() values instead of having to pass them as parameters.

dmitrio95 · 2020-04-16T19:05:28Z

libmscore/textbase.cpp

+            Tid defaultTid = Tid(propertyDefault(Pid::SUB_STYLE).toInt());
+            if (tid() == defaultTid) {


... and this calculation is exactly what needs to use virtual functions mechanism here, but propertyDefault() seems to do this job well (and it is indeed a virtual function properly overridden in TextBase subclasses).

MarcSabatella · 2020-04-16T22:34:57Z

Virtual functions are a better way for a logic which depends on object's type, but here we have exactly the same logic which depends on whether Tid for this object has been changed. In this case, in my opinion, having multiple getPropertyStyle function overrides only leads to duplicating code without real benefit to code readability or maintainability. In this case only the "whether Tid has been changed" part needs to use virtual functions mechanism, and this is what defaultTid calculation tries to achieve here. Mapping of Tid to Sid for placement does not depend on object type at all, so making this a separate function is, in my opinion, a good thing to do (although I would rather make it a static member of TextBase class as it logically belongs to TextBase).

Therefore I personally would prefer to recommend this PR with some small adjustments over #5728.

It took me a minute to understand the distinction you are making, but I get it - the conversion in question doesn't depend on element type (which would be a good candidate for a virtual function) buyt on text style. The confusion between these, of course, being the whole reason for the bug, and also something I wasn't comfortable with in the code suggested by @mattmcclinch .

I'll need to think further about this, I guess. Simply adding this function as a static member is harmless enough of course, so I'll do that as soon as I can.

mattmcclinch · 2020-04-17T02:28:34Z

libmscore/textbase.cpp

@@ -2476,12 +2476,49 @@ int TextBase::getPropertyFlagsIdx(Pid id) const
      return -1;
      }

+//---------------------------------------------------------
+//   offsetTid


Don't you mean offsetSid? 😉

yes! too much wine...

So that's why your head is spinning!

MarcSabatella · 2020-04-17T02:30:22Z

Pushed a commit to make offsetSid (thanks for the correction, @mattmcclinch!) into a member of TextBase. I made it non-static, so that this function could do the check against the default Tid. It works equally well to have it static and move that check back to getPropertyStyle().

At this point to me it's really six of one, a half dozen of the other.

MarcSabatella changed the title ~~fix #301259~~ fix #301259: wrong text offset for non-default text styles Apr 16, 2020

MarcSabatella mentioned this pull request Apr 16, 2020

fix #301259: wrong offset used for non-default text styles #5728

Closed

dmitrio95 reviewed Apr 16, 2020

View reviewed changes

MarcSabatella mentioned this pull request Apr 16, 2020

MuseScore 3.5 Alpha PRs to be merged #5960

Closed

make offsetSid a member function of TextBase

d240284

mattmcclinch reviewed Apr 17, 2020

View reviewed changes

fix typo in comment

b4844d0

anatoly-os merged commit e29406d into musescore:master Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #301259: wrong text offset for non-default text styles #5947

fix #301259: wrong text offset for non-default text styles #5947

MarcSabatella commented Apr 16, 2020

MarcSabatella commented Apr 16, 2020

Spire42 commented Apr 16, 2020

ecstrema commented Apr 16, 2020 •

edited

Loading

njvdberg commented Apr 16, 2020

MarcSabatella commented Apr 16, 2020

dmitrio95 commented Apr 16, 2020 •

edited

Loading

dmitrio95 Apr 16, 2020

mattmcclinch Apr 16, 2020

mattmcclinch Apr 16, 2020

MarcSabatella Apr 17, 2020

mattmcclinch Apr 17, 2020

dmitrio95 Apr 16, 2020

MarcSabatella commented Apr 16, 2020

mattmcclinch Apr 17, 2020

MarcSabatella Apr 17, 2020

mattmcclinch Apr 17, 2020

MarcSabatella commented Apr 17, 2020

		Tid defaultTid = Tid(propertyDefault(Pid::SUB_STYLE).toInt());
		if (tid() == defaultTid) {

fix #301259: wrong text offset for non-default text styles #5947

fix #301259: wrong text offset for non-default text styles #5947

Conversation

MarcSabatella commented Apr 16, 2020

MarcSabatella commented Apr 16, 2020

Spire42 commented Apr 16, 2020

ecstrema commented Apr 16, 2020 • edited Loading

njvdberg commented Apr 16, 2020

MarcSabatella commented Apr 16, 2020

dmitrio95 commented Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcSabatella commented Apr 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcSabatella commented Apr 17, 2020

ecstrema commented Apr 16, 2020 •

edited

Loading

dmitrio95 commented Apr 16, 2020 •

edited

Loading