Disables automations for some widgets (mostly ComboBoxes)

[michaelgregorius]

Extends AutomatableModelView by a new method to enable or disable
automations. The state of the variable is taken into account with
regards to the ability to drag & drop automations and to show the
context menu in the classes ComboBox and Knob.

Disables automations for all ComboBoxes in the editors (Song-Editor,
Piano Roll, Automation-Editor and BB-Editor).

Also disables automations for the tension knob in the Automation-Editor.

Fixes issue #2382

[Wallacoloo]

I haven't looked into the issue, or looked through the code, but doesn't it
not make sense to have an AutomatableModelView that's not automatable?
Seems like a contradiction.

I believe we also have a class, ModelView. I'm not very familiar with
these areas of the code, but would it make more sense to have the editor
controls be ModelViews, which I assume are non-automatable by default?

[michaelgregorius]

@Wallacoloo It depends on how you interpret the name AutomatableModelView. I'm not a native speaker but to me the word "automatable" conveys a meaning of "can be automated (but does not have to)". If every instance would be automated I would expect a name like AutomatedModelView.

The problem with letting ComboBox (and other controls) inherit from ModelView is that we have to be sure that not a single of the existing ComboBoxes needs to be automated. If there was a ComboBox that needs to be automated we could surely have something like an AutomatedComboBox that inherits from AutomatableModelView but this might lead to an explosion of classes in my opinion.

[Spekular]

It's true that automatable implies that something can be automated (but doesn't have to), but I'm pretty sure this is referring to the user. An automatable model can be automated by the user, so it doesn't make sense to have something the user can automate with automation disabled. An automated model sounds like it would always be automated.

[tresf]

@Wallacoloo @michaelgregorius, in C++ can the combobox have two separate constructors for two separate inherited classes?

I agree with @Wallacoloo that putting this in the Automatable category is quite misleading, but I also strongly agree with @michaelgregorius about the unnecessary duplication of (otherwise identical) classes that we'd have if we had two combo boxes, etc, etc.

[tresf]

Can someone point me to the portion of code where ComboBox is initialized as automatable? I can't seem to find it.

From what I'm reading, most of these GUI components don't assume they're automatable (or if they do, I think it's a bug). I believe rather than toggling the automatable class as non-automatable, we should instead do a type-check on their own internal model before adding any context menus to right-click. I may be wrong here and if I am, @michaelgregorius's PR is the only way to go.

[tresf]

Ok... I think I found it... IntModelView is dynamically created by AutomatableModelView, which binds the ComboBox to be automated. This is a bit misleading as the IntModelView is more appropriately the IntAutomatableModelView, etc but anyway this technique seems to be use for many other GUI items such as LcdSpinBox, AutomatableSlider, etc.

This means architecturally most GUI components would fall under the automatable model and are directly coupled to extend the class but the underlying problem is that our naming convention is off a bit with the combo box.

For example, at one point, we labeled automatable components e.g. AutomatableButton but then later we have classes extending them e.g. PixmapButton that -- by name alone -- don't illustrate they're automatable (hint, TabWidget's one that isn't automatable.) Uggg...

So the question comes back to -- do we let this ComboBox inherit a bunch of automatable logic that it cannot use? It shouldn't, really. Rolling a separate ComboBox and AutomatableComboBox would make much more sense despite the redundancy.

This would have to be repeated with Knob of course, which I'd expect to be an even larger undertaking.

[jasp00]

IMHO, being automatable is a concept from the model. While the idea in this request is similar, model objects should have isAutomatable() and setAutomatable() rather than GUI objects having automationsEnabled() and enableAutomations(). So ComboBox should check whether its model is automatable and AutomatableComboBox is unnecessary.

[jasp00]

Setting the automatable capability in the constructor would be better than setAutomatable().

[tresf]

IMHO, being automatable is a concept from the model. While the idea in this request is similar, model objects should have isAutomatable() and setAutomatable() rather than GUI objects having automationsEnabled() and enableAutomations(). So ComboBox should check whether its model is automatable and AutomatableComboBox is unnecessary.

So take it out of the view. Agreed. Close an re-PR?

[jasp00]

Close an re-PR?

@michaelgregorius decides whether to reuse this PR.

[michaelgregorius]

@jasp00, @tresf I think it's cleaner to open a new PR for the alternative solution. I propose to open a new PR, let this PR and the new one reference each other so that the conversation does not get lost and then to close this one.

[JohannesLorenz]

Adding "in-progress" label, as this PR does currently not require any review.

[husamalhomsi]

@michaelgregorius Please reference this PR when you open a new PR. Closing.