Code review comment for lp:~marcustomlinson/indicator-location/lp-1407921
Revision history for this message
| Marcus Tomlinson (marcustomlinson) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
> The code looks good; Approved.
>
> Marcus, I do have one optional request though, it would be slightly cleaner to
> add an enum class { Disabled, Idle, Active } to the controller and expose that
> state as a property, rather than the current patch which has two slightly
> overlapping bool states for enabled/disabled active/inactive.
Yeah, that is definitely a good idea. The motivation behind doing this way though, was in order to allow us to release this independent of the location-service updates. If I depend solely on the "State" property for both the enabled and active states, we would have to land them together.
While both are currently in a silo together, we are currently blocked on the location-service updates. Thomas is looking into it, but at least this way we have the option of landing independently if it comes to that.