Add: Accessibility Panel - Score Style Preset option

[nasehim7]

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 Accessibility Panel
• Add Score style preset dropdown

Backend Changes:

• Add Score style preset dropdown functionality code in the backend

Open Issues:
None

Enhancements:

1. Update the "Reset" button with the "Three dot menu" option

---

• 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)

[shoogle]

This Score Style Preset dropdown works well for me, along with the reset button!

I think the three-dots menu with "Apply to all parts" is a nice-to-have, so lets try to get this merged with just the Score Style Preset dropdown and the reset button.

---

If I make changes in Format > Style then the Score Style Preset dropdown changes to 'Custom'. That's good, but if I use this button in the Format > Style dialog...

...then the dropdown doesn't go back to 'Default'. That's a minor nitpick though. I wouldn't try to fix that at this stage.

---

Please make the changes I requested, then move everything to do with notehead scheme (i.e. shapes) into a separate PR. The new PR should contain the commit from this PR plus a new commit for the new things.

When the "MSN PR" (this one) gets merged, you'll be able to drop the first commit from the "scheme PR" and rebase it on master.

Colours should go in another new PR after that.

[nasehim7]

@shoogle Thanks for the review! I have updated this PR. Working on creating the other PR. Cheers!

[shoogle]

This file contains #include <QColor>, which causes the CI_Without_Qt check to fail.

That's a clue that this is the wrong approach here. You shouldn't need to use Qt functionality in the engraving module.

One solution might be to return an MStyle object directly, instead of returning a path to a style file. That would enable you to call DefaultStyle::defaultStyle().

Otherwise, you'll need another way to get the file path to the default style.

[shoogle]

Probably your getStyleFilePath() function should behave more like DefaultStyle::resolveStyleDefaults().

The first time DefaultStyle::resolveStyleDefaults() is called with a certain argument, it creates an MStyle object from a style file. If it gets called again in future with the same argument, it just returns the same MStyle object it created on the first call. So the actual style files only get accessed once.

[cbjeukendrup]

Another possibility is to use IEngravingConfiguation instead of INotationConfiguration. IEngravingConfiguration namely also has a defaultStyleFilePath method. (In fact, the one from INotationConfiguration uses IEngravingConfiguration.)

In general, you shouldn't really ever include notation files in the engraving module. This is to avoid circular dependencies. Instead, the dependencies between modules should be seen as a hierarchy, and engraving is near the top of that hierarchy, meaning that it should not depend on most modules.

[nasehim7]

I see, yes that makes sense. Let me try it out

[cbjeukendrup]

PropertyItem* properties are supposed to be constant:

Suggested change

	Q_PROPERTY(PropertyItem * scoreStylePreset READ scoreStylePreset WRITE setScoreStylePreset NOTIFY scoreStylePresetChanged)
	Q_PROPERTY(PropertyItem * scoreStylePreset READ scoreStylePreset CONSTANT)

Why? The PropertyItem* is not the value of the property, but an object that has own properties for the value, default value, etc.
So, the properties of that object can change and can be modified from QML, so those have NOTIFY and WRITE functions. But that object itself stays constant.

[cbjeukendrup]

After looking through the whole PR, I have some slightly more fundamental comments. The main message: PropertyItem* and everything around it is a high-level utility framework that works well for standard properties, but Pid::SCORE_STYLE_PRESET is far from a standard property, so PropertyItem* is not suitable here.

[cbjeukendrup]

I'm starting to doubt more and more whether this should be a "property" at all.

The first question is: of what EngravingObject is it a property? Surely not of any individual element, but perhaps of a Score or MasterScore. But these classes don't work with properties at all currently, and thus their properties are also not written to a file.

