Make `EffectControlDialog` inherit `PluginView`

[JohannesLorenz]

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]

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]

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]

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]

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]

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]

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.

[JohannesLorenz]

However, if multiple other PRs are waiting on this, I think it's okay to merge.

They are not waiting because they need it, but only because their code would differ if that PR would be merged :smiley:

Usually refactors should offer a clear benefit though simplification, elegance, removal of duplicate code, etc. but this PR doesn't do that as far as I can tell.

I am totally fine with this opinion (and not offended, I am happy about the honesty). This PR is not needed, so I am also fine with not finishing it. At least, we could finally continue with the other PRs :laughing:

@michaelgregorius What do you think? Let's move this PR to the archives and finally move on with our resize/maximize PRs? Or do you have a strong desire/dependency that I finish this PR?

[michaelgregorius]

Sorry for the late answer, @JohannesLorenz! I do not have a dependency on this PR so feel free to archive it. I think the splitting of the models as described in https://github.com/LMMS/lmms/pull/7544#issuecomment-2456917148 might still make sense but it could also be done in a separate PR.

[JohannesLorenz]

Closing this, as discussed.