add "a Tempo" in the Tempo palette

[rtbo]

Aims to provide "a Tempo" text mark to reset the tempo after a gradual change. At the moment it is a simple tempo text that defaults to 120 BPM and can be edited to whatever the user wants.

It is already better than present situation, but I'd prefer the item to automatically pick-up the tempo prior to previous gradual change. ~~Don't really know how to achieve this though.~~

• [x] I signed the CLA
• [x] The title of the PR describes the problem it addresses
• [x] Each commit's message describes its purpose and effects, and references the issue it resolves
• [x] If changes are extensive, there is a sequence of easily reviewable commits
• [x] The code in the PR follows the coding rules
• [x] There are no unnecessary changes
• [x] 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)

[randoguyname]

In regards to the comment of automatically picking up the tempo; to the developer who works on this: keep in mind that users will expect that to update automatically by default as well, but also to be able to manually select a tempo. Perhaps by default a checkbox titled "Detect tempo automatically" should be ticked by default, which then updates with changes to the score, and users can uncheck that and do whatever they want.

[rtbo]

I added TEMPO_RESET_PREVIOUS property on TempoText and added corresponding check box in the inspector panel. Whenever it is checked, the tempo spinbox is disabled and previous explicit (or default) tempo is restored. When unchecked, the item behaves just like any other TempoText item.

The checkbox will be there for every TempoText so I chose generic text description ("reset previous tempo"). I'm open to suggestion for better name/description.

TODO:

• [x] add a tooltip on the checkbox
• [ ] add a test
• [x] french translation

[rtbo]

I changed terminology to "restore previous tempo". Here is with tooltip

[rtbo]

I've run tests manually of all functions, but it's not obvious what I could do as automated test. Is it possible to script playback start on a test score and may be check the tempomap? Or is it OK to not include automated test for this?

[rtbo]

Regarding UX, maybe the additional Restore previous tempo check-box isn't very valuable. Isn't it better if the Follow written tempo do what's right when the text is a tempo (case insensitive)? The fact that a tempo is proposed in the tempo palette self-documents the feature in a much better way IMO.

@CombinedAtom @randoguyname @Tantacrul opinions?

[Jojo-Schmitz]

The French translation doesn't belong into this PR. Those are done on Transifex, once the new strings got pushed there.

[Tantacrul]

Regarding UX, maybe the additional Restore previous tempo check-box isn't very valuable. Isn't it better if the Follow written tempo do what's right when the text is a tempo (case insensitive)? The fact that a tempo is proposed in the tempo palette self-documents the feature in a much better way IMO.

@CombinedAtom @randoguyname @Tantacrul opinions?

Just tested this. Nice work!

If you mean that the functionality should read the characters 'a tempo' and only then apply the correct logic, I would advise against that. We have generally agreed to avoid trying to deduce intent from written text due to how unreliable it is - and how it needs to localise if a different language is chosen. I think having a checkbox (enabled by default) that you can disable makes more sense. Sorry if I've misunderstood your question.

I actually like 'Restore previous tempo' setting and I think we should get rid of 'follow written tempo'.

• 'Restore previous tempo' already does what we need
• Turning it off allows the user to specify an exact tempo anyway, so all needs are covered

A few points which I think will help the whole thing feel a little easier to understand:

1. I'd place the 'a tempo' palette in the main section, at the bottom of the list. Don't keep it in 'More'. It's a primary option, I feel.
2. Change the text from 'override written tempo' to simply 'set tempo'.

Then I think we're good! Thanks very much.

I'll review again when you're done.

[cbjeukendrup]

@Tantacrul We can't simply remove the "follow written tempo" functionality, because that also has implications for normal tempo markings (because the new "a tempo" item is in fact just a tempo marking, and therefore it shares its properties widget with normal tempo markings).

So we need to design a widget that would work well with both "normal" tempo markings and "a tempo" markings.

How about something like this?

