Make `EffectControlDialog` inherit `PluginView` #7544

JohannesLorenz · 2024-10-13T21:24:26Z

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:

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.
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).

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 · 2024-10-13T21:27:02Z

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 · 2024-10-13T22:41:57Z

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 · 2024-10-17T12:03:27Z

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 · 2024-10-19T16:58:42Z

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 · 2024-11-05T11:25:14Z

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 · 2024-11-20T22:00:37Z

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.

messmerd

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 don't like how Effect::instantiateView is now unused, which seems like a step back.

However, if multiple other PRs are waiting on this, I think it's okay to merge. A proper refactor of the instrument/effect views would take more time anyway.

I tested it and there do not seem to be any regressions.

messmerd · 2024-11-24T17:41:15Z

include/EffectControlDialog.h

 {
 	Q_OBJECT
 public:
-	EffectControlDialog( EffectControls * _controls );
+	EffectControlDialog(EffectControls *controls);


Suggested change

EffectControlDialog(EffectControls *controls);

EffectControlDialog(EffectControls* controls);

style

messmerd · 2024-11-24T17:41:26Z

include/EffectControlDialog.h

 protected:
-	void closeEvent( QCloseEvent * _ce ) override;
+	void closeEvent(QCloseEvent *closeEv) override;


Suggested change

void closeEvent(QCloseEvent *closeEv) override;

void closeEvent(QCloseEvent* closeEv) override;

messmerd · 2024-11-24T17:41:40Z

include/EffectView.h

 {
 	Q_OBJECT
 public:
-	EffectView( Effect * _model, QWidget * _parent );
+	EffectView(Effect *model, QWidget *parent);


Suggested change

EffectView(Effect *model, QWidget *parent);

EffectView(Effect* model, QWidget* parent);

messmerd · 2024-11-24T17:41:56Z

src/gui/EffectControlDialog.cpp

-	QWidget( nullptr ),
-	ModelView( _controls, this ),
-	m_effectControls( _controls )
+EffectControlDialog::EffectControlDialog(EffectControls *controls) :


Suggested change

EffectControlDialog::EffectControlDialog(EffectControls *controls) :

EffectControlDialog::EffectControlDialog(EffectControls* controls) :

messmerd · 2024-11-24T17:42:06Z

src/gui/EffectControlDialog.cpp

 }




-void EffectControlDialog::closeEvent( QCloseEvent * _ce )
+void EffectControlDialog::closeEvent(QCloseEvent *closeEv)


Suggested change

void EffectControlDialog::closeEvent(QCloseEvent *closeEv)

void EffectControlDialog::closeEvent(QCloseEvent* closeEv)

messmerd · 2024-11-24T17:42:20Z

src/gui/EffectView.cpp

-	PluginView( _model, _parent ),
-	m_bg( embed::getIconPixmap( "effect_plugin" ) ),
-	m_subWindow( nullptr ),
+EffectView::EffectView(Effect * modelParam, QWidget *parent) :


Suggested change

EffectView::EffectView(Effect * modelParam, QWidget *parent) :

EffectView::EffectView(Effect* modelParam, QWidget* parent) :

JohannesLorenz · 2024-11-24T21:26:53Z

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 😃

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 😆

@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?

JohannesLorenz mentioned this pull request Oct 13, 2024

Maximize button for resizable instruments #7514

Open

messmerd approved these changes Nov 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `EffectControlDialog` inherit `PluginView` #7544

Make `EffectControlDialog` inherit `PluginView` #7544

JohannesLorenz commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

michaelgregorius commented Oct 17, 2024

JohannesLorenz commented Oct 19, 2024

michaelgregorius commented Nov 5, 2024

JohannesLorenz commented Nov 20, 2024

messmerd left a comment

messmerd Nov 24, 2024

messmerd Nov 24, 2024

messmerd Nov 24, 2024

messmerd Nov 24, 2024

messmerd Nov 24, 2024

messmerd Nov 24, 2024

JohannesLorenz commented Nov 24, 2024

	EffectControlDialog(EffectControls *controls);
	EffectControlDialog(EffectControls* controls);

	void closeEvent(QCloseEvent *closeEv) override;
	void closeEvent(QCloseEvent* closeEv) override;

	EffectView(Effect model, QWidget parent);
	EffectView(Effect* model, QWidget* parent);

	EffectControlDialog::EffectControlDialog(EffectControls *controls) :
	EffectControlDialog::EffectControlDialog(EffectControls* controls) :

	void EffectControlDialog::closeEvent(QCloseEvent *closeEv)
	void EffectControlDialog::closeEvent(QCloseEvent* closeEv)

	EffectView::EffectView(Effect * modelParam, QWidget *parent) :
	EffectView::EffectView(Effect* modelParam, QWidget* parent) :

Make EffectControlDialog inherit PluginView #7544

Are you sure you want to change the base?

Make EffectControlDialog inherit PluginView #7544

Conversation

JohannesLorenz commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

michaelgregorius commented Oct 17, 2024

JohannesLorenz commented Oct 19, 2024

michaelgregorius commented Nov 5, 2024

JohannesLorenz commented Nov 20, 2024

messmerd left a comment

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

messmerd Nov 24, 2024

Choose a reason for hiding this comment

JohannesLorenz commented Nov 24, 2024

Make `EffectControlDialog` inherit `PluginView` #7544

Make `EffectControlDialog` inherit `PluginView` #7544