UIOptions code review review - FLUID-4171

Antranig Basman antranig.basman at colorado.edu
Wed May 11 05:09:50 UTC 2011


Hi there - this is primarily for Cindy, Michelle and Justin, who are currently collaborating on preparing 
UIOptions for the 1.4 release at the end of the month. Justin has given the pull request 38 one round of 
comments at https://github.com/fluid-project/infusion/pull/38/commits - I think all of the comments which 
are attached to individual are basically sound, and carrying the component in the right direction. I wanted 
to comment more globally on the component and its strategy since there are a few areas in which it is still 
some way from the form we would want.

I will be commenting on the FLUID-4171 branch at revision 5f15d2 - which can be browsed at
https://github.com/cindyli/infusion/blob/5f15d26cd7710b080ba2a3fff312471be6ea4549/src/webapp/components/uiOptions/js/UIOptions.js

The first dodgy area is in the finalInit function lines 69-73 -
         var sliderOptions = that.options.sliderOptions;
         sliderOptions.value = that.model.value;
         sliderOptions.min = that.model.min;
         sliderOptions.max = that.model.max;

"State-shuffling" code of this kind is always suspicious - whenever you see long chains of assignments be 
sure that the framework can be doing work for you. In general the textFieldSlider should be making more use 
of the changeApplier and framework facilities for dealing with models.

i) These 4 lines should just be replaced by a call to fluid.merge or jQuery.extend - such as
var sliderOptions = $.extend(true, {}, that.options.sliderOptions, that.model);
  If necessary, the specific layout which is of interest being copied can be noted in a comment, in case 
there is a worry about the looseness of the contract of aligning the model layout of the component with the 
options accepted by jQuery UI slider.

ii) The event "modelChanged" on the textfieldSlider should be removed, and all the collaborating code should 
just make use of the events fired from the changeApplier instead (which also happens to be named "modelChanged")

iii) The code packaged in textfield.finalInit lines 103-115 should be broken out into a free function which 
performs validation for the model state of the slider. The best way to deliver this would be as a "guard" 
for the changeApplier - some docs are at http://wiki.fluidproject.org/display/fluid/ChangeApplier+API and 
also the relevant behaviour may be seen in the test cases for the changeApplier in DataBindingTests.js lines 
173-208.

Moving on to UIOptions itself - its handling of model state, especially from interacting with UIEnhancer, is 
a little odd. In general, this component needs to be completely generic, and should be free of bits of 
configuration freely floating at top level.

iv) So, the treatment of "textSize" and "lineSpacing" at lines 222-228 is a bit odd. This state should 
instead form part of the configuration of subcomponents of uiOptions which are nested within "controls". The 
"current state" of textSize as well as its bounds should be factored into a self-contained component with 
well-defined model state. Perhaps something like a "boundedRangeOption" component which in addition manages 
a) construction of component tree
         b) selector binding to markup in the controls
         c) bounds for the range, and
         d) model state

So, in the current design, configuration relating to "textSize" is scattered across the whole component. The 
fragments are held at lines 213, 222-225, 352 and 472-475 - these should all be held in one place in one 
subcomponent.

Always ask yourself - if someone wants to come and modify my component, for example, by creating a new kind 
of option - how many places will they need to touch my code? The answer right now is that they need to edit 
the physical implementation of UIOptions itself in 4 places. In the ideal design they would not need to edit 
the implementation at all, but simply supply different IoC configuration to UIOptions - by adding a new 
subcomponent. Clearly the same goes for lineSpacing, which will become a component of the same type, and all 
the boolean-valued options, etc.

v) This refactoring will extend out into the UIEnhancer as well, since these "view setting subcomponents" 
will to some extent be autonomous ("addStyles", "styleLinks" etc. are all still hardbaked like last month's 
bread). A big oddity with our current design which I noticed when working with Clayton is that the 
"defaultSiteSettings" record is held in an odd place - in UIEnhancer. It really belongs with the component 
holding the state - either UIOptions or in a newly created subcomponent of it.

vi) As I think we had already planned, UIEnhancer also needs to be converted over to IoC - for example, the 
ToCSetting logic needs to be moved out into an IoC-configured subcomponent, and just like UIOptions itself, 
UIEnhancer needs to be freed of all non-generic configuration appearing at top level. Rather than using 
jQuery.data, this impl should instead use the static environment to store the enhancer.

vii) Should we get through all of this, we can talk about the non-standard construction workflow of 
UIEnhancer which I think came up today. Certainly it is unusual in that it accepts a document as first 
argument rather than a container. This would best be dealt with by moving out responsibility for indirecting 
on the document etc. to a supercomponent of UIEnhancer and returning UIEnhancer to be a standard view 
component, accepting as its first argument the value which currently ends up manually on that.container on 
line 225.

viii) In the "modern style", throughout both of these files, closure-private utility functions should be 
replaced by publically namespaced free functions.

Good luck - we should talk over these changes some more at the dev meeting.



More information about the fluid-work mailing list