(OBVIOUSLY, the "metric modulation" option should not be there yet, but I've added it to convey the point of this design)

[Tantacrul]

Can we not just have different options for either thing, since the user won't care about what goes on under the hood? I'd rather leave any discussion of other kinds of tempo markings (other than 'a tempo') out of this.

For example, I definitely do not think an 'a tempo' marking should be as complex as this. If the user wants to set something complex as a metric modulation, they shouldn't be using 'a tempo' to do it, no?

However, a fully-realised tempo marking probably should have these kinds of options.

My point is that we should show or hide certain options based on the outward purpose of the marking, rather than the internal similarity.

[cbjeukendrup]

I'd rather leave any discussion of tempo markings out of this.

Of course I understand that, but unfortunately it isn't that easy.

The problem is that MuseScore has no way of distinguishing an "a tempo" marking from any other tempo marking (with this implementation).

Comparing the text is no option, because who says that the user will call this item "a tempo"? They might also write "Tempo I". So anything about comparing text is not more than a fragile heuristic. The rigorous solution would be to implement the "a tempo" item as a separate class with a separate item type, but I'm not sure whether it is a good idea to create yet another new class in Libmscore. Another option would be to add just a bool isATempo to the Tempo class, but that's not great either. (Conclusion: from a technical POV it's not a very nice thing to implement.)

Beside that, from a user point of view, I wonder whether "a tempo" should get a different treatment at all. If it did, then a strange situation would occur when the user changes their mind and changes the text from "a tempo" to an explicit metronome marking. MuseScore would still think that it is an "a tempo" item, because it was initially created as such, and thus it displays the "a tempo" options, while the user thinks that it is just a tempo item, and wonders why the Inspector displays a different set of options than for any other tempo item. Instead of just changing the text, the user would need to remove the "a tempo" item and add a new normal tempo marking.

So I guess the main question is: would a user intuitively expect that "a tempo" is a different kind of item than any other tempo marking and therefore you can't change one into the other without deleting and re-adding? My personal answer is no, I would expect them to be the same kind of thing, only the effect on the playback tempo is slightly different. But that's just my answer.

[Tantacrul]

I think the case of someone changing 'A tempo' to a metric marking is an unlikely thing to happen - and there are ways we could avoid that confusion (by giving an 'A tempo' object an explicit 'A tempo' name in Properties, for example)

However, let me narrow my issue with UX in this implementation:

• First, for an 'a tempo' marking we have a 'follow written tempo' checkbox that doesn't make much sense, given the context
• Second (and much worse), this solution forces us to place an illogical 'Restore previous tempo' option in all other tempo markings, which I think is a non-starter. This is a classic MS3 solution I want us to avoid in 4. In the image below, 'Restore previous tempo' makes no sense. It serves no purpose in this context.

The illogical extra option is not worth the benefit of an 'a tempo' marking.

I think they have to be separate objects.

[Tantacrul]

Actually, to be a little clearer: I don't necessarily think they need to be different objects. Just that they need different options to appear in the Properties panel. This implementation requires that the comprehensibility of tempo markings take too much of a hit.

As it stands, I think this solution can't be used, unfortunately.

I do appreciate the explanation though @cbjeukendrup.

[cbjeukendrup]

Okay. @rtbo I suggest to implement this in the following way:

• Change the new TEMPO_RESET_PREVIOUS property to determine whether the tempo marking is an "a tempo" marking (rather than whether the marking does indeed use the previous tempo or that the user has customised the playback tempo)
• Instead, use the TEMPO_FOLLOW_TEXT property to determine whether the tempo marking does indeed use the previous tempo or that the user has customised the playback tempo

So:

	TEMPO_RESET_PREVIOUS == false	TEMPO_RESET_PREVIOUS == true
TEMPO_FOLLOW_TEXT == false	This is a normal tempo marking where the user has overridden the playback tempo	This is an "a tempo" marking where the user has overridden the playback tempo
TEMPO_FOLLOW_TEXT == true	This is a normal tempo marking where the playback tempo is determined automatically based on what's written in the text of the marking	This is an "a tempo" marking where the playback tempo is determined automatically by looking at the previous tempo marking

Then, you'll need to create separate Inspector views for "a tempo" and normal markings. To see an example of how you can create different views/models based on the properties of an item, you can start by looking at HairpinLineSettingsModel (and the places where it's used), which differentiates based on whether the selected hairpin is a crescendo or decrescendo.

[rtbo]

I'm not sure to get the UX right.

What options are to be proposed for the "a tempo" mark?

Either the user can override the tempo to a specific one, in such case the same options as regular tempo apply:

• "follow text" check box
• "override tempo" spin button (enabled if "follow text" is unchecked)

Or the user don't have this choice, in which case there is no option to display at all. (FOLLOW_TEXT and RESTORE_PREVIOUS properties are true and can't be changed on such object)

