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 #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

Conversation

wizofaus
Copy link
Contributor

@wizofaus wizofaus commented Jun 7, 2024

Resolves: #18312

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

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

@wizofaus wizofaus Jun 7, 2024

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

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.

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.

Tremolo stem direction does not update in Properties panel when tremolos are flipped with keyboard shortcut
2 participants