|
|
DescriptionMake 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
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
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 File app/views/topology/relation.js (right): https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js#... app/views/topology/relation.js:101: }).length; Given the function in utils.js, a lighter weight solution might be http://pastebin.ubuntu.com/1518726/ - feel free to ignore, though. 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#newcode652 app/views/utils.js:652: }; Thanks for the comment!
Sign in to reply to this message.
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.
Sign in to reply to this message.
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: return charmUrl && utils.isGuiCharmUrl(charmUrl); So it's ok that this can happen, good. Yesterday I was not sure about it.
Sign in to reply to this message.
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. the utility functions here feel like they should be in model.utils instead of view.utils. needs fixing, imo.
Sign in to reply to this message.
On 2013/01/11 00:59:15, matthew.scott wrote: > 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 > File app/views/topology/relation.js (right): > > https://codereview.appspot.com/7069068/diff/1/app/views/topology/relation.js#... > app/views/topology/relation.js:101: }).length; > Given the function in utils.js, a lighter weight solution might be > http://pastebin.ubuntu.com/1518726/ - feel free to ignore, though. Nice change. I like it, and will use it. Thank you. Thanks for the review! Gary > > 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#newcode652 > app/views/utils.js:652: }; > Thanks for the comment!
Sign in to reply to this message.
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
Sign in to reply to this message.
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. 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. From a maintenance and efficiency perspective it seems a clear win to me, to implement once vs. implement n. > > - 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. > > > > > 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. > > > > 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. thanks, Kapil
Sign in to reply to this message.
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. Neither of us have proof that one approach's maintenance burden is greater over time. 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. > 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. > > > > > - 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. > > > > > > > > > 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. We want to have high-quality code and design. We should try not to achieve that via rework requests. I feel we have had too many rework requests in our project's history, as most recently demonstrated by this branch and the one for https://bugs.launchpad.net/juju-gui/+bug/1074423 . We ask for rework during reviews. Reviews are one of the tools we have to encourage high-quality code. They are also among the most expensive tools when they ask for rework, rather than small changes. Reworking is expensive in terms of time, money, workflow, and even morale. Asking for rework should be a red flag for a possibly broken process, and a suggestion that we are not using our other tools for quality as well as we might. It should also be challenged: is the rework request actually necessary? What are our other tools for quality? - We clearly write down our broad expectations and standards: tests need to pass, code needs to conform to style guides, and so on. We document those in our docs/process.rst right now. Robert Collins documented his SOA expectations at https://dev.launchpad.net/ArchitectureGuide/ServicesRequirements . These sorts of expectations ideally should be SMART-style--measurable, achievable, realistic, and so on. Vague expectations like "KISS" are a lot harder to expect much from here. - We also clearly write down our individual tasks in bugs and kanban cards. When I write a bug, I try to specify the goals for that bug. Ideally, the bug is written such that it specifies necessary goals and leaves unspecified everything else, for the implementer to decide. Ideally, if a solution conforms to the written goals and our written standards, it will pass review. - Prototypes and pre-implementation calls are the best times to try and direct KISS and other vaguer ideas. The less one is involved in these tools, the less one can expect from the results If an issue isn't caught by these tools, and leads to a potential rework request in a code review, the first thing we should ask is if we could have used those other tools better. The second thing we should ask is whether we are confident that the rework request is worth the cost. Sometimes it is worth the cost. For what it is worth, we have one other tool, which in part is supposed to reduce the cost of rework requests: keeping our individual tasks as small as possible, and trying to deliver value as quickly as possible. If we are going down a wrong path, it's better to know sooner rather than later. If we only have to rework a day or two's worth of work, as with this branch, because we are landing a small branch soon, that's a lot better than reworking a week's or a month's worth of work. But the cost of rework is still high, it's still quite possibly indicative of a process problem, and it still should be challenged. So, perhaps this branch is worth a rework request to you. I ask that you please wield these requests with great care, and each time you are tempted to use them, consider whether it indicates an improvement we should make in our process and tools, and whether rework is really worth the cost. Thanks Gary
Sign in to reply to this message.
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.
|