Mitigate crashes when user quickly rearranges pipeline views.

[imikejackson]

There is some minimal flashing of the SVPipelineView at the start/end of the animation
but by disabling the SVPipelineView we stop the user from putting the PipelineModel
into a state that will cause crashes.

updates #315

Signed-off-by: Michael Jackson [email protected]

[mmarineBlueQuartz]

What happens if the user double-clicks a filter in the filter list widget? If a filter is added in this way, the pipeline view will preemptively be enabled, but if it isn't added, users might not be paying attention and assume their pipeline has the filters they tried to add. As a quick fix for a release, the former is perfectly fine and hard to abuse, but the latter could be a usability nightmare and a great frustration for users.

As long as double-clicking filters still adds them to the pipeline view while an animation is in progress, I am okay with this change.

[imikejackson]

Will have to test that combination...

[joeykleingers]

I am OK with this fix as long as what Matthew said still works.

For a future, more permanent fix, is there a way that we could have an AboutToRemove state for AbstractFilters in a FilterPipeline? That way we can set the state to AboutToRemove right before starting the animation and then a subsequent preflight only preflights filters that are not in the AboutToRemove state.

There may also be other reasons in the future why we would be interested in knowing that a filter is about to be deleted from a pipeline.

[mmarineBlueQuartz]

We already have SIMPL_INSTANCE_PROPERTY(bool, Removing) in AbstractFilter precisely because of a similar crash where users could delete the same filter multiple times. Is that what you're talking about? It kind of feels hacky since the pipeline shouldn't care about the animations as a non-GUI entity. A filter is either there, or it isn't. I'd rather remove the Removing property entirely as it doesn't belong, but that would require redesigning the GUI to prevent the crash in the first place. The FilterPipeline doesn't (and shouldn't) keep track of the GUI to prevent it from going off the rails. That's the GUI's job.

SIMPLView might have issues keeping track of the GUI, but do we need to bloat the operation in other applications using SIMPLib that don't have this issue with checks for animations particularly in cases where there is no GUI being used? Does the REST API need to check for animations coming over a network connection during its preflight? What about our other applications? Yes, a boolean check on every single filter in preflight won't slow things down a noticeable amount, but why is it there to begin with? Why isn't that check where the issue actually comes from instead? It's not a permanent fix. It's a band-aid to cover up an issue.

[imikejackson]

I agree that disabling the pipeline is a band-aid over the issue but at the moment it will help prevent the crashes. I believe a better overall design is needed to solve the issue with a proper separation of GUI and low level libraries. If this PR does not pass the smell test then we can go back to the design board and try again. I don't really want to put in code that just makes fixing things later more difficult.

[joeykleingers]

If we don't want to have some mechanism that marks the filter as AboutToBeRemoved (which is not a clean solution, I agree), then I don't see any other choice than to design it so that filters are removed immediately from the pipeline when the removal is triggered. And then we'll have to use different animations because we can't animate an already-removed filter. We simply can't have a split second of time where the animation is running, the filter isn't deleted yet but is about to be, and the user can also still trigger operations on the pipeline model.

[mmarineBlueQuartz]

If we're allowing the model to become desync'ed for the purpose of animation, then we can iterate over the pipeline (as we already are), and asking the model to update the specific filter instead of doing so by index. The act of removing a filter through animation already has to access the model, so it can update the specific index to reflect that change.

[imikejackson]

I think we might want to explore swapping in a "dummy filter" into the pipeline model when the pipeline needs to animate. The act of generating the pipeline is handled by the PipelineModel, which can make a decision based on the "type" of item in the QListView whether to add that to the actual FilterPipeline object that is being created.

If worse comes to worse we can get rid of the animation effect all together which stops this bug from happening.