-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix logic in writeProperty()
#29973
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
base: master
Are you sure you want to change the base?
Fix logic in writeProperty()
#29973
Conversation
src/engraving/rw/write/twrite.cpp
Outdated
} | ||
PropertyFlags f = item->propertyFlags(pid); | ||
PropertyValue d = !force && (f != PropertyFlags::STYLED) ? item->propertyDefault(pid) : PropertyValue(); | ||
PropertyValue d = !force && (f != PropertyFlags::NOSTYLE) ? item->propertyDefault(pid) : PropertyValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not match "properties without style are written if different from default value".
Instead, properties without style are now written always, because they are compared to an invalid PropertyValue, rather than the default value.
Since isStyled
is indeed checked at the top of the method, isn't the solution (as far as there is anything to be solved at all) to remove this check completely?
PropertyValue d = !force && (f != PropertyFlags::NOSTYLE) ? item->propertyDefault(pid) : PropertyValue(); | |
PropertyValue d = force ? PropertyValue() : item->propertyDefault(pid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May well be. But why has there been that check in the first place?
Well, as PropertyFlags::STYLED
can't happen, != PropertyFlags::NOSTYLE
means `== PropertyFlags::UNSTYLED``, and that is when they do get writte (now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't it the wrong way around now? With your change, for NOSTYLE we don't check the default, and for UNSTYLED we do check the default.
So, shouldn't it be like this?
PropertyValue d = !force && (f != PropertyFlags::NOSTYLE) ? item->propertyDefault(pid) : PropertyValue(); | |
PropertyValue d = (force || f == PropertyFlags::UNSTYLED) ? PropertyValue() : item->propertyDefault(pid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, most probably... this inverted logic makes me go dizzy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit from the history seems relevant: 7523286
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly. Yet the logic is obviously wrong, that case just can't happen
Indeed 3.x has the same issue, that's how I found out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @jthistle can enlighten us here about the intention of his change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could help, but this was almost 7 years ago. I don't even remember writing this commit. How time flies....
Good luck!!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So back to @cbjeukendrup's simplification, that at least keeps the current behavior
as `isStyled()` has been asked for already and lead to leaving this method, asking it again doesn't make any sense. Reinstating a comment from 3.x reveals that this method is supposed to be working differently, so now it does.
7c85805
to
0ce0347
Compare
as
isStyled()
has been asked for already and lead to leaving this method, asking it again doesn't make any sense. Reinstating a comment from 3.x reveals that this method is supposed to be working differently, so now it does.This would render the
force
parameter unneeded, if insteadsetPropertyFlags(<pid>, PropertyFlags::UNSTYLED)
would be used.I guess the
force
parameter got introduced as a workaround for that bug?