Code review comment for lp:~unity-api-team/unity-scope-click/blacklist-apps

Revision history for this message
dobey (dobey) wrote :

First thing I see is that the bug is not linked with commit --fixes on the commit.
Beyond that, the more I've looked at this MP, the less I like it. The code itself isn't terribly incorrect or anything, but the term "blacklist" just disturbs me, and the implementation here has some inconsistency with the existing implementation of the core-apps gsettings key. I'm also not fond of moving the one function into the struct in application.h. I also noticed that you modified the find_installed_apps() API call, but did not actually adjust the call to it within apps-query.cpp's run() method. You seem to have only implemented a call to it within Configuration::get_core_apps() by removing items from that list only. This would not completely resolve the bug, as it would only remove the items from the top set in the scope. The way the tests work here also seems a bit complicated.

I've made an MP at https://code.launchpad.net/~dobey/unity-scope-click/ignored-setting/+merge/286597 which I think is more consistent and complete. Please feel free to review it and comment, and we can discuss further tomorrow.

review: Needs Fixing

« Back to merge proposal