Instead of with properties, Score and MasterScore work with "style" items. So, should this option be a style option? Perhaps, but also not really: that results in the weird situation that one special style item determines which style file needs to be loaded, and that causes all other style items (and if you don't watch out, even that special item itself) to be overridden.

Judging from how you are currently using this property, namely only in ScoreAccessibilitySettingsModel, it should be neither a property, nor a style item, because its value isn't stored anywhere anyway.

[shoogle]

Just noticed my reply was still 'pending'. Looks like I forgot to click the 'submit' button!

---

I don't think we were fully clear about what we were expecting here, partly because we weren't completely sure ourselves.

For the MVP, the style preset dropdown can be purely a UI concept; it doesn't have to exist in the score model/file at all. When the dropdown value changes, a new style would be loaded like with Format > Load Style. MuseScore would instantly "forget" that the settings came from a file rather than manually via Format > Style.

However, I think the eventual aim is to save the name of the MSN style in the score, probably as a style rather than a property, or maybe even as an attribute on the <Style> element:

<Score>
    <Style preset="16mm MSN">
        <titleFontFace>Arial</titleFontFace>
        <Spatium>4</Spatium>
        <!-- more style settings -->
    </Style>
</Score>

If this attribute is present, that does not mean we will ignore the individual style settings in the score and load the specified MSN style file again and again whenever the score is opened.

Instead, the individual style settings will take priority for engraving purposes. The preset attribute would only be used to set the UI dropdown to the correct value when the score is opened.

Once we know what value to use in the dropdown, we can potentially do something more clever with it in a future version. For example, if the user clicks "Reset all styles to default", we could reset to the specified MSN default rather than to the global default. Or we can show a dialog like this:

Load the new 16mm MSN style?
This score was created in MuseScore Studio 4.5 using an old version of the 16mm MSN style. Would you like to load the new version of this style?
[ Yes ]          [ No ]

However, anything beyond simply storing the preset name and using it to determine the dropdown value is outside the scope of this GSoC project.

[nasehim7]

Thanks @shoogle @cbjeukendrup for providing the insights. I just updated the PR, will further refine it as we continue this week

[cbjeukendrup]

Because the Pid::SCORE_STYLE_PRESET property is not a normal property of an element, the usage of PropertyItem* is not appropriate here. Instead, a simple dedicated getter and setter method should be created.

[cbjeukendrup]

The unusual use of PropertyItem* also leads to a very complicated code flow here. So, when the user clicks an option in the dropdown, this code is called...

[cbjeukendrup]

...which leads to here...

[cbjeukendrup]

...and then this signal...

[cbjeukendrup]

...causes this handler to be called, which updates the model of the dropdown.

In total, that's a very indirect way of updating the model of the dropdown.

A better way would be to make possibleScoreStylePresets a Q_PROPERTY with a NOTIFY signal, instead of a Q_INVOKABLE. When doing so, you need to drop the () here in QML, and the Connections object should be removed too. All you need to do is emitting the NOTIFY signal from C++ at the appropriate moment.

[nasehim7]

Yes Casper makes sense :D . Thank you for providing such a thorough insight, helpful ✨

[shoogle]

When I create a new score, the dropdown says "18mm MSN" rather than "Default". It works properly after I select a different value though.

It's not saying "(edited)" for me yet, but perhaps that's something you're still working on?

[nasehim7]

@shoogle Yes Peter! currently I am adding the "edited" capability in the code. Thanks for the information, let me check and update

[nasehim7]

Need to fix: When an edited option is on the dropdown and the score is saved & restored - the dropdown currently shows the regular preset value and not the edited preset. For example - 18mm MSN (edited) when saved and restored comes to be 18mm MSN

[nasehim7]

Updated the implementation to handle the edited case in the dropdown (reading and saving). Currently the preset is marked edited for any change done in the style settings

[shoogle]

If you want to save the edited status, you should do this in a separate attribute:

<Style preset="16mm MSN" edited="true"/>

Combining them as "16mm MSN (edited)" creates a microsyntax within XML that's extra work to read and write.

[shoogle]

It would be better to check the current style values against the MSN style file to see if the current values were edited, but storing the "edited" attribute is an acceptable workaround for now.

[shoogle]

This function is not used in the QML so it should be private and take an enum as argument rather than an int.

[shoogle]

Use #pragma once instead of include guards in all new files.

[shoogle]

@cbjeukendrup, what do you think of moving this enum to types.h as enum ScoreStylePreset?

That way it could have userName(), translatedUserName() and maybe toXml() fromXml() functions in typesconv.h.

[cbjeukendrup]

That could be a good idea indeed.
(Perhaps in the further future it won't be an enum anymore but for example a string or preset filename; but for now this enum, with accompanying TConv stuff as far as necessary, is fine.)

[nasehim7]

So we port the enum?

[shoogle]

@nasehim7, yes, please move it to types.h as enum ScoreStylePreset and define userName() and translatedUserName() functions for it.

You can then use the translatedUserName() function in ScoreAccessibilitySettingsModel::possibleScoreStylePreset(), when you create the QVariantList.

Instead of writing the QVariantList out by hand, you can iterate over the enum (you'll need a dummy ScoreStylePreset::MAX_PRESET value to stop iteration). For each enum value, you can get its text from the translatedUserName() function and insert it into the list. You can also test to see whether it is the current preset and is edited. If so, you can insert the %1 (edited) value into the list straightaway. This is better than doing it at the end because it saves having to make space for it in the list by shuffling all the other items down a place.

[nasehim7]

Okay sure Peter! let me do that and update the PR

[nasehim7]

@shoogle Updated the PR with the suggested approach

[shoogle]

This should be double layer now:

QVariantList presets = {
        QVariantMap {
            { text, muse::qtrc("inspector", "Default") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::DEFAULT) },
                    { edited, false }
                }
            },
        },
        QVariantMap {
            { text, muse::qtrc("inspector", "16mm MSN") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::MSN_16MM) },
                    { edited, false }
                }
            },
        },
        QVariantMap {
            { text, muse::qtrc("inspector", "18mm MSN") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::MSN_18MM) },
                    { edited, false }
                }
            },
        },

