Code review comment for lp:~gary/juju-gui/bug1090716

Revision history for this message
Gary Poster (gary) wrote :

On 2013/01/11 12:51:03, hazmat wrote:
> On 2013/01/11 05:29:36, hazmat wrote:
> > On 2013/01/10 22:08:46, gary.poster wrote:
> > > Please take a look.
> >
> > this seems like a lot more code then just filtering at the model /
delta
> stream
> > level.

> more specifically trying to keep every known view/render location
guarded with a
> service filter is much more fragile and more code then simply
> removing/preventing in one place at ingest into the app models of the
gui
> service.

Benji and I discussed the two options in a pre-implementation call. We
preferred filtering on the view level for a few reasons.

- We were not convinced that it would be any easier in the long run to
make the change at the db/ingestion level rather than the view level.
If we had felt that this ease was the primary driver for an important
choice, we would have considered commissioning competing
implementations. However, we had other reasons to not pursue the
db/ingestion approach, given below.

- This is a view-level feature: we don't want to confuse users by
presenting certain charms. The db has nothing to do with the desired
outcome. In both of our experiences, implementing view-level behavior
in view code not only makes more sense intuitively but also is a better
long-term design decision.

- As a concrete example of why keeping the db truthful might be an
advantage, we discussed a possible future feature in which hidden
services could be revealed. For instance, let's say we want a proxy in
front of the gui or some other future hidden service. If we can reveal
hidden services with a gesture, we can make this happen, and we can also
see why a proxy that appears to be disconnected is actually connected
and performing a desired role.

> the utility functions here feel like they should be in model.utils
instead of
> view.utils.

Given our perspective that this is a view-level feature, and that these
utils are used in view-level code, the utilities make sense where they
are. Certainly if the code is refactored to filter at the ingestion/db
level then the utils belong somewhere else.

> needs fixing, imo.

I think this is an "implementer's choice" disagreement, where both sides
have reasonable arguments and the resolution should be in the hands of
the person who did the implementation. That's your call.

Gary

https://codereview.appspot.com/7069068/

« Back to merge proposal