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

Add: Accessibility Panel - Notehead Scheme option #23344

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nasehim7
Copy link
Contributor

@nasehim7 nasehim7 commented Jun 24, 2024

Hi All,
This is the PR related to GSoC '24 - Accessibility Profiles project. To check what is goal for this project, kindly refer to: https://musescore.org/en/user/1088561/blog/2024/05/27/gsoc-2024-project-accessibility-profiles-musescore

Current Status of the PR:

UI Changes:

  • Add Notehead Scheme dropdown

Backend Changes:

  • Add Notehead Scheme dropdown backend code. (WIP)

Open Issues:
None

Enhancements:
None

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@nasehim7 nasehim7 force-pushed the ap_feature_ns branch 4 times, most recently from 487d0e0 to 2eecbb4 Compare July 4, 2024 23:16
@nasehim7
Copy link
Contributor Author

nasehim7 commented Jul 9, 2024

@nasehim7 nasehim7 force-pushed the ap_feature_ns branch 2 times, most recently from 9199d91 to e8d8282 Compare July 9, 2024 01:54
class MSQE_##Name { \
Q_GADGET \
public:
class MSQE_##Name { \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This update has come due to uncrustify run on the file. Need to revert

Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

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

Looking good so far!

@@ -1679,5 +1679,6 @@ const std::array<StyleDef::StyleValue, size_t(Sid::STYLES)> StyleDef::styleValue
{ Sid::dummyMusicalSymbolsScale, "dummyMusicalSymbolsScale", 1.0 },

{ Sid::autoplaceEnabled, "autoplaceEnabled", true },
{ Sid::defaultsVersion, "defaultsVersion", Constants::MSC_VERSION }
{ Sid::defaultsVersion, "defaultsVersion", Constants::MSC_VERSION },
{ Sid::accessibleNoteHead, "accessibleNoteHead", PropertyValue::fromValue(NoteHeadScheme::HEAD_AUTO) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be:

{ Sid::noteHeadScheme, "noteHeadScheme",  PropertyValue::fromValue(NoteHeadScheme::HEAD_AUTO) }

Reason: The style is generally useful; it's not specifically aimed at accessibility, though it happens to be useful for that purpose. People might want to use it for other reasons (e.g. education).

@@ -534,6 +534,7 @@ static const std::vector<Item<NoteHeadScheme> > NOTEHEAD_SCHEMES = {
{ NoteHeadScheme::HEAD_PITCHNAME_GERMAN, "name-pitch-german", muse::TranslatableString("engraving/noteheadscheme", "German pitch names") },
{ NoteHeadScheme::HEAD_SOLFEGE, "solfege-movable", muse::TranslatableString("engraving/noteheadscheme", "Solf\u00e8ge movable Do") }, // &egrave;
{ NoteHeadScheme::HEAD_SOLFEGE_FIXED, "solfege-fixed", muse::TranslatableString("engraving/noteheadscheme", "Solf\u00e8ge fixed Do") }, // &egrave;
{ NoteHeadScheme::HEAD_FIGURENOTES_STAGE_3, "figure-notes-stage-3", muse::TranslatableString("engraving/noteheadscheme", "Figurenotes (stage 3)") },
Copy link
Contributor

Choose a reason for hiding this comment

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

Figurenotes is one word, so it should be: "figurenotes-stage-3"

QVariantList ScoreAccessibilitySettingsModel::possibleAccessibleNoteHeadTypes() const
{
QMap<mu::engraving::NoteHeadScheme, QString> types {
{ mu::engraving::NoteHeadScheme::HEAD_AUTO, muse::qtrc("inspector", "Auto") },
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need "Auto" as an option here anymore.

  • If the scheme is auto for individual notes, it means use the staff setting.
  • If the scheme is auto for the staff, it means use the general style setting.
  • If the general style is auto... there's nothing else to use!

So the default here should be normal and auto shouldn't be an option. This is true in the Accessibility section and also in Format > Style > Notes. In all other places, auto should be the default.

@@ -967,6 +982,7 @@ SymId Note::noteHead() const

Key key = Key::C;
NoteHeadScheme scheme = m_headScheme;
NoteHeadScheme accessibleScheme = style().value(Sid::accessibleNoteHead).value<NoteHeadScheme>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line.

@@ -976,10 +992,15 @@ SymId Note::noteHead() const
}
}
}

if (scheme == NoteHeadScheme::HEAD_AUTO) {
scheme = accessibleScheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this line to:

scheme = style().value(Sid::noteHeadScheme).value<NoteHeadScheme>();

Now you only look up the style value if you really need to.

NoteHeadScheme m_noteHeadScheme = NoteHeadScheme::HEAD_NORMAL;
NoteHeadScheme m_noteHeadScheme = NoteHeadScheme::HEAD_AUTO;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change in default will have to be handled when reading and writing staves in MSCX files. Don't worry about doing that yet, but I'm adding this as a reminder for later.

When it comes time to handle this, my suggestion is as follows:

  • When reading old files (created in MS 4.3 and earlier), if it says normal in the file, read it as auto instead to make sure the general style setting gets used.
  • When reading new files (created in MS 4.4 and later), read the actual value (auto, normal, or whatever it may be) and trust this value.

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

2 participants