[shoogle]

    MStyle style = globalContext()->currentNotation()->elements()->msScore()->score()->style();

    if (!style.stylePresetEdited()) {
        return presets;
    }

    int currentPresetValue = static_cast<int>(style.stylePreset());

    int i = 1;
    for (const QVariant& variant : presets) {
        QVariantMap outer = variant.toMap();
        QVariantMap inner = outer.value(value).toMap();
        int presetValue = inner.value(preset).toInt();
        if (presetValue == currentPresetValue) {
            inner[edited] = true;
            outer[value] = inner;
            outer[text] = muse::qtrc("inspector", "%1 (edited)").arg(outer.value(text).toString());
            presets.insert(i, outer);
            break;
        }
        ++i;
    }

    return presets;

[shoogle]

            onActivated: function(index, value) {
                scoreStylePreset.currentIndex = index
                if (!value.edited) {
                    root.model.loadStyle(value.preset)
                }
            }

[shoogle]

I'd still set QString text = "text", QString value = "value", etc. outside the loop, then use them here inside the loop, like this:

presets.append(QVariantMap {
    { text, presetName },
    {
        value,
        QVariantMap {
            { preset, i },
            { edited, false }
        }
    }
});

Then I would write out the edited one explicitly like this as well.

Previously I asked you to recycle the map object, but that's because you had to unpack it anyway to get the string and the integer value. That's not the case anymore, so you might as well create a new one.

[shoogle]

This function should return the value directly rather than an index to the value.

return QVariantMap {
    { "preset", style.stylePreset() },
    { "edited", style.stylePresetEdited() }
}

Then in ScoreAccessibilityInspectorView.qml you can do:

scoreStylePreset.currentIndex = indexOfValue(root.model.scoreStylePreset())

