GNOME Bugzilla – Bug 597309
Add workspace button is recreated every time the overview is shown
Last modified: 2009-10-05 16:55:51 UTC
The (+) (ie add workspace button) is created every time the overview is shown (in Main.overview.show() ), but is never destroyed. So after a while we end up with multiple copies of the icon.
Created attachment 144763 [details] [review] Don't create multiple copies of workspaces and the (+) button
Comment on attachment 144763 [details] [review] Don't create multiple copies of workspaces and the (+) button >From 59129ecabc680581d2c5315b1f89c6cdee8eae92 Mon Sep 17 00:00:00 2001 >From: Adel Gadllah <adel.gadllah@gmail.com> >Date: Mon, 5 Oct 2009 09:23:24 +0200 >Subject: [PATCH] Don't create multiple copies of workspaces and the (+) button > >Currently we recreate them every time Main.overview.show() is called, > so destroy them to avoid having multiple copies floating around. > >https://bugzilla.gnome.org/show_bug.cgi?id=597309 >--- > js/ui/overview.js | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > >diff --git a/js/ui/overview.js b/js/ui/overview.js >index 4d5b37c..b51d71d 100644 >--- a/js/ui/overview.js >+++ b/js/ui/overview.js >@@ -293,6 +293,10 @@ Overview.prototype = { > this._dash.show(); > > /* TODO: make this stuff dynamic */ >+ if (this._workspaces != null) { >+ this._workspaces.destroy(); >+ this._workspaces = null; >+ }] We do this in _hideDone(); I'm not sure how looking at the code we could end up not calling _hideDone. Were you hitting this in practice or just being defensive? > this._workspaces = new Workspaces.Workspaces(this._workspacesWidth, this._workspacesHeight, > this._workspacesX, this._workspacesY); > this._group.add_actor(this._workspaces.actor); >@@ -357,6 +361,10 @@ Overview.prototype = { > this._activeDisplayPane.close(); > this._workspaces.hide(); > >+ this._addButton.actor.destroy(); >+ this._addButton.actor = null; >+ this._addButton = null; >+ The AddWorkspaceButton code should be using ShellTextureCache so we avoid re-loading the SVG each time, but that's unrelated to this bug. This change looks fine.
(In reply to comment #2) > (From update of attachment 144763 [details] [review]) > >From 59129ecabc680581d2c5315b1f89c6cdee8eae92 Mon Sep 17 00:00:00 2001 > >From: Adel Gadllah <adel.gadllah@gmail.com> > >Date: Mon, 5 Oct 2009 09:23:24 +0200 > >Subject: [PATCH] Don't create multiple copies of workspaces and the (+) button > > > >Currently we recreate them every time Main.overview.show() is called, > > so destroy them to avoid having multiple copies floating around. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=597309 > >--- > > js/ui/overview.js | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > >diff --git a/js/ui/overview.js b/js/ui/overview.js > >index 4d5b37c..b51d71d 100644 > >--- a/js/ui/overview.js > >+++ b/js/ui/overview.js > >@@ -293,6 +293,10 @@ Overview.prototype = { > > this._dash.show(); > > > > /* TODO: make this stuff dynamic */ > >+ if (this._workspaces != null) { > >+ this._workspaces.destroy(); > >+ this._workspaces = null; > >+ }] > > We do this in _hideDone(); I'm not sure how looking at the code we could end up > not calling _hideDone. Were you hitting this in practice or just being > defensive? I have hit this with a custom button I added and confirmed it for the addButton, but seems you are right that is already done in _hideDone. > > this._workspaces = new Workspaces.Workspaces(this._workspacesWidth, this._workspacesHeight, > > this._workspacesX, this._workspacesY); > > this._group.add_actor(this._workspaces.actor); > >@@ -357,6 +361,10 @@ Overview.prototype = { > > this._activeDisplayPane.close(); > > this._workspaces.hide(); > > > >+ this._addButton.actor.destroy(); > >+ this._addButton.actor = null; > >+ this._addButton = null; > >+ > > The AddWorkspaceButton code should be using ShellTextureCache so we avoid > re-loading the SVG each time, but that's unrelated to this bug. This change > looks fine. OK, will reattach with only this change.
Created attachment 144807 [details] [review] Don't create multiple copies of the (+) button
Comment on attachment 144807 [details] [review] Don't create multiple copies of the (+) button approval