Apologies that I'm waaaay late to this party, but the whole template search/replace thing doesn't seem ideal in that something like a typo in the template name could cause some seriously hard-to-debug code.
Did you consider the case that if a property provider returns null, then the fallback would get used? I like that idea because (1) it doesn't rely on textual search/replace and (2) it seems like a good pattern to form the basis of property provider chaining, which is a long-standing problem for properties that get extended. That is, if a derived module extends a property, it usually wants to also extend the property provider, which today involves brittle copy/paste. It would be great to figure out some protocol for calling each property provider -- most-derived to least -- such that the first one that return non-null is taken to be the official answer, and returning null means "ask the next most derived provider for this same property". -- Bruce On Mon, Jun 15, 2009 at 11:03 PM, Freeland Abbott <fabb...@google.com>wrote: > Revision to the concept, for design review: See > http://code.google.com/p/google-web-toolkit/wiki/DefaultLocaleBinding, but > the short-and-sweet is: > > - Introduction of XML tag <set-property-fallback name="*propname*" > value="*fallbackString*"/>. It's fine, and expected, to set this zero, > one, or many times for any given property. > - A new field gets added in BindingProperty, with setter and getter, > and a default value of empty string > - getPropertyProvider() is rewritten to return, not the literal > property provider string, but the result of replacing all instances of > /*-FALLBACK-*/ with the fallback string, whether empty or otherwise. > - For my default locale use case, our I18N.gwt.xml sets the locale > fallback to "default", and user code sets it to something real if the user > doesn't like that. > > > On Wed, Jun 10, 2009 at 11:49 AM, Freeland Abbott <fabb...@google.com>wrote: > >> Yes, he and I already discussed it. I was initially trying to avoid >> needing a new XML tag when the existing one was such a near fit, but the >> legacy linker support issues convinced me we needed it. >> >> >> >> >> On Tue, Jun 9, 2009 at 5:37 PM, Bruce Johnson <br...@google.com> wrote: >> >>> I like what jat said. Freeland? >>> >>> On Tue, Jun 9, 2009 at 5:33 PM, <j...@google.com> wrote: >>> >>>> >>>> The alternative to this would be Bruce's suggestion of defining a >>>> specific fallback value for a selection property rather than using >>>> config properties for it. That would narrow the scope to exactly what >>>> we know we want to support and simplify the linker changes and avoids >>>> the breaking API change for existing linkers (where they have to supply >>>> the config properties to avoid breaking, which means the same linker >>>> can't work for GWT 1.6 and GWT 2.0). >>>> >>>> Something like: >>>> <set-property-fallback name="locale" value="default/> >>>> and the last set value wins. >>>> >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/2 >>>> File >>>> >>>> dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java >>>> (right): >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/2#newcode194 >>>> Line 194: new TreeSet<ConfigurationProperty>()); >>>> It seems like allowing this could cause incorrect behavior. I know it >>>> is a tradeoff of not breaking existing code, but if there is a linker >>>> which calls this method, any code depending on config property lookup >>>> will just break. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/2#newcode223 >>>> Line 223: } >>>> It looks like we are just substituting the empty string for unknown >>>> properties, which is especially bad if the call is relayed through the >>>> above method. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/4 >>>> File dev/core/src/com/google/gwt/dev/cfg/StaticPropertyOracle.java >>>> (right): >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/4#newcode149 >>>> Line 149: for (String v : prop.getAllowedValues()) { >>>> setValues.addAll(cprop.getAllowedValues())? >>>> >>>> Also, maybe this should be computed in getPossibleValues() instead. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/5 >>>> File >>>> dev/core/src/com/google/gwt/dev/shell/ModuleSpacePropertyOracle.java >>>> (right): >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/5#newcode127 >>>> Line 127: for (String v : cprop.getAllowedValues()) { >>>> setValues.addAll(cprop.getAllowedValues())? >>>> >>>> Also, maybe this should be computed in getPossibleValues() instead. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/7 >>>> File user/src/com/google/gwt/i18n/I18N.gwt.xml (right): >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/7#newcode51 >>>> Line 51: $wnd['__gwt_Locale'] = locale || >>>> '/*-GWTCONFIGPROP(default.locale)-*/'; >>>> I think we haven't nailed this down enough that we want anyone else >>>> using it and expecting it to keep working. So, I would suggest a >>>> comment here to that effect. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/7#newcode55 >>>> Line 55: return "/*-GWTCONFIGPROP(default.locale)-*/"; >>>> Since these can all be empty strings, I would suggest storing >>>> "/*-GWTCONFIGPROP(default.locale)-*/" || 'default' in a variable and >>>> reusing that. Things will fall over if the property provider returns a >>>> value that is not valid. >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/8 >>>> File >>>> user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java >>>> (right): >>>> >>>> http://gwt-code-reviews.appspot.com/34832/diff/1/8#newcode136 >>>> Line 136: logger.log(Type.WARN, "default.locale has more than one value, >>>> using " >>>> Is this even possible since the config property definition says it isn't >>>> multivalued? >>>> >>>> http://gwt-code-reviews.appspot.com/34832 >>>> >>>> >>>> >>>> >>> >> > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
