Issue3778 - Separate MainForm from LeftPanel

[sgpearse]

Separates MainForm's image-capture and import-data functionality into the classes CaptureController and DatasetImporter so these tools can be used by the LeftPanel.

Submitted as a draft because of one question: MainForm.cpp:1246 (on main) has the following logic:

    string          vizName = p->GetActiveVizName();
    if (vizName != _capturingAnimationVizName) { MSG_WARN("Terminating capture in non-active visualizer"); }
    if (_controlExec->EnableAnimationCapture(_capturingAnimationVizName, false)) MSG_WARN("Image Capture Warning;\nCurrent active visualizer is not capturing images");

Is this warning still achievable? I am not able to produce it when changing visualizers during an image capture, or any other way I can think of. I've removed it in this PR, but we can keep it if the MainForm communicates _capturingAnimationVizName to the CaptureController whenever it changes. It was added in 2017 and does not appear to be needed anymore, from what I can tell.

[StasJ]

This introduces duplication of application state, for example through _sessionNewFlag.

The objects that were added to replace coupling MainForm are themselves tightly coupled to MainForm. Most of their functionality can be stateless and the few things that need to be communicated with MainForm are events and can therefore just use signals.

With regard to the code snippet in your PR description, have you tried reproducing the issue with multiple visualizers?

[sgpearse]

Thanks for your review @StasJ. I can fix the code comments no problem. I do want to talk about this overall approach and the points you raise in your review.

This introduces duplication of application state, for example through _sessionNewFlag.

This is true. I think the correct fix is to put these state flags into GUIStateParams. Do you agree?

The objects that were added to replace coupling MainForm are themselves tightly coupled to MainForm. Most of their functionality can be stateless and the few things that need to be communicated with MainForm are events and can therefore just use signals.

While the MainForm is coupled to these classes, separating this logic makes MainForm more cohesive. As we know, MainForm is over a thousand lines long and it violates many OO principles. Single Responsibility Principle, from SOLID, especially. It's hard to test, understand, and modify because it's doing too much, making it very fragile. This was a big pain point in our last release. Using signals would keep data-import and image-capture logic within the MainForm.

What is the benefit of keeping this logic in the MainForm, versus breaking it out?

With regard to the code snippet in your PR description, have you tried reproducing the issue with multiple visualizers?

Yes, noted this in the PR submission. I'm only able to reproduce this on older versions of VAPOR where we need to "End Capture Sequence" manually.

[StasJ]

This is true. I think the correct fix is to put these state flags into GUIStateParams. Do you agree?

_sessionNewFlag is ephemeral state and does not need to be persisted, therefore I would keep a single instance as a regular variable, however, putting it in the GUIStateParams would be fine.

While the MainForm is coupled to these classes, separating this logic makes MainForm more cohesive. As we know, MainForm is over a thousand lines long and it violates many OO principles. Single Responsibility Principle, from SOLID, especially. It's hard to test, understand, and modify because it's doing too much, making it very fragile. This was a big pain point in our last release. Using signals would keep data-import and image-capture logic within the MainForm.
What is the benefit of keeping this logic in the MainForm, versus breaking it out?
With regard to the code snippet in your PR description, have you tried reproducing the issue with multiple visualizers?

There is a difference between having an object contain code for something and that object having ownership over it. I am not advocating for or against putting this code back in the MainForm, rather I am saying they don't need to be objects at all (well they might technically need to be for qt's sake depending on how you do it but that's an implementation detail). For example the dataset importer can be purely a few static functions rather than an object with its own state which needs to be tracked and passed around.

Yes, noted this in the PR submission. I'm only able to reproduce this on older versions of VAPOR where we need to "End Capture Sequence" manually.

It sounds like the underlying issue has been resolved so that code can be removed.

[sgpearse]

@StasJ - To remove state from the Capture/Import utilities, I've added new parameters so state can still be communicated to the MainForm. These changes tracked in MainForm's event filter during ParameterChangeEvents.

[StasJ]

Thanks Scott. Just a couple more comments.

[StasJ]

You defined static variables to represent these strings in your last PR. You are also using magic numbers here.

#3719 (comment)

[StasJ]

This will miss overwriting later timesteps. Low priority.

[StasJ]

You are using the params database to essentially recreate signals/slots here.

[sgpearse]

Thanks for your patience here while I've been abroad.

You are correct. Using signals and slots would require me to shuffle events across the following series of classes:

PButton -> PCaptureHBox -> PCaptureWidget -> ExportTab -> LeftPanel -> MainForm

Since each of these objects would need to be configured for a signal and a slot, I found it simpler to have the MainForm watch for a parameter change in the eventFilter.

[StasJ]

There are a couple ways to handle this. IMO, the signal chain is preferable to sending the signal through a flag in the params database. The way you've implemented it here you can undo the application acknowledging that the user started the animation playback which makes no sense.