Remove use of showView to control view display.

Remove use of showView to control view display.

- minimized, sidebar, and fullscreen were using showView, remove that.
- Make sure they all destroy themselves cleanly
- Make sure that their destructor method is called when going from one view to
another.
- Update tests to work with these changes.

Note:
The test file was indented as it was wrapped in a closure to enable all suites
to share the addBrowserCotnainer function.

Tests were updated for additional cleanup and the "browser subapp display
tree" test beforeEach/after to adjust how we track if the main views were
indeed called during dispatching.

https://code.launchpad.net/~rharding/juju-gui/view-cleanup/+merge/178140

(do not edit description out of merge proposal)

[rharding, 2013-08-01 18:01:09]

Please take a look.

[jeff.pihach, 2013-08-01 18:16:25]

LGTM - with the changes below.
will QA.

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser...
app/subapps/browser/browser.js:423: this._sidebar.destroy();
maybe these should be stored in a browserViews object property which can be
looped over? These properties should also be documented somewhere so that they
dont' just 'appear'

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser...
app/subapps/browser/browser.js:648: this._fullscreen = new views.FullScreen(
Because this setup is done every time this should probably be abstracted out
into a custom showView like method so that someone in the future doesn't
accidentally forget to addTarget for example.

[j.c.sackett, 2013-08-01 18:21:09]

LGTM, one note on view management.

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser...
app/subapps/browser/browser.js:421: // If we've got any views hanging around
wipe them.
It would probably be easier to just keep a collection of all views, and destroy
any non null ones as we iterate across the collection.

[jeff.pihach, 2013-08-01 18:23:10]

QA Failed
visit root path w/o flag, click the icon to close the sidebar, open it, close
it, it will no longer open when clicking the icon.

[rharding, 2013-08-02 15:37:55]

Please take a look.

[rharding, 2013-08-02 15:43:29]

Thanks for the reviews. A couple of notes below and an updated branch to add a
test to catch the bug that Jeff found during QA.

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser...
app/subapps/browser/browser.js:423: this._sidebar.destroy();
On 2013/08/01 18:16:25, jeff.pihach wrote:
> maybe these should be stored in a browserViews object property which can be
> looped over? These properties should also be documented somewhere so that they
> dont' just 'appear' 

I'm waiting to pull this back into the animations branch before I refactor this.
I'll have to control this as part of the animation process and not sure the best
way yet. At least this way it's explicit and clear.

https://codereview.appspot.com/12272043/diff/3001/app/subapps/browser/browser...
app/subapps/browser/browser.js:648: this._fullscreen = new views.FullScreen(
On 2013/08/01 18:16:25, jeff.pihach wrote:
> Because this setup is done every time this should probably be abstracted out
> into a custom showView like method so that someone in the future doesn't
> accidentally forget to addTarget for example.

I had thought about trying to do this but the _$var = new $class means two
different vars passed and will see what I can come up with as I move forward.

[rharding, 2013-08-02 15:48:31]

*** Submitted:

Remove use of showView to control view display.

- minimized, sidebar, and fullscreen were using showView, remove that.
- Make sure they all destroy themselves cleanly
- Make sure that their destructor method is called when going from one view to
another.
- Update tests to work with these changes.

Note:
The test file was indented as it was wrapped in a closure to enable all suites
to share the addBrowserCotnainer function.

Tests were updated for additional cleanup and the "browser subapp display
tree" test beforeEach/after to adjust how we track if the main views were
indeed called during dispatching.

R=jeff.pihach, j.c.sackett
CC=
https://codereview.appspot.com/12272043