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

Inspector offset property: fix editing x and y value individually when multiple elements are selected with potentially different values for x and/or y #15772

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cbjeukendrup
Copy link
Contributor

@cbjeukendrup cbjeukendrup commented Jan 8, 2023

Resolves: #15452
Res0lves: #15752 (already fixed meanwhile)

To fix issue #15452, we need to do two things:

  1. Create separate property items for x and y (and do something ugly to make resetting and applying to style work correctly)
  2. Create a special setter function that work per element, so that if you set the x value, for each element individually the y value will be preserved.

So we need to add another customisation point to buildPropertyItem.

We can also see that different properties need different kinds of customisation possibilities:

  • some need nothing
  • some only need to specify a simple element-agnostic function to convert from UI value to internal PropertyValue
  • some need to specify an element-specific function to convert from UI value to internal PropertyValue; in those cases we need to force the caller to specify a way to convert to a style value too, which solves #15752.
  • some need to specify a fully custom setter function; often they only need to call some extra method after calling the default behaviour, so we add a convenient way to retrieve the default function.

In this PR, I added all these ways. I also eliminated an unnecessary conversion from QVariant to QVariant, by allowing to specify a direct conversion between PropertyValue and QVariant.

Necessary for "compound" properties, where we use multiple PropertyItems for what's under the hood a single property. In this situation, when setting the value for one part of the property, the other parts of the property need to be preserved _for each element individually_, because the other parts might differ per element. Therefore, we introduce the possibility to inject a custom function to convert from QVariant to PropertyValue per element.
In 6ded409, we had made offset properties a single PropertyItem to fix some problems with applying to style. This turns out to cause other problems, because we want to be able to edit x and y values separately, also when multiple items are selected, see musescore#15452.
@cbjeukendrup cbjeukendrup marked this pull request as draft November 5, 2023 00:47
@cbjeukendrup
Copy link
Contributor Author

Finally rebased this dreadful PR. After looking at it again, I think it's not that dreadful at all; it may look a bit complicated, but it isn't really. Also, the complicatedness is mostly confined to AbstractInspectorModel; any attempt to simplify that, would result in some awkwardness elsewhere, so it's a bit of a trade-off.
However, there are still some refinements to be made, so I'm marking it as draft.

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.

[MU4 Issue] X and Y offset values cannot be managed separately when multiple items are selected
2 participants