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 #293464: Slash style for staff type change: Inspector label and function are not the same #5271

Merged
merged 2 commits into from
Aug 28, 2019

Conversation

Jojo-Schmitz
Copy link
Contributor

No description provided.

@@ -114,7 +114,7 @@
<item row="11" column="0" colspan="2">
<widget class="QCheckBox" name="slashStyle">
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to rename the widget as well? Otherwise, the combination of names looks really weird :)

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Aug 19, 2019

Choose a reason for hiding this comment

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

OK, guess we could...

But maybe better not, slashStyle is used in many many places in the source, apparently for hysterical raisins ;-) See also @MarcSabatella's remark about that in the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

It is indeed the same setting used in Staff Properties, where the widgets themselves are called stemlessPitched and stemlessPercussion but the corresponding member of StaffType class is called slashStyle. For tab, it's set as a side effect of the radio button to control stems. I've never understood why this name existed, but guessed it either had something to do with tab or else it predated our support for ther forms of slash notation, and setting a staff to stemless was the first step in creating slash notation. Maybe even at one time this setting also forced the notes to slash heads. We'd probably have to go back to sourceforge or pick @wschweer or @lasconic memory to know.

Anyhow, I'm fine with a global replace of slashStyle to "stemless", except for compatibility we probably shouldn't change the tag written to the MSCX (or we should write both forms always and accept either on read).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be a rather big change... for IMHO very little benefit

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I said I'm fine with it, but I don't actually recommend it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have the changes for this ready shortly (but no idea what might break along the way), will make it a separate commit, for the decision takers to see ;-)

and all related things along with it, including a compatibility layer
@anatoly-os
Copy link
Contributor

@Jojo-Schmitz could you please describe possible consequences of renaming Pid properties in the commit log? The changes from this PR will look very weird in a year. We need to clearly describe the reason we do what (and why) we do in the commit log.

I like the results of the renaming, btw. I wonder how developers understood the code around that variables before!

@Jojo-Schmitz
Copy link
Contributor Author

For that 2nd commit? Something along the lines of "making names match their function"?

@anatoly-os
Copy link
Contributor

I mean something like "new names might be incorrectly read and written to the file". Or "I left the read/write properties names the same to keep backward compatibility".

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 28, 2019

There is a comment in the code for backward compatibility, twice. On write it writes the old and the new name, on read it reacts in the old and the new name. There's nothing read or written incorrectly, just superfluos/redundant, for post merge versions.

@anatoly-os anatoly-os merged commit 6546b0e into musescore:master Aug 28, 2019
@Jojo-Schmitz Jojo-Schmitz deleted the staff-type-change branch August 28, 2019 10:47
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