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
Allow brackets to be invisible #16734
Allow brackets to be invisible #16734
Conversation
4d99bb5
to
547d5b4
Compare
Terrific that we're fixing this @mike-spa. I think for now, there would be a practical advantage in conforming to the regular 'rule' about the Properties panel: i.e. the visibility toggle only affects the selected element (in this case, bracket). Just as a side observation, the |
The problem here is that there really is only one instance of the bracket in the score; its recurrences on each system are not explicit, discrete items, nor are they really attached to anything. These 'system header' items (brackets, clefs, key/time sigs, instrument labels) are set at the beginning of the score, and then are just automatically replicated on each system, though subject to certain rules (e.g. we can use short instrument labels after the first system, or hide brackets when they span only one stave, etc). Some of these things can change during the score (via a clef/key/time sig change, or an instrument change), but those changes are explicit at a certain point in the music and then are, again, automatically reflected when the next system(s) are laid out. While it is highly desirable to be able to make these editable on a per-system basis, that requires first figuring out a way to define a particular instance of a bracket as being an explicit instance of itself (rather than automatic), and also considering what happens to any changes made to it if the layout of the score changes - by no means impossible, but rather beyond the ambition of this PR. |
ok this is very helpful additional information @oktophonie. If it is, as you say, highly desirable to be able to make these editable on a per-system basis (I'm sure I agree with you here), then I think it would ultimately be worth figuring out a way to do it. The behaviour I specified in the comment above would probably be my preference from a UX POV, because it involves no additional UI components and, once learnt, "should" be straightforward to adopt. As far as this PR is concerned though, it does solve the immediate problem of non-responsive functionality, which is good. If we merge it, we might just have to manage some questions about why the action affects the score globally (to which we can affirmatively respond that we are working on a solution). |
547d5b4
to
73d250a
Compare
@bkunda thanks for the help, now I know what kind of user interaction we want to be able to achieve eventually. As Simon pointed out, modifying the properties of a single instance of a bracket requires some very careful work under the hood, because it is part of the larger problem of being able to locally override global settings (something that Musescore simply can't do atm). |
Completely agree with this. Thanks @mike-spa! |
73d250a
to
232729e
Compare
28c794e
to
83ad915
Compare
fix utests
83ad915
to
cff8673
Compare
@@ -92,7 +92,7 @@ | |||
<StaffType group="pitched"> | |||
<name>stdNormal</name> | |||
</StaffType> | |||
<bracket type="1" span="2" col="2"/> | |||
<bracket type="1" span="2" col="0" visible="1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if the user opens their old score that does not have this tag for brackets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked to be sure. The "visible" property is read from here after the other properties. Old files don't have the "visible" tag so it will just default to 1 (i.e. visible, as expected).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, you should add a unit test that verifies this. You can add it to your next PR
Resolves: #15741
(partially)
I'm opening this PR mostly as a design question for @Tantacrul or @bkunda, whenever you have a spare minute to look into this. Currently, if you click on an bracket and toggle the "Visible" switch, nothing happens (see related issue). Clearly this is bad.
With this PR, if you toggle the "Visible" switch on a bracket, it will make all instances of that bracket in the score invisible (see for example here: brackets are now invisible also on the second page).
So, the first question is whether this is acceptable and/or sufficiently intuitive as it is. Surely it's better than a switch that doesn't do anything, and codewise it is quite trivial, so I'd be inclined to put this in.
The second question is that, in future, we want the ability to make a single instance of a bracket invisible, so at that point we'd have to decide whether the "Visible" toggle in the property panel refers to a single instance or to all of them, and how to communicate the difference. Again, what I'm doing here is quite trivial codewise so it won't interfere with the future, more complete solution. I just don't want to make shortsighted design choices.
Let me know what you think!