Code review comment for lp:~unity-team/unity/places-bar

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Hey, super nice work. Only nitpickings:

unity-private/panel/panel-home-button.vala:

 * "along with this program" -> "along with program" in the GNU header. A s/this// on the lose? :-)

unity-private/panel/panel-view.vala:

 * public void set_indicator_mode (bool mode) could be set_indicator_hidden(bool hidden) to make the code clearer

unity-private/places/places-place-model.vala:

 * private async void load_place_files() calls load_place() which does sync io. You'd need to async load data into a buffer (fx. GInputStream -> GMemoryOutputStream) and do g_key_file_load_from_data(). Either add FIXME or do it fully async.

unity-private/places/places-place-search-bar.vala:

 * Timeout.add (0, bg.update_background); - why not Idle.add() then? A code comment to clarify why we need it in the first place perhaps?

 * Define const int for 400?

 * Hard coded BG in PlaceSearchBarBackground. Should be something like Config.DATADIR + "/unity/dash_background.png"

 * Sync io when loading pixbuf. Nontrivial fix. Perhaps add a code comment or FIXME and consider it for later

Regressions? We seem to no longer have:

 * Trash, Home screen

File bugs for missing work items to ensure we don't forget some details?

review: Approve

« Back to merge proposal