Code review comment for lp:~marcustomlinson/indicator-location/lp-1407921

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> > 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.

I just realised after hitting save that we could still abstract this behind the controller. If you feel strongly about it, sure, can do.

« Back to merge proposal