[nasehim7]

@shoogle I tried to do this but came across a finding that there is strict check in the indexOfValue() method. So as per JS Docs If both operands are objects, return true only if they refer to the same object. which is not the case here due to the fact that even though two objects have the same values, they are considered different objects in memory unless they are the exact same reference so it was going back to -1 index. I thought of multiple solutions but the one where we pass the model to the scoreStylePreset() sounded fair. Ideally, this method should now be called like getScoreStylePresetIndex() or something which would look like

QVariantMap ScoreAccessibilitySettingsModel::getScoreStylePresetIndex(const QVariantList& presets) const
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->style();
    int currentPresetValue = static_cast<int>(style.stylePreset());

    QString text = "text";
    QString value = "value";
    QString preset = "preset";
    QString edited = "edited";

    for (const QVariant& p : presets) {
        QVariantMap presetMap = p.toMap();
        QVariantMap valueMap = presetMap.value(value).toMap();
        int presetValue = valueMap.value(preset).toInt();
        bool editedValue = valueMap.value(edited).toBool();

        if (presetValue == currentPresetValue && editedValue == style.stylePresetEdited()) {
            return p.toMap().value(value).toMap();
        }
    }
    
    return presets.first().toMap().value(value).toMap(); // or we could do: return QVariantMap();
}

or Am I missing something? I checked when I do JSON.stringify() in the JS file, it works

[cbjeukendrup]

I think it's okay for this method to return int, but would rename it to currentScoreStylePresetIndex. However the implementation can be cleaned up; casting to int and then checking every possible int value is not great. Either rewrite it as a switch, without a cast to int; or as an arithmetic expression, i.e. return isAccessibilityStyleEdited ? presetIndex + 1 : presetIndex. Personally I'd prefer the latter in this case.

[shoogle]

Ahh, good catch! I actually had a go at this myself and wondered why it wasn't working... 🤪

There are three options here:

1. Make the check non-strict (i.e. use == instead of ===).
   • Qt Creator will display a warning if you do this, so you'd need to leave a comment there so that nobody "fixes" it later on.
2. Unpack the possibleScoreStylePresets() list and return the actual object rather than an equivalent one.
   • I don't think this would work though, because you'd get a different (albeit equivalent) object each time you call the function.
   • If it happens to always give the same object on your machine, that might be a compiler optimization that wouldn't necessarily work with other compilers or with other compiler settings.
3. Return an index instead of a value.

Your suggested code is a strange mix of 2 and 3. You rename the function to include "index", but then you return an object (i.e. a value) from the list.

Perhaps it is best to return an index after all. If so, this is the way to do it:

int ScoreAccessibilitySettingsModel::scoreStylePresetIndex() const
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->style();
    int currentPresetValue = static_cast<int>(style.stylePreset());
    if (style.stylePresetEdited()) {
        return currentPresetValue + 1;
    }
    return currentPresetValue;
}

This works because when you looped over the enum values to create the QVariantList in possibleScoreStylePresets(), you already assumed that the first enum value is 0 and it increments by 1 each time.

If that assumption is true (which we know it is from types.h) then it follows that the index in the list is equal to the enum value. If it's edited you just need to add 1 to that value.

The presence of the edited value in the list throws all the later indexes off by one. But the neat thing is that the edited one is removed from the list as soon as it is no longer the current one, so this function never needs to return an index "after" the edited one.

[shoogle]

TLDR: Pick option 3 and use that code snippet. It will be faster than the other options because it avoids iterating over the list. You can use your knowledge of the enum to jump straight to the correct index.

[shoogle]

Missing 's' at end of function name: possibleScoreStylePresets().

[shoogle]

This function is no longer needed. You can use untranslated the TConv version instead.

[shoogle]

You can use the untranslated TConv function here.

[shoogle]

Under INJECT and above public, you should have these lines:

