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.

Sign up for GitHub

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

Jump to bottom

Fix #18312 - ensure stem direction is updated correctly in tremolo properties pane #23126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −0
Open

Fix #18312 - ensure stem direction is updated correctly in tremolo properties pane #23126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/inspector/models/notation/tremolos/tremolosettingsmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ void TremoloSettingsModel::resetProperties()
m_direction->resetToDefault();
}

void TremoloSettingsModel::onNotationChanged(const PropertyIdSet&, const StyleIdSet&)
{
loadProperties();
}
Copy link
Contributor Author

@wizofaus wizofaus Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NB at least InspectorModelWithVoiceAndPositionOptions has exactly the same implementation, other models tend to check whether the changed property set contains the relevant ID, but it strikes me as premature optimisation...if anything what would seem to make more sense is to refactor all the of the property loading etc. to be handled automatically by AbstractInspectorModel (buildPropertyItem() would need to keep track of the link between property ids and the returned items etc., then loadProperties( ) could be implemented generically for all models)

Copy link
Contributor

Choose a reason for hiding this comment

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

And it's not perfectly done in that model either. You should check if PropertyIdSet contains the required properties

Copy link
Contributor Author

@wizofaus wizofaus Jun 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok so can I propose as a compromise:

a) we add "loadProperties( const PropertyIdSet&, const StyleIdSet&) to AbstractInspectorModel, and the default implementation of onNotationChanged is to call this
b) all models should only have a single loadProperties( ) function (overriding the above), whereby the assumption is that if the sets are empty, then all properties should be loaded.

Because the current duplication of code seems to be highly likely to lead to unexpected behaviour in the future if one version of loadProperties( ) is updated differently from the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid any refactoring within the scope of this PR and only change the implementation of TremoloSettingsModel::onNotationChanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then you're asking me to continue the pattern of code duplication in the other models that we really should be trying to avoid. If you'd prefer to do the refactoring yourself then ok, as that will fix this issue and prevent similar future issues.


PropertyItem* TremoloSettingsModel::style() const
{
return m_style;
Expand Down
2 changes: 2 additions & 0 deletions src/inspector/models/notation/tremolos/tremolosettingsmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ class TremoloSettingsModel : public AbstractInspectorModel
void requestElements() override;
void loadProperties() override;
void resetProperties() override;
void onNotationChanged(const mu::engraving::PropertyIdSet& changedPropertyIdSet,
const mu::engraving::StyleIdSet& changedStyleIdSet) override;

PropertyItem* style() const;
PropertyItem* direction() const;
Expand Down
Loading