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 #271505: Allow user to set mode along with key signature #3633

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Apr 18, 2018

MusicXML im- and export of all possible key signatur modes.
Make these availble to the UI, via the Key Signature Inspector. (WIP, it doesn't quite work yet, any idea why?)

@marnen
Copy link

marnen commented Apr 18, 2018

Oh awesome. Thanks!

@Jojo-Schmitz Jojo-Schmitz force-pushed the keysigmode branch 2 times, most recently from 38938f1 to a04fa52 Compare April 20, 2018 10:59
// undoSetModeType
//---------------------------------------------------------

void KeySig::undoSetModeType(KeyMode v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why modetype? and not just mode ?

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Apr 27, 2018

Choose a reason for hiding this comment

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

Just because the enum is KEYSIG_MODETYPE. I can change that, no problem

@lasconic
Copy link
Contributor

What part is no working specifically? How do you plan to change the mode of C major ?

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Apr 27, 2018

The dialog in Inspector doesn't work, it doesn't show the mode and doesn't allow to change it

And indeed C-Major / a-minor is a problem. As is open/atonal.
But this problem exists also for courtesy sigs and the ability to switch these on or off

case Pid::KEYSIG_MODETYPE:
if (generated())
return false;
//setModeType(v);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be the reason why it's not set by the inspector maybe ?

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Apr 27, 2018

Choose a reason for hiding this comment

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

It is commented out because it doesn't compile ;-)

no known conversion from 'const QVariant' to 'Ms::KeyMode'

@@ -678,7 +678,8 @@ InspectorKeySig::InspectorKeySig(QWidget* parent)
const std::vector<InspectorItem> iiList = {
{ Pid::LEADING_SPACE, 1, s.leadingSpace, s.resetLeadingSpace },
{ Pid::SHOW_COURTESY, 0, k.showCourtesy, k.resetShowCourtesy },
// { Pid::SHOW_NATURALS, 0, k.showNaturals, k.resetShowNaturals }
//{ Pid::SHOW_NATURALS, 0, k.showNaturals, k.resetShowNaturals },
{ Pid::KEYSIG_MODETYPE,0, k.modeType, k.resetModeType }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not enough to make the combo works. You need to define user data in the combo to match the value of the Keymode enum. If not, MuseScore uses enum value as index void InspectorBase::setValue(const InspectorItem& ii, QVariant val)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this might be it then.

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Apr 28, 2018

Choose a reason for hiding this comment

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

Now at least that menu doesn't start empty, but with "Unknown", still the setting doesn't stick though

case Pid::KEYSIG_MODE:
if (generated())
return false;
//setMode(v); // no known conversion from 'const QVariant' to 'Ms::KeyMode'
Copy link
Contributor

Choose a reason for hiding this comment

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

KeyMode(v.toInt()) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, indeed

@Jojo-Schmitz
Copy link
Contributor Author

Probably case P_TYPE::KEYMODE needs to get added to/dealt with in void XmlWriter::tag()

return "major";
case KeyMode::MINOR:
return "minor";
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

This switch should probably include all the new KeyMode entries too (if this PR is still actual).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 28, 2019

Strange, why do I get merge conflicts on rebase, but none are reported here?
Anyway, got it rebased now (conflicts in ui files are a royal pain :-()

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 28, 2019

Ooops, something went wrong with inspector_keysig.ui...

for MusicXML im- and export of all possible key signatur modes
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 29, 2019

@anatoly-os I believe the "work in progress" label can get removed

@dmitrio95 dmitrio95 removed the work in progress not finished work or not addressed review label Aug 29, 2019
@dmitrio95 dmitrio95 merged commit 5b02dc0 into musescore:master Aug 30, 2019
@Jojo-Schmitz Jojo-Schmitz deleted the keysigmode branch August 30, 2019 10:11
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 28, 2022
follow up to musescore#3633, there this had been forgotten (by me)
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 28, 2022
follow up to musescore#3633, there this had been forgotten (by me)

Backport of musescore#13060
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
follow up to musescore#3633, there this had been forgotten (by me)

Backport of musescore#13060
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.

5 participants