In either case, I would change the "Tempo" section name by "Restore previous tempo (a tempo)" for clarity for the user.

[Tantacrul]

I would have one option

• A checkbox that says 'Set specific tempo' (rather than 'override tempo', which is less clear).
• If you check the box, then it enables a text field where the specific tempo can be set.
• If the box is unchecked, then the 'A tempo' mark just behaves as it should

[rtbo]

Here is what I have now

Ideally I'd like to have the checkbox where the greyed out title is.

[Tantacrul]

OK, given that you have the word 'specific' twice. I think we can simplify this.

I'd change the second title from 'specific tempo' to just 'tempo'.

I get what you are saying about the checkbox but it's no biggy. We never created a checkbox / title component like that before.

[rtbo]

I've rebased the PR on master. Is there anything left preventing the merge?

[cbjeukendrup]

A rebase is needed again...

[Jojo-Schmitz]

It is a completely painless and conflict-free rebase, I guess you could rebase and merge in one step. It does build cleanly on Windows/MSVC too.

I'll submit a new PR (even if only for the tests to run), see #18017

[Jojo-Schmitz]

All tests in #18017 passed, so I guess this here is ready for (rebase and) merge.

[rtbo]

Great! Do I have to squash commits as well?

[bkunda]

Are we supposed to see a different option in Properties for the new "a tempo" object (per this comment above)?

At the moment (on macOS) I am seeing the same options for both regular tempo marks and "a tempo"

https://github.com/musescore/MuseScore/assets/86290556/ad1b4719-c9fe-4beb-8d56-e3ac1be0f034

FWIW I did (as I always do) perform a factory reset, and reset the "Tempo" palette before re-testing. Version number: OS: macOS 13.1, Arch.: x86_64, MuseScore version (64-bit): 4.1.0-231680012, revision: github-musescore-musescore-78be98f

Given that we don't have an "a tempo" object in master though, I assume I haven't made a silly mistake and that I am in fact testing on the correct build!

Aside from this, there is certainly some playback functionality tied to the "a tempo" object I tested. Some issues with it though were that:

• "Follow written tempo" does not recall the first tempo marking assigned to the start of the piece
• Overriding the tempo, then reverting back to "follow written tempo", sees the overridden tempo persist

I made a (apologies) rather lengthy 6min demo video of my test: https://www.dropbox.com/s/16brsjwaky4jphd/Screen%20Recording%202023-06-19%20at%2010.03.27%20pm.mov?dl=0

Unless I've made some mistake in how I've either understood the comments above (especially this most recent one containing a design mockup), or in the version I'm testing, I have to admit that I'm quite reticent about merging this now for 4.1 – particularly since we are now entering a "feature freeze" state and are really trying to focus on stabilisation and regression testing. Anything connected to playback is also especially risky, so we want to make sure the feature is as bulletproof as possible before merging it.

But I am extremely glad this has been worked on – it is a much, much needed feature! I will be happy to ensure it gets prioritised for the next release (4.2) if these issues can be ironed out.

[lgvr123]

As mentioned on the forum, there are various "a tempo" forms.

The traditional "a tempo" (back to the previous tempo), the "Tempo primo" (resume the original (first) tempo), ...

What about labelling the tempo marks (such as in the jumps) and referring to those labels in the "a tempo" to indicate which tempo to reuse ?

[rtbo]

Hi @bkunda , Thanks for the test and feedback. What you observe is obviously not what is intended, but I also reproduce it on my side.

Weirdly, the test file I built before I rebased on master 2 weeks ago still works fine (both properties UI and playback). I have to dig why it is now broken.

Regarding playback, the feature is designed to reset to the last set tempo in the score. So in your test, "a tempo" on bar 16 would reset to 220 BPM, and therefore have no effect. In fact it is effective only after a progressive tempo change, unless "Set specific tempo" option is used.

[rtbo]

It should work now. @bkunda can you repeat your test?

[rtbo]

@lgvr123 I added "tempo primo" in the palette. It is similar to "a tempo" but will restore the initial score tempo.

[rtbo]

Rebased on master and solved conflict (again :smile:). I can remove "tempo primo" if you want this PR more focused.

[AceAttorneyMaster111]

Any idea when this PR will be merged?

[Jojo-Schmitz]

First another rebase is needed