Fix automation track unmute bugs #6550
Conversation
Fixes bug where automation tracks in 1.2 projects were not unmuted when the projects were saved with a soloed track, loaded into 1.3, and un- soloed. Also fixes related bug where pre-solo state of automation tracks was not reverted properly when an automation track was un-soloed.
|
I haven't studied the code closely but I somehow think this belongs in |
|
Ignore what I said. The legacy behavior setting means the user can switch between behaviors so there is no way to make an upgrade method. And loadSettings is already the way I suggested, so this is probably the only place to fix this issue. |
| @@ -622,14 +624,10 @@ void Track::toggleSolo() | |||
| track->m_soloModel.setValue(false); | |||
| } | |||
| } | |||
| else if (!soloBefore) | |||
| // Revert all tracks to pre-solo values | |||
| else if (!otherTrackAlreadySolo) | |||
There was a problem hiding this comment.
Thinking out loud:
soloEnabled is true if this track is solo . otherTrackAlreadySolo is true if any track is solo. At first glance it seems like this function is only called when the solo LED is clicked. So you may think that if you uncheck the solo LED of a track, then otherTrackAlreadySolo must be false. But this function also calls itself through this line:
track->m_soloModel.setValue(false);
And in that case a solo LED is unchecked while another solo LED is active. A bit confusing and ineffective but probably the simplest solution.
TL;DR there is nothing wrong here.
| { | ||
| track->setMuted(track->m_mutedBeforeSolo); | ||
| } | ||
| track->setMuted(track->m_mutedBeforeSolo); |
There was a problem hiding this comment.
More thinking out loud:
If soloLegacyBehavior is false, then the automation track is not muted, and "restoring" it will just unmute it (no change). So the if statement is not needed. This is correct.
(And for 1.2 projects m_mutedBeforeSolo defaults to false)
|
Thanks for your contribution! It's really appreciated, so don't let the rest of this comment discourage you. I don't think I agree with what behaviour would be most intuitive for the new solo mode. In my opinion, "soloing leaves automation tracks alone, but unsoloing restores them" is rather unintuitive, and "soloing/unsoloing ignores automation tracks" would be better (hence my original comment on the PR that introduced this bug). The problem isn't unique to the 1.2->1.3 upgrade: the same behaviour occurs if a track is muted under the legacy mode, but unmuted under the new mode. It's just that upgrading effectively switches your mode from legacy to new. I think that the real bug (or source of unintuitive behaviour, at least) is that an unsolo operation uses the current solo mode, rather than the mode that was in effect during the solo operation. As an alternative fix, I would consider making |
|
I'd imagine we need an upgrade method to adress the original bug if we choose to use @DomClark your suggestion fixes this problem:
Mute changes during solo isn't preserved for any other tracks so I wouldn't raise an eyebrow if it isn't preserved for automation tracks either (even with the new solo mode on). So personally I'd go as-is, just to keep it simple if nothing else. |
I think we'd be fine without - the way we did this last time is that we omit the attribute if the optional is empty. So all existing project files are still valid: they correspond to an optional that contains a value (and thus you get the desired behaviour).
That's true, but you could make a similar argument about consistency in favour of never implementing #5547 in the first place. Also, with #5547 in place (which I think was a useful change), automation track mute changes on unsolo can either be consistent with how other tracks behave on unsolo, or with how automation tracks behave on solo, but not both. So a consistency argument alone doesn't really clear anything up. We need to decide which is better, and I think we should aim for the most useful, intuitive workflow we can. Perhaps it's worth asking some users on Discord? That said, I'm happy with the code in this PR if we do choose to go with the current approach. |
|
Ah sorry @dom I was thinking of the wrong thing. I mean we need an upgrade method if we load a project older than #5565 that doesn't save the mute states prior to solo. In that case it would default to null which would mean no change so #6327 would persist. As for the logic your right. I'm just being lazy. |
This is my first contribution to LMMS! Let me know if you have any feedback.
This change is to fix #6327 - 1.2 projects with soloed tracks were not unmuting automation tracks properly when loaded into 1.3 and unsoloed.
While fixing this, I found a similar issue where you could mute an automation track, solo it, unsolo it, and the track would not revert to being muted. This change should fix that as well.
The issue seems to be the block associated with this PR #5547 (comment)
I don't think this check is necessary though, since we record the previous muted state of all tracks anyway whenever one is soloed. Soloing a different track will still leave automation tracks unmuted, and unsoloing a track will revert automation track mute statuses to whatever they were before. To me this seemed more intuitive.
Also I've changed a few var names (thought these might be more descriptive, open to reverting if anyone feels strongly though)