Q_PROPERTY(QVariantList possibleScoreStylePresets READ possibleScoreStylePresets NOTIFY possibleScoreStylePresetsChanged);
Q_PROPERTY(int scoreStylePresetIndex READ scoreStylePresetIndex WRITE setScoreStylePresetIndex NOTIFY scoreStylePresetIndexChanged);

This declares two properties that you can use in the QML as:

• model.possibleScoreStylePresets
• model.scoreStylePresetIndex

For this to work, the class also needs to have these functions, as declared in the Q_PROPERTY() macros:

public:
    QVariantList possibleScoreStylePreset() const; // READ: not Q_INVOKABLE now
    int scoreStylePresetIndex() const;             // READ: not Q_INVOKABLE now
    int setScoreStylePresetIndex(int index);       // WRITE: replaces loadStyle(int preset)

signals:
    void possibleScoreStylePresetsChanged(); // NOTIFY: replaces accessibilityStyleEditedChanged()
    void scoreStylePresetIndexChanged();     // NOTIFY

Note: There's no WRITE function for possibleScoreStylePreset because it's a read-only property.

Now, assuming you're emitting (with emit) those signals in the right places, if you set this in the QML:

StyledDropdown {
    id: scoreStylePreset
    currentIndex: model.scoreStylePresetIndex
    // Etc.
}

Qt will use the READ and NOTIFY functions specified in the Q_PROPERTY() macro to make sure that scoreStylePreset.currentIndex always stays in sync with model.scoreStylePresetIndex.

[nasehim7]

Ah yes makes sense and it is much more cleaner now. Thanks Peter and Casper for the inputs :D Just updated the PR with the changes

[shoogle]

onActivated: function(index, value) {
    root.model.scoreStylePresetIndex = index // internally, Qt will call the WRITE function
                                             // specified within the Q_PROPERTY() macro.
                                             // i.e. setScoreStylePresetIndex(index)
    if (!value.edited) {
        root.model.loadStyle(value.preset)
    }
}

You could remove the call to root.model.loadStyle(value.preset) and do that inside setScoreStylePresetIndex(index) instead. But it's fine to do it here as long as you don't do it in both places.

[cbjeukendrup]

I think it might be preferable to call loadStyle from C++ indeed. If you only change the index, but don't call loadStyle, things are in an inconsistent state, so you'll never want to update only the index. Therefore, always calling loadStyle from C++ seems good to me.

[nasehim7]

Yeah makes sense. Moved the logic to the setScoreStylePresetIndex(index)

[shoogle]

There should either be three variables here:

ScoreStylePreset m_scoreStylePreset = m_scoreStylePreset::DEFAULT;
bool m_scoreStylePresetEdited = false;
bool m_ignoreStyleChange = false;

Or there should just be m_ignoreStyleChange.

If you're caching the edited value, you might as well cache the stylePreset value as well.

If you have these values cached, you might as well use them everywhere in the .cpp instead of reading from the style each time.

[nasehim7]

I see. I have updated the code with the caching approach

[shoogle]

Is this function still needed? If so, you should change it as follows:

void ScoreAccessibilitySettingsModel::updateScoreStylePreset()
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->score()->style();
    ScoreStylePreset stylePreset = style.stylePreset();
    bool stylePresetEdited = style.stylePresetEdited();
    bool indexChanged = false;

    if (m_scoreStylePresetEdited != stylePresetEdited) {
        m_scoreStylePresetEdited = stylePresetEdited;
        emit possibleScoreStylePresetsChanged();
        indexChanged = true;
    }

    if (m_scoreStylePreset != stylePreset) {
        m_scoreStylePreset = stylePreset;
        indexChanged = true;
    }

    if (indexChanged) {
        emit scoreStylePresetIndexChanged();
    }
}

Your setScoreStylePresetIndex(int index) should look a little bit like this, though not exactly the same.

[nasehim7]

