Add option to connect last changed model to AutomationClip

[regulus79]

This PR adds a new context menu action to AutomationClipViews which allows users to connect the last changed model.

A pointer to the last changed model is kept as a static variable AutomatableModel::s_lastChangedModel, which is updated in AutomatableModel::setValue(). When the AutomationClipView context menu is constructed, if AutomatableModel::s_lastChangedModel is not a nullptr and if the model's display name is not empty (Many hidden models have empty names, and it would be confusing to show them to users), then an option to connect it is given.

Demo:

2024-12-17.13-51-06.mp4

[sakertooth]

I am okay with this.

[sakertooth]

Oops, I messed up the indent. I also think we can move s_lastChangedModel into the private section of the class? Sorry for the back and fourth.

[regulus79]

AutomatableModel::s_lastChangedModel is accessed by AutomationClipView, so if it is moved to private I believe we would need to add a getter method, but sure.

[sakertooth]

AutomatableModel::s_lastChangedModel is accessed by AutomationClipView, so if it is moved to private I believe we would need to add a getter method, but sure.

That's fine. Yeah, The getter method is nice because it only allows read access to the variable.

[sakertooth]

Maybe I don't want to merge just yet: There's something I think I can do to remedy the unknown model name problem, at least when making clips.

[regulus79]

Also, has anyone tested this with a VST?

[sakertooth]

Also, has anyone tested this with a VST?

Automation with VST is unusable because of the spike in CPU, so there may not be a big use case for that at the moment.

Maybe I don't want to merge just yet: There's something I think I can do to remedy the unknown model name problem, at least when making clips.

The problem is basically that when constructing automatable models, it calls setValue in the constructor. I was thinking of maybe moving s_lastChangedModel into AutomatableModelView somehow and overriding mousePressEvent, but come to realize that it doesn't even inherit QWidget. This idea might take some refactoring, which we probably don't want to do in this PR, so I'm fine with just saying "(Unknown model name)" instead of just "()"

[sakertooth]

I'm having issues with this as setValue is called in the constructor, causing it to be set too soon. I also don't know how good of an idea is storing something like s_lastChangedModel in the Core when it only concerns the GUI layer.

We should stop storing s_lastChangedModel in AutomatableModel. It can probably go into AutomatableModelView. Just connect the Model::dataChanged signal to a lambda which sets s_lastChangedModel. That would be better than what is currently done now, IMO, but there's another problem.

Another problem I am seeing is if the project already has automation or LFO controllers attached to an AutomatableModel. It would just keep changing s_lastChangedModel when the user isn't changing it at all. We need some way to distinguish between AutomatableModel changes from the user and some other actor. A better idea maybe is to still store s_lastChangedModel in AutomatableModelView, but instead of taking the widget representing itself, it instead inherits QWidget virtually. Then you can override mousePressEvent and set it there. That isn't to say that mousePressEvents ultimately change the model by themselves, but I think this is solution works the way we would expect it to.

I have newfounded doubts that should be addressed.

[sakertooth]

Nevermind, it seems like the "consistently changing the model value" issue isn't there. I thought it was. Still don't like storing s_lastChangedModel in the Core though, but I guess this works for now..

[regulus79]

We should stop storing s_lastChangedModel in AutomatableModel. It can probably go into AutomatableModelView. Just connect the Model::dataChanged signal to a lambda which sets s_lastChangedModel. That would be better than what is currently done now, IMO, but there's another problem.

Some ModelViews change their models after they have been initialized (for whatever reason), which makes this idea more complex. I can connect the model's dataChanged signal to a lambda to update s_lastChangedModel, but if the model changed via AutomatableModelView::setModel, then I will have to also connect that one to a lambda too.

This is not necessarily an issue, but it makes the code a little less pretty since more checks have to be done and the connect statement appears in two places. It works, but it doesn't feel optimal, and it's easy to cause segfaults if I miss something.

[sakertooth]

Could you give an example?
Edit: Nevermind, I'll take a look myself soon.

[regulus79]

When you add a new automation track (for example), it first creates the Mute and Solo models, but they appear to be temporary as they are soon replaced with new Mute and Solo models. (I suspect this may be due to a call to Knob::setModel() after the knob has been constructed.)

This is a small excerpt of me printing

• qDebug() << this << "Connecting" << modelUntyped() << modelUntyped()->fullDisplayName() << "in AutomatableModelView constructor"; in the constructor and
• qDebug() << this << "AutomatableModelView::setModel was called with" << model << model->fullDisplayName() << "Old model:" << modelUntyped() << modelUntyped()->fullDisplayName(); in setModel()

0x5b122333c1a0 Connecting lmms::BoolModel(0x5b1223218cc0) "Mute" in AutomatableModelView constructor
0x5b1223210fb0 Connecting lmms::BoolModel(0x5b12232f60e0) "Solo" in AutomatableModelView constructor
0x5b122333c1a0 AutomatableModelView::setModel was called with lmms::BoolModel(0x77df3c005380) "Automation track>Mute" Old model: lmms::BoolModel(0x5b1223218cc0) "Mute"
0x5b1223210fb0 AutomatableModelView::setModel was called with lmms::BoolModel(0x77df3c005448) "Automation track>Solo" Old model: lmms::BoolModel(0x5b12232f60e0) "Solo"

Almost all models do this, so lmms is spamming debug info like this when it starts up.

[sakertooth]

Got the program to crash by adding a mixer channel and then right-clicking on an automation clip. We might need to rethink the solution a bit.

[regulus79]

I cannot reproduce the crash. Did you add the mixer channel after adding the clip? Also did you change any knobs before doing it, or was it a fresh project?

[sakertooth]

No, I just added a mixer channel with the "+" button, and then right clicked on an automation clip. I honestly don't know why you can't reproduce it. I'll try again soon.

Edit: The clip was already created. I right clicked on an already existing automation clip after adding the mixer channel. The project was completely empty but for the automation clip and the extra mixer channel. I don't think I changed any knobs before doing it.

[regulus79]

I can now reproduce the crash. It appears it only occurs when starting from an empty project.

[firewall1110]

Is s_lastChangedModel always initialized properly?
May be some more nullptr checks needed ...

P.S.
It is only quick impression - may be wrong ...