Code review comment for lp:~charlesk/indicator-location/gmenuify

Revision history for this message
Ted Gould (ted) wrote :

* Need to drop data/indicator-location.desktop

* I don't think that we need to update the icon cache, that's handled by
the dpkg hook as it needs to be refreshed on package install not source
install.

* Need to add phone.cc to po/POTFILES.in

* The build system doesn't seem to be building the internationalization
at all

* Surprised the compiler doesn't hate this, but should probably put a
"static" in the cc file for
on_location_service_controller_status_changed() just to make it clearer

* I think that in controller.cc you need to check to see if
on_gps_enabled_changed or on_location_service_enabled_changed is null as
it's possible a subclass would implment one and not both

* We probably should either use or remove the icons. I don't care which
way we go there :-)

* The accessible string "Location" should be translatable.

* If you're going to use C++ 11 would have liked to see more usage of
lambdas ;-)

* Showing the settings needs to use URL Dispatcher

  review needs-fixing

review: Needs Fixing

« Back to merge proposal