I think it's needed as when the style file is read, the same values need to be propagated to the scoreaccessibilitysettingsmodel member variables (m_scoreStylePresetEdited, m_scoreStylePreset). This should happen when the UI component load is completed so under Component.OnCompleted: {}.

[shoogle]

Here you're supposed to use the supplied int index to determine whether the preset enum and/or edited value should change. If so, you should update those values in the style (as well as your cached versions if caching) and then emit the appropriate signals.

[nasehim7]

Yes, that makes sense. I thought about it like this:

As we know, the member variables m_scoreStylePreset and m_scoreStylePresetEdited are caching the latest ScoreStylePreset and whether it is an edited version or not. We can use this information to determine if there has been a change. When m_scoreStylePresetEdited is false, it means there are 6 values in the dropdown; otherwise, there are 7 values, with the edited option always appearing just below the corresponding standard option. So it should look like the following:

void ScoreAccessibilitySettingsModel::setScoreStylePresetIndex(int index)
{
    bool isEdited = false;
    ScoreStylePreset selectedPreset;

    if (m_scoreStylePresetEdited) {
        if (index == static_cast<int>(m_scoreStylePreset) + 1) {
            isEdited = true;
        }
        selectedPreset = static_cast<ScoreStylePreset>(index - (isEdited ? 1 : 0));
    } else {
        selectedPreset = static_cast<ScoreStylePreset>(index);
    }

    bool presetChanged = (selectedPreset != m_scoreStylePreset);
    bool editedChanged = (isEdited != m_scoreStylePresetEdited);

    if (presetChanged || editedChanged) {
        m_scoreStylePreset = selectedPreset;
        m_scoreStylePresetEdited = isEdited;

        if (!isEdited) {
            loadStyle(static_cast<int>(m_scoreStylePreset));
        }

        emit possibleScoreStylePresetsChanged();
        emit scoreStylePresetIndexChanged();
    }
}

I think the signal possibleScoreStylePresetsChanged() should ideally be emitted when there is a selection from edited version to standard and not from standard to standard but yeah for simplicity, I have kept it like this for now. Let me know if this is appropriate, Peter! I just updated the PR :)

[shoogle]

To save yourself the hassle of fixing all the unit tests, you can avoid storing the preset if it's the default one.

Also, you don't want to call .translated() on these. It's best to stick to the source language for semantic text, otherwise you have to handle every possible language in your read functions. You should use (i.e. create) a TConv::toXml() function. It will return AsciiStringView rather than TranslatableString, so you can't accidently call .translated() on it.

[nasehim7]

I see, updated it in the latest push. Ran the tests locally and it was working fine. Thanks for the suggestion :)

[nasehim7]

I added the logic to skip saving the default preset like following:

if (presetEdited()) {
        if (preset() != ScoreStylePreset::DEFAULT) {
            xml.startElement("Style", {
                { "preset", TConv::toXml(preset()) },
                { "edited", String(u"true") }
            });
        } else {
            xml.startElement("Style", {
                { "edited", String(u"true") }
            });
        }
    } else if (preset() != ScoreStylePreset::DEFAULT) {
        xml.startElement("Style", {
            { "preset", TConv::toXml(preset()) }
        });
    } else {
        xml.startElement("Style", {});
    }

Currently, If the score style preset is default but is edited, at the time of saving, I am only adding the edited tag only to the XML. At the time of reading, if the preset tag is not present, the code sets the preset to default.

[shoogle]

Better to remove this function and create a fromXml() function in typesconv.cpp instead.

[nasehim7]

Added

[shoogle]

I don't think you need readStylePreset() or readStylePresetEdited()

m_preset = TConv::fromXml(e.attribute("preset", String(u"Default")), ScoreStylePreset::DEFAULT);
m_presetEdited = e.attribute("edited", String(u"false")) == "true";

[nasehim7]

Yes sure Peter! Acknowledged in the latest push

[shoogle]

ScoreStylePreset m_preset = ScoreStylePreset::DEFAULT;
bool m_presetEdited = false;

You don't need to include "style" in the name of members of the Style class. It's like having Tom.tomsName() instead of just Tom.name().

[nasehim7]

Yes makes sense. Acknowledged in the latest push

[shoogle]

Translator comment above the default one:

//: Score notation style: Default

And above each of the others:

//: Score notation style: Modified Stave Notation (MSN) with 16mm staff size. Intended for visually-impaired musicians.

[nasehim7]

Added

[shoogle]

return static_cast<int>(m_scoreStylePreset) + (m_scoreStylePresetEdited ? 1 : 0);

[nasehim7]

Ah this is much simpler and cleaner. Thanks!

[shoogle]

if (!m_ignoreStyleChange && !m_scoreStylePresetEdited)

[shoogle]

On second thoughts, it may not be possible to cache these after all. I was thinking you would use onNotify to detect when the style changes then update these accordingly, but I don't think notifications are sent when the style is updated programmatically, only when it's updated via the GUI.

[shoogle]

I guess you can leave it how it is for now since you're the only person who is changing these values.

[shoogle]

You can do something like this:

Attributes attributes; // might need namespace before Attributes

if (preset() != ScoreStylePreset::DEFAULT) {
    attributes.push_back({ "preset", TConv::toXml(preset()) });
}

if (presetEdited()) {
    attributes.push_back({ "edited", String(u"true") });
}

xml.startElement("Style", attributes);

If you're using Qt Creator, you can click on .startElement( and press F2 to go to the function definition (or right click > Follow symbol under cursor). Seeing the definition will tell you about the types it will accept. From there you can get the namespace if needed.

[shoogle]

Thanks. Now you can remove m_ignoreStyleChange = false; from the end of the function too. DEFER will work regardless of how the function exits. Only a crash could prevent it (but then the final line wouldn't be reached either).

[shoogle]

You can do this in the .h file since it's a trivial one-liner. That will improve performance since callers #include the header directly, so they can see the function definition at compile time and no longer need to look it up at runtime.

Doing that with a complicated function would slow compilation of files that include the header, but for a trivial function it makes no difference. The files would have to be recompiled each time the function is changed, but we're unlikely to change trivial functions like this very often.

[shoogle]

Can do this in the header too.

[shoogle]

Since the type name is mentioned elsewhere on the line, you can also do:

auto m_preset = ScoreStylePreset::DEFAULT;

But writing it explicitly is fine too.

[shoogle]

Please swap these last two lines so it becomes:

m_scoreStylePreset = selectedPreset;
style.setPreset(selectedPreset);
loadStyle(selectedPreset);

Every time you update the cached value, we want to see the real value updated immediately afterwards.

[shoogle]

This also works:

loadStyle(selectedPreset);
m_scoreStylePreset = selectedPreset;
style.setPreset(selectedPreset);

Or:

loadStyle(selectedPreset);
style.setPreset(selectedPreset);
m_scoreStylePreset = selectedPreset;

What matters is style.setPreset(selectedPreset); and m_scoreStylePreset = selectedPreset; are on neighbouring lines.

[nasehim7]

I see, yeah makes sense. I was going forward with the first one. Thanks Peter!

[shoogle]

You emit scoreStylePresetIndexChanged() twice in this function. That should never happen.

Emitting will cause connected slots to fire (i.e. it will make something happen in the UI), which could potentially be costly in terms of performance. You don't want to emit things more often than you need to.

Normally, it would be better to add a separate conditional block at the end of the function:

if (presetChanged || editedChanged) {
    emit scoreStylePresetIndexChanged();
}

Except in this case you already know that index != oldIndex, so you can do this outside of a conditional block:

emit scoreStylePresetIndexChanged();

That should be the final line of this function.

[shoogle]

bool editedChanged = m_scoreStylePresetEdited; // because index != oldIndex