search

Kitware / cinema

ParaView Cinema
Other
11 stars 9 forks source link

View removal propagation #107

Closed zachmullen closed 9 years ago

patrickoleary commented 9 years ago

wait

On Thu, Nov 6, 2014 at 12:45 PM, Zach Mullen notifications@github.com wrote:

You can merge this Pull Request by running

git pull https://github.com/Kitware/cinema view-removal-propagation

Or view, comment on, or merge it at:

https://github.com/Kitware/cinema/pull/107 Commit Summary

  • Window resize event handler now unbound when view is removed
  • Merge remote-tracking branch 'origin/master' into view-removal-propagation
  • Propagate view removal to render views within top level widgets

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/Kitware/cinema/pull/107.

patrickoleary commented 9 years ago

The

getSharedData(this.compositeModel, this.$el, this._hasAnalysis).remove();

should remove the renderView and the searchView. We need to detach all the this.* connections to these resources. When are we calling the remove? It looks like this call would be elevated somewhere in the workbench manipulation area. The single view systems probably don't have to call remove, but it would be nice to systematically cleanup.

Patrick

On Thu, Nov 6, 2014 at 12:51 PM, Patrick O'Leary patrick.oleary@kitware.com wrote:

wait

On Thu, Nov 6, 2014 at 12:45 PM, Zach Mullen notifications@github.com wrote:

You can merge this Pull Request by running

git pull https://github.com/Kitware/cinema view-removal-propagation

Or view, comment on, or merge it at:

https://github.com/Kitware/cinema/pull/107

Commit Summary

Window resize event handler now unbound when view is removed Merge remote-tracking branch 'origin/master' into view-removal-propagation Propagate view removal to render views within top level widgets

File Changes

M web/src/js/app/views/Chart.js (7) M web/src/js/app/views/Composite.js (12) M web/src/js/app/views/CompositeCompCalcWebGl.js (7) M web/src/js/app/views/CompositeLight.js (7) M web/src/js/app/views/CompositeWebGl.js (7) M web/src/js/app/views/CompositeWebGlJpg.js (7) M web/src/js/app/views/CompositeWebGlLight.js (7) M web/src/js/app/views/CompositeWebGlLut.js (7) M web/src/js/app/views/StaticImage.js (7)

Patch Links:

https://github.com/Kitware/cinema/pull/107.patch https://github.com/Kitware/cinema/pull/107.diff

— Reply to this email directly or view it on GitHub.

zachmullen commented 9 years ago

When are we calling the remove? It looks like this call would be elevated somewhere in the workbench manipulation area.

Right now we are calling remove on this top level view when switching a workbench element to a new view.

The getSharedData(this.compositeModel, this.$el, this._hasAnalysis).remove(); should remove the renderView and the searchView. We need to detach all the this.* connections to these resources.

Unsetting the this.* references would not be sufficient; they would still be reachable via their statically scoped references in sharedDataMap in the IIFE, which is in the closures of the two top level view prototypes. The reference graph for this is getting rather complicated; in the workbench case, we want to be able to orphan the entire top-level-view subtree to the mark and sweep cycle, but it seems in the non-workbench case we want to never free these models and widgets that are shared. I'm having a hard time coming up with a way to reconcile those two cases in a way that will be easy to maintain...