Fix #274839: Add capo setting to staff text

[jthistle]

Refers to: https://musescore.org/en/node/274839.
This adds the functionality to make staff text change the tuning of a staff by some semitones, as if a capo had been attached to the instrument (most likely a guitar). This is useful for guitars, which of course often have to use capos to make playing tunes easier. Before, there was a tedious workaround that allowed this functionality. Now, you can add some staff text, set it to change the capo to a fret of your choice, and change the text to match the capo you have applied. It's a lot easier :)

Here are some screenshots (NOTE: capo numbers are now numbers, not Roman numerals):

Capo staff text in use

Selecting a fret

'No capo' removes a previous capo, returning to normal tuning

The pitch is recognised as being different, so it will indicate when it is out of the range.

[jthistle]

Not sure why the Travis build is failing, but it seems to be working fine for me and appveyor 🤷‍♂️

[lasconic]

Only Travis runs the unit tests. Did you run the mtest locally ?

[lasconic]

Could we store the capo only if the fret is != 0 ? or that wouldn't work ?

[jthistle]

yes, that would work fine, just an oversight on my part :)

[jthistle]

@lasconic Ah, I'm going to be honest, I had no idea mtest existed. I'll run it now, it'll probably crash.

[jthistle]

@lasconic , quick question, how do you go about running mtest locally?

Edit: never mind

[lasconic]

you should be able to run "make" in the build.debug/mtest directory. Then run mtest or each individual tst_**** exe file. You can see how it's done on travis in build/travis/job1_Tests/run_tests.sh

[jthistle]

@lasconic Great, I've done mtest. Out of 33 tests, 25 failed. How am I meant to work out what's making them fail?

[lasconic]

as you can see in the README https://github.com/musescore/MuseScore/blob/master/mtest/README.md most of the tests are comparing files to reference files. In your case, you output a new tag for every single staff text. Since it's a new tag, the reference files don't have it. So the comparisons fail and the tests fail. Fix the output of this new tag, and run the tests again. You should see a lot less failure. From there, study the diffs to check for other problems if needed.

[jthistle]

@lasconic
Thanks, but what do you mean exactly by:

Fix the output of this new tag

edit: never mind, it was just the change you were suggesting before. Now it says it's failing 3 tests, but at the same time it says 0 failed. Hmmm.

edit: Well the Travis build has passed now, so I guess no tests failed then.

[lasconic]

I mean this #3882 (comment)

If you output the tag only when needed, then it will not be needed in all the staff text already existing in tests. So the tests should just pass. Make sense ?

[jthistle]

@lasconic yes, that makes sense thanks, see my previous comment. All tests are passing now, I just have to make one last little change and it should be ready for merge.

[jthistle]

@lasconic All changes made, and passing tests - thanks for your help. A final review maybe?

[anatoly-os]

Why not to make _capoList public variable? This method gives direct access to the private data member which breaks incapsulation. If you use this member to only insert or clear elements, implement these methods in the Staff class like insertCapo and clearCapo and remove this capoList() method.

[jthistle]

@anatoly-os In that case, surely the swingList method also breaks encapsulation? It returns the private _swingList.

edit: and there are loads of other methods that return private variables, just look at staff.h. Although I guess there is the slight difference that swingList and capoList return pointers, but I don't know if that changes anything.

[anatoly-os]

It is the boyscouts principle: "make the code a bit better after your changes". And surely don't make it worse.
Getting access to private data members is bad coding practice. It is better to make data members public.
Anyway, new code entries should improve the code and make it better. IMHO

[jthistle]

Well, that's true, they should. But this is a common thing across the entire MuseScore codebase, I think? Also, is it just the swingList and capoList methods you're talking about, or is every function that returns the value of a private variable (rather than a pointer to one) breaking encapsulation as well?

[anatoly-os]

It is not me personally, but also Meyers and Stroustrup and others. Returning non constant references or pointers to private data members allows changing the state of the class externally. Returning const references or pointers or copies of the objects don't allow changing class state externally, so it is safe.

Regarding codebase, this approach allows writing more code in less time and makes sense if you are only one contributor. Things are changing, so contributing more or less safe and well designed code is a priority.

[jthistle]

Ah, so if I made the swingList and capoList methods both const, then it would be safe? (sorry about this BTW, I'm very new to C++, so there are some concepts I still haven't quite got my head around yet)

[anatoly-os]

Making the method const won't allow you to to call non const insert/clear. You need to remove these methods and implement wrappers instead like this:
void insertCapo(int tick, Capo* capo) { _capoList.insert(tick, capo); }
I'm not sure about the Capo* pointer type, I'm from mobile and cannot quickly find the type of the st->capo() returning value.

[jthistle]

OK, I'm on mobile too so I will do this when I can.

edit: and, as far as I can remember, st->capo() is an int.

[jthistle]

@anatoly-os I've added the wrappers, so that now the private variable can only be directly accessed from within the Staff class. I'm going to leave this as a separate commit, seeing as it deals with a different issue to the one the other commit on this branch is fixing. Let me know if you think it should be squashed instead.

[anatoly-os]

LGTM. @lasconic?

[lasconic]

Just like for swing, it brings the question : do we add something more to staff text that is entirely different semantically than the other properties ? or should we have a SwingText and a CapoText instead ? How many entirely different properties a staff text can tolerate before becoming a mess ?

[jthistle]

@lasconic the solution to that is to add to the default text palette a pre-made swing text that can be easily added without even having to go into staff text settings. As for the argument that the options panel might become messy - yes, it's a possibility. But, how about we consider how messy it is now: not very, with only four tabs. If it turns out that we have to add loads of tabs in the future, then maybe the approach to text that changes playback should be reconsidered, but for now I think it is nowhere near messy enough for separate capo text and swing text elements.

[lasconic]

Sure. I'm not worried about the UI. What I'm talking about is the score format itself. Changing the UI is easy, but if we decide that StaffText with coda tag becomes CodaText then we will need compatibility code, and we need to maintain it forever. So it's a mid/long term decision to use a property instead of a new element type.

[jthistle]

Ah, that's a good point. What you're talking about would require a rewrite of the whole StaffText element to split it into distinct new elements, which might be in order anyway before release of 3.0. But for now, surely, adding one more property can't do any harm?

[MarcSabatella]

To me, the logical place for this isn't as part of staff text but as part of the staff type change element. After all, that's what is actually being implemented - a staff property. So why not just add "capo" to the list of things you can set for that element in the Inspector?

[jthistle]

@MarcSabatella

staff type change element

What do you mean exactly by this? And do you mean that the "capo" feature should be in stave properties dialog?

[MarcSabatella]

3.0 has a new element for changing staff type - see the "S" in a box on the Text palette (Advanced workspace). Currently it seems it can be used for things like changing the scale of the staff and other properties like whether or not it shows barlines, ledger lines, etc. These are all things that are in the global staff properties dialog as well. Under the hood I assume this is implemented by making these tick-dependent staff properties whose initial value is set in staff properties but for which new value can be added using this staff type change element. I can't imagine any reason why capo setting shouldn't work exactly the same way.

Actually, the only question to me is whether we should keep the existing instrument change element or roll that functionality into the staff type change as well, since they are also virtually the same idea. I might also ask that about swing, but that's actually a system-level element normally (although it can be created as staff text).

[jthistle]

That's fair enough, but my argument for attaching the capo property to staff text is that when you change the capo on an instrument, you always use some text to tell the player that the capo has been changed/set. So, it only makes sense to attach it to the text: it seems intuitive that text which tells the player to do something, also does that thing in playback.

[MarcSabatella]

Sure, but to me me that just suggests the staff text change element should allow for text. I assumed it did, since it is on the Text palette (!) but apparently it does not?

EDIT: on the other hand, same argument could be made about the regular channel-changing staff texts - these are actually changing staff properties by tick and in principle could be wrapped into the staff type change. I'd say it makes sense for both channel change and capo, or neither.

[jthistle]

@MarcSabatella

The staff type change element, despite being in the text palette, weirdly doesn't allow for text to be displayed by it. My assumption is that it's there to make purely cosmetic changes (rather than playback ones), which can be seen by looking at its inspector options. This distinguishes it from the staff text, which can make playback changes. So, because of this, I think that the capo feature should remain with the staff text. Thoughts?

[MarcSabatella]

Well, as I said, as long as we currently have this distinction between staff text and staff type change, I would agree capo belongs with staff text. But I do wonder if the distinction really makes sense. Conceptually, they are doing the same thing - temporarily changing some staff properties. Just some of these need actual text and some don't. I think long term they could be merged. But I don't think that's your job here :-)

[jthistle]

Yeah, I just wanted to add this feature to be honest - it took me a while to put together - although I agree that maybe they could be merged in the long term :)

[Jlfiddler]

I just added issue #275757 and proposed fix. It could interact with this change, though I don't think it creates any major problems.

[jthistle]

It's been almost a month now since we last discussed this, and over a month since I last committed (so I've just done a quick rebase with the latest master). Could we agree that this feature is either good to be added, needs changes, or should not be implemented? Sorry, it's just that every day that I don't do anything with it, I forget a little bit more about how my changes work :)

[Jojo-Schmitz]

commit title should read "fix #274839 ..." not "add #274839 ..."

[MarcSabatella]

I agree it would be good to add :-) We can always tweak specifics later.

[anatoly-os]

@jthistle could you please squash the commits?

[jthistle]

Done @anatoly-os

[dmitrio95]

If this gets merged then we could probably add handling this new setting when importing other fileformats (at least Guitar Pro). Of course, not as a part of this pull request, but I think it would be nice to implement this.

[anatoly-os]

@dmitrio95 could you please add a feature request with the mentioned importing functionality?

[dmitrio95]

@anatoly-os Described it briefly in https://musescore.org/en/node/277452

[dmitrio95]

I suppose this might be not the best place to call this. openScore is not called, for example, when opening a score from a command line or a file manager. Wouldn't readScore or some other function be a better place for this?

[MarcSabatella]

I don't see anywhere in the code or comments here where chord symbols are discussed. Was this meant to be used in addition to the existing capo setting for chord symbols in Format / Style?

[jthistle]

@MarcSabatella this was meant to be a way of alternating the playback of notes with a simple bit of staff text. I don't think chord symbols do that.

[MarcSabatella]

@jthistle I'm not following. I wasn't suggesting that chord symbols could do what this staff text does, I'm saying that a capo setting should affect chord symbols just as surely as it does the relationship between fingering and playback pitch. Normally the whole point of capo is to change the chords, and that's what the built-in style setting for capo already does. I had assumed the point of this PR had been to extend that same functionality to work per-staff and be changeable mid-score, which is great. But if it doesn't affect chord symbols, I'm not really understanding the point.

[MarcSabatella]

Tio be clear: see https://musescore.org/en/node/307704. The expectation users - including myself - have is that this capo setting will do for chord symbols the same as what the existing capo style setting always has done: cause the chord symbols to display both the sounding and written chord, as in F7(D7) when using capo 3.

As it is, the situation is now very confusing, with the original capo style setting being about notation but not playback, the "new" capo staff text being about playback but not notation, both setting made in completely different places, and neither setting having any awareness of the other.