Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(39)

Issue 7069068: Make GUI charm not shown in GUI

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 2 months ago by gary.poster
Modified:
11 years, 2 months ago
Reviewers:
teknico, mp+142794, hazmat, matthew.scott
Visibility:
Public.

Description

Make GUI charm not shown in GUI This only hides juju-gui charms in the environment. Relationships with the gui will be shown on inner pages, and you can actually click through to see a page for the juju-gui charm. This is regarded as a valuable incremental step--and possibly where we will leave things for now until we discover a need for greater hiding. Branch by Benji, with tweaks from Nicola and myself. https://code.launchpad.net/~gary/juju-gui/bug1090716/+merge/142794 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -15 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M app/views/topology/relation.js View 1 chunk +10 lines, -0 lines 1 comment Download
M app/views/topology/service.js View 5 chunks +23 lines, -14 lines 0 comments Download
M app/views/utils.js View 1 chunk +79 lines, -1 line 2 comments Download
M test/test_environment_view.js View 1 chunk +70 lines, -0 lines 0 comments Download
M test/test_utils.js View 2 chunks +104 lines, -0 lines 0 comments Download

Messages

Total messages: 10
gary.poster
Please take a look.
11 years, 2 months ago (2013-01-10 22:08:46 UTC) #1
matthew.scott
This looks good and works well. Land as is, or land with minor below. https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js ...
11 years, 2 months ago (2013-01-11 00:59:15 UTC) #2
hazmat
On 2013/01/10 22:08:46, gary.poster wrote: > Please take a look. this seems like a lot ...
11 years, 2 months ago (2013-01-11 05:29:36 UTC) #3
teknico
Land as is. Very nice, thanks for completing this. https://codereview.appspot.com/7069068/diff/1/app/views/utils.js File app/views/utils.js (right): https://codereview.appspot.com/7069068/diff/1/app/views/utils.js#newcode666 app/views/utils.js:666: ...
11 years, 2 months ago (2013-01-11 11:56:51 UTC) #4
hazmat
On 2013/01/11 05:29:36, hazmat wrote: > On 2013/01/10 22:08:46, gary.poster wrote: > > Please take ...
11 years, 2 months ago (2013-01-11 12:51:03 UTC) #5
gary.poster
On 2013/01/11 00:59:15, matthew.scott wrote: > This looks good and works well. Land as is, ...
11 years, 2 months ago (2013-01-11 13:24:17 UTC) #6
gary.poster
On 2013/01/11 12:51:03, hazmat wrote: > On 2013/01/11 05:29:36, hazmat wrote: > > On 2013/01/10 ...
11 years, 2 months ago (2013-01-11 13:44:38 UTC) #7
hazmat
crappy network ate my first response, redo with brevity. On 2013/01/11 13:44:38, gary.poster wrote: > ...
11 years, 2 months ago (2013-01-11 17:22:53 UTC) #8
gary.poster
On 2013/01/11 17:22:53, hazmat wrote: > crappy network ate my first response, redo with brevity. ...
11 years, 2 months ago (2013-01-11 18:54:59 UTC) #9
hazmat
11 years, 2 months ago (2013-01-11 20:15:51 UTC) #10
On Fri, Jan 11, 2013 at 12:54 PM, <gary.poster@canonical.com> wrote:

> On 2013/01/11 17:22:53, hazmat wrote:
>
>> crappy network ate my first response, redo with brevity.
>>
>
>  On 2013/01/11 13:44:38, gary.poster 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.
>
>  the purpose of content in the db is for presentation to the user, else
>>
> it has no
>
>> purpose being in the client side db.
>>
>
> Certainly.  However, we have a clear model/view distinction in our code.
>  The db/delta/model's job is a simple one at the moment: synchronize the
> client db with the server db.  Any one-off customizations from that
> mission are a maintenance burden.


all code is a maintenance burden, that's why its generally better to do
something once then repeat yourself in code (DRY).


>  Neither of us have proof that one
> approach's maintenance burden is greater over time.


The number of touchpoints with the current implementation is clearly
greater than the alternative i've posited, and will continue to grow as we
introduce additional views. Moreover it introduces valid paths to data that
we want to hide that we're not exercising or testing, for example direct
navigation to the gui url service, etc, that are not being accounted for.
All of those issues would disappear with the alternative implementation
path.

As I said, IME
> moving these kinds of very-high-level UI decisions down into the db
> level is a mistake.  You have a different perspective.


>  the problem with this is a view level feature is that it has to be
>>
> implemented
>
>> everywhere we render a service. If we grow new views we have to
>>
> duplicate the
>
>> same filtering code there as well.
>>
>
> With trivial and already-implemented code reuse.  Moreover, your "grow
> new views" story is a less concrete example than the one I gave, with no
> user story associated with it.
>
>
I'm finding it quite hard to reasonably argue with a perspective that's
defending conditionals throughout the view code, that's feels like the road
to spaghetti. I don't find the maintenance question or violation of DRY a
matter of subjective opinion just a simple fact.

We've discussed a number of alternative views over the course of the
project, from timeline views to aggregate costs which have quite a bit more
value then showing hidden services. that's still an area of active
exploration much more so than hidden service display, which is a non issue,
and will likely become obsolete as we move forward with jaas.


>
>   From a maintenance and efficiency perspective
>> it seems a clear win to me, to implement once vs. implement n.
>>
>
> In terms of code efficiency, it is true that simply not rendering the
> GUI service rather than hiding it would reduce some computation.
> However, that appears to be a negligible increase as I understand the
> code paths.
>
> In terms of developer efficiency, I think it is a matter of opinion,
> given the current state of the app and future plans.


not having to think about service hiding when writing views involving
services is a win in developer efficiency i think.

the efficiency wins are very minor by themselves but i think we've
 identified operational, developer, and runtime memory efficiency wins. all
minor but coupled with the code repetition/dry violation, accidental
information disclosures (service gui url), and a clear alternative that
addresses all of these points imo its hard to understand what the
comparative value is of the current implementation.

>
>  >
>> > - 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.
>>
>
>  that future use case isn't well defined or even clear that its valid.
>>
> in future
>
>> with jaas, the typical environment won't have these services.
>>
>
> It's one example story, which does have a real and reasonable use case
> associated with it, whether or not we choose to pursue it.  The point is
> that it is a good example of other stories for which keeping
> model/db/sync code simple and direct and accurate is valuable.


its a story and use case that will be historical in nature, if we deploy
the gui with state servers or in jaas mode, its not useful to visualize it
separately. The  exception is an application config policy or state for
making them visible which amounts to the same between the two
implementations, ie the utility filter code is conditionalized by
application state/config.

>
>
>  >
>> > >
>> > > 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.
>> >
>>
>
>  regardless of usage location, the fact that these only operated on
>>
> model
>
>> attributes (or in future app state) was my differentor to definition
>>
> location.
>
>> its a minor though, implementor's choice on that one.
>>
>
> View code works with model objects also.  Moreover, these utilities also
> operate on the controller-ish BoundingBox objects as well as models.
>
>

>  > >
>> > > 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.
>> >
>>
>
>  i think this one is more an architectural/maintenance question then
>>
> the normal
>
>> implementor's choice. i'd like to do one more round of discussion
>>
> before we
>
>> resolve via other mech.
>>
>
> I have a meta concern.
>
>
i'll think on this one a bit more and reply separately. perhaps we should
have a call to discuss.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b