GNOME Bugzilla – Bug 672941
Properly update application icons in dash/application view on icon theme changes
Last modified: 2012-12-14 08:32:44 UTC
To reproduce: (1) go to overview (2) open the Universal Access menu and toggle "High Contrast" Application icons in the dash should change if a high-contrast icon is available for them, but they don't until the shell is restarted. This is a regression introduced in the 3.3 cycle which slipped through, see attached patch.
Created attachment 210716 [details] [review] baseIcon: Always recreate icon texture on style changes Commit 26580f8f reintroduced an optimization on style changes to avoid creating icons unconditionally. As this breaks icon theme changes (for instance when toggling "High Contrast" in the universal access menu), remove it again.
Review of attachment 210716 [details] [review]: Looks good.
Attachment 210716 [details] pushed as b833aff - baseIcon: Always recreate icon texture on style changes
We need to find something better than this. This causes us to recreate the icon texture every time :hover changes, which is brutally inefficient.
Created attachment 230373 [details] [review] iconGrid: Only recreate icons when needed Recreating icons on every style change -- like hover, can have disasterous effects. Not only is the quick creation/destruction of the actors bad, but adding/removing actors at runtime queues many relayouts, which makes the whole system slower as a lot of unnecessary reallocations are figured out. While an optimization was here before, it was broken because it broke high-contrast themes. Connect explicitly to the texture cache to know when the icon theme has changed, instead of removing a valuable optimization.
Review of attachment 230373 [details] [review]: ::: js/ui/iconGrid.js @@ +123,3 @@ return; + this._createIconTexture(); Obviously broken.
Created attachment 230904 [details] [review] iconGrid: Only recreate icons when needed Recreating icons on every style change -- like hover, can have disasterous effects. Not only is the quick creation/destruction of the actors bad, but adding/removing actors at runtime queues many relayouts, which makes the whole system slower as a lot of unnecessary reallocations are figured out. While an optimization was here before, it was broken because it broke high-contrast themes. Connect explicitly to the texture cache to know when the icon theme has changed, instead of removing a valuable optimization.
Review of attachment 230904 [details] [review]: ::: js/ui/iconGrid.js @@ +127,3 @@ }, + _createIconTexture: function() { This function is called three times, twice as this.iconSize = size; this._createIconTexture(); Just leave the function as-is and call it as this._createIconTexture(this.iconSize) once. @@ +140,3 @@ }, + _ensureIconTexture: function() { Merge this with _onStyleChanged() (the only caller) - otherwise you should not just assume that you can call get_theme_node() in here. @@ +145,3 @@ + let node = this.actor.get_theme_node(); + let [found, len] = node.lookup_length('icon-size', false); + this.iconSize = found ? len : ICON_SIZE; The existing code looks clearer to me, but if you want to change the code anyway (which is really unrelated to the functional change), it'll help to s/size/oldSize/
Created attachment 231531 [details] [review] iconGrid: Only recreate icons when needed Recreating icons on every style change -- like hover, can have disasterous effects. Not only is the quick creation/destruction of the actors bad, but adding/removing actors at runtime queues many relayouts, which makes the whole system slower as a lot of unnecessary reallocations are figured out. While an optimization was here before, it was broken because it broke high-contrast themes. Connect explicitly to the texture cache to know when the icon theme has changed, instead of removing a valuable optimization.
Review of attachment 231531 [details] [review]: Much better, thanks!
Attachment 231531 [details] pushed as 429f9e1 - iconGrid: Only recreate icons when needed