search

LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.18k stars 1.01k forks source link

Make `EffectControlDialog` inherit `PluginView` #7544

Open JohannesLorenz opened 1 month ago

JohannesLorenz commented 1 month ago

Before this commit, EffectView inherits PluginView and EffectControlDialog inherits ModelView. This commit switches those base classes.

The reason is that we previously had no base class for the 2 classes InstrumentView and EffectControlDialog: PluginView was the base class for InstrumentView, which does not contain the models for e.g. the "Volume" knob, and for EffectView, which does contain models for e.g. the "D/W" knob. This makes it questionable whether it is good that Effect contains an m_wetDryModel, but Instrument contains no "volume model" - but this commit does not fix this.

Notes:

  1. This commit should be a no-op, but isn't exactly: While before, EffectControlDialog passed EffectControls* to the ModelView, now, it passes Effect* to the PluginView, which passes it directly to the ModelView.
  2. The function Effect::instantiateView(QWidget*) seemed to be never executed. Nonetheless, it needs an implementation in order to provide a virtual method - we thus replace the content of this function with an assert(false).
JohannesLorenz commented 1 month ago

Note: I am not sure if this PR is a good idea. In my opinion, PluginView should be a base class of EffectControlDialog rather than EffectView, which I tried to justify in the commit message. But it is a matter of opinion.

For more details about what the widgets mean, please check allejok's widget overview .

JohannesLorenz commented 1 month ago

For discussion: An alternative would be to not touch PluginView, but instead add a new base class for InstrumentView and EffectControlDialog. I am not sure though if this will not complicate the inheritance hierarchy.

michaelgregorius commented 1 month ago

For discussion: An alternative would be to not touch PluginView, but instead add a new base class for InstrumentView and EffectControlDialog. I am not sure though if this will not complicate the inheritance hierarchy.

IMO it might help to think about how such a new base class would look like. What would be the functionality that's shared between InstrumentView and EffectControlDialog?

Another aspect that might be worth considering is where InstrumentView and EffectControlDialog are shown. Instances of InstrumentView are shown beneath the "Plugin" tab in the instrument window. Instances of EffectControlDialog are shown directly beneath the SubWindow in the MDI area. Does it make sense to have an IsResizable method for InstrumentView? Does it make sense to have an element that's deeper down in the widget hierarchy to dictate how the SubWindow that one of its parents is wrapped should behave with regards to resizing?

JohannesLorenz commented 1 month ago

Aside from isResizable(), what both classes have in common is ModelView (or PluginView, which is just passes everything through to its parent ModelView). The idea of ModelView is that subclasses have a reference to their model (ModelView::model()) and that an update of that model triggers an update of the view (ModelView::doConnections()).

The inconsistency currently ist that both EffectView (inside the EffectRackView) and EffectControlDialog (base class for the different Effect GUIs) have Effect set as that ModelView::model(). This means a change of any Effect parameter, be it a common parameter (D/W, Decay, Gain) or a specific parameter (e.g. specific to TripleOsc) will update both views. This is why I think the class Effect should be split into two classes (EffectCommon and Effect) which are neither child of each other, and have different views (EffectView and EffectControlDialog which should be renamed).

For discussion: An alternative would be to not touch PluginView, but instead add a new base class for InstrumentView and EffectControlDialog. I am not sure though if this will not complicate the inheritance hierarchy.

IMO it might help to think about how such a new base class would look like. What would be the functionality that's shared between InstrumentView and EffectControlDialog?

For the "alternative" approach of adding an extra class, currently solely isResizable. The ModelView stuff would not go into a base class. One function alone does not justify that base class approach IMO.

Another aspect that might be worth considering is where InstrumentView and EffectControlDialog are shown. Instances of InstrumentView are shown beneath the "Plugin" tab in the instrument window. Instances of EffectControlDialog are shown directly beneath the SubWindow in the MDI area. Does it make sense to have an IsResizable method for InstrumentView?

Definitely yes. While some people say that all LMMS instruments should be rewritten to be resizable, we cannot control that behavior for Lv2 UIs, which can be both resizable or not. The same argument counts for effects, ofc.

Does it make sense to have an element that's deeper down in the widget hierarchy to dictate how the SubWindow that one of its parents is wrapped should behave with regards to resizing?

Given my elaboration on the beginning of this comment: We have the commonalities "isResizing" and "ModelView". As they are both needed in both classes, IMO it comes naturally to have a base class.

Concluding everything, I think this PR approach is correct, but a further approach should still separate Effect into common and specific controls, and another should rename EffectView to EffectRackEntry and EffectControlDialog to EffectView. Does all of this make sense?

michaelgregorius commented 2 weeks ago

The inconsistency currently ist that both EffectView (inside the EffectRackView) and EffectControlDialog (base class for the different Effect GUIs) have Effect set as that ModelView::model(). This means a change of any Effect parameter, be it a common parameter (D/W, Decay, Gain) or a specific parameter (e.g. specific to TripleOsc) will update both views. This is why I think the class Effect should be split into two classes (EffectCommon and Effect) which are neither child of each other, and have different views (EffectView and EffectControlDialog which should be renamed).

I should then be considered to split the models. It sounds like the common parameters (D/W, Decay, Gain) should then get their own model. In the end they are applied as an "afterburner" after the processing of the actual effect, aren't they?

JohannesLorenz commented 1 day ago

It should then be considered to split the models. It sounds like the common parameters (D/W, Decay, Gain) should then get their own model. In the end they are applied as an "afterburner" after the processing of the actual effect, aren't they?

Sounds reasonable to me (I hope I am not complicating things unnecessarily?). However, as he was very active at exactly that code, I would like to ask @messmerd for a second opinion.