GNOME Bugzilla – Bug 657548
overview: removeSearchProvider API
Last modified: 2011-09-04 11:41:39 UTC
There's no API to remove a SearchProvider. It'd be useful for extensions adding a SearchProvider to be able to remove it in their "disable" function.
Created attachment 194960 [details] [review] viewSelector: removeSearchProvider API Unregister the SearchProvider added via the addSearchProvider method of the ViewSelector.
Created attachment 195563 [details] [review] overview: removeSearchProvider API Unregister the SearchProvider added via the addSearchProvider method of the Overview.
Review of attachment 195563 [details] [review]: Code looks reasonable, except for some nits (I didn't actually test the code though). I don't think the commit message is appropriate though - the small API addition to overview.js is hardly the main point of the patch, it's about adding the ability to remove search providers from the search system in the first place. ::: js/ui/search.js @@ +360,3 @@ + provider.searchSystem = null; + let index = this._providers.indexOf(provider); + if (index != -1) I guess we shouldn't reset provider.searchSystem in case the provider hasn't been registered? How about: let index = this._providers.indexOf(provider); if (index == -1) return; provider.searchSystem = null; this._providers.splice(index, 1); ::: js/ui/searchDisplay.js @@ +307,3 @@ + if (meta.provider == provider) { + this._content.remove_actor(meta.actor); + meta.actor.destroy(); Destroying the actor is enough, the container will do any necessary cleanup automatically. @@ +309,3 @@ + meta.actor.destroy(); + providerIndex = i; + break; In general it's not advisable to manipulate an array while iterating over it, but as you are doing a break anyway, you can just splice the array here.
Created attachment 195614 [details] [review] overview: Add removeSearchProvider API Add a public API to remove search providers, so extensions don't have to access the view selector directly, which is now a private property of the overview.
Review of attachment 195614 [details] [review]: Code look good, commit message somehow misses the point - as you don't appear to have commit access on gnome.org, I'll be so frank to rewrite it when pushing.
Comment on attachment 195614 [details] [review] overview: Add removeSearchProvider API Pushed as 595be5083 - overview: Add API to remove search providers