On Fri, Nov 20, 2009 at 5:42 PM, P T Withington <[email protected]>wrote:
> I don't like the idea of ClassCompiler.updateSchema not calling its super. > This makes me think there is something wrong with our modularization. > Maybe you should leave the super call and conditionalize the code in > ViewCompiler to not run on subclasses? > Hmm, yeah I was waffling about whether to put a conditional in the ViewCompiler.updateSchema call,I could put back the super call and make ViewCompiler check for whether a class or instance is being compiled. > > tagOrClassName and getClassModel together duplicate (but with less error > checking, and I think with a bug) getParentClassModel. Consider a class > definition that does not specify extends because it extends the default > class, view. tagOrClassName will say it's a 'class' when it's really a > 'view'. I think getClassModel should be replaced by getParentClassModel. > OK I will look at consolidating these > I'm confused about the use of tagOrClassName in checkRequiredAttributes. > If I am defining a new class, is it checking that my definition has the > required attributes of a <class> or of my superclass? > Good question! > > At this point, I think I hit some merge errors. More when you have an > updated review. > OK I just sent an update that will merge properly, but that was before I got these comments. I'll make some more changes and send another review, probably late tonite. > > On 2009-11-20, at 10:23, Henry Minsky wrote: > > > Change 20091120-hqm-i by [email protected] on 2009-11-20 08:58:31 EST > > in /Users/hqm/openlaszlo/trunk > > for http://svn.openlaszlo.org/openlaszlo/trunk > > > > Summary: support mixins on instances > > > > New Features: > > > > Bugs Fixed: LPP-8602 Allow with="" on instances declarations > > > > Technical Reviewer: ptw > > QA Reviewer: max > > Doc Reviewer: (pending) > > > > Documentation: > > > > Release Notes: > > > > mixins are supported now on instances as well as classes, e.g., > > > > <mixin name="textmixin"> > > <attribute name="foo" value="bar" type="text"/> > > </mixin> > > > > <text name="mixinstance" with="textmixin"> > > <attribute name="text" value="${this.foo}"/> > > </text> > > > > Overview: > > > > > > Details: > > > > > > ClassCompiler.java: remove call to super.updateSchema, since we made > > ViewCompiler's updateSchema now do some things that only apply to > > instances. > > > > schema/lfc-undeclared.lzx: Moved the "with" atribute down from <class> to > <node> > > beause instances can now have mixins > > > > ViewCompiler.java: add updateSchema method, so that instances with mixins > will > > call the ClassModel machinery to add the needed interstitial classes to > the app. > > This is done by rewriting the instance as a <anonymous > extends="tagclass"> instance, > > and then ClassModel and NodeModel have been modified to know how to deal > with these > > 'class-like instances' > > > > Also removed call to the class inlining code that is no longer used in > > the compiler. > > > > > > ToplevelCompiler.java: use generalized 'tagname' accessor to get the > > classname of an instance, since an element may be an anonymous instance > > class. > > > > > > ViewSchema.java: add an explicit arg to say if we're defining a public > class or a > > private (anonymous instance) one > > > > NodeModel.java: Since instance classes may now be given a 'anonymous' > > tag, define generalized accessor tagOrClassName which returns the > > value of 'extends' if it exists, otherwise return the tag name > > > > > > Compiler.java: remove some of the class inlining code that has not worked > in forever > > > > DebugCompiler.java: remove class inlining code > > > > > > ClassModel.java: make the ClassModel constructor accept a <anonymous> tag > that has mixins, and build the > > interstitial classes just like for a <class> that has mixins. > > > > > > > > > > > > > > Tests: > > > > test/lztest/lztest-mixins.lzx added to the "ant lztest" suite > > > > test/smoke/mixin-simple.lzx > > > > testcase from bug (uncomment the commented out region) > > > > > > Files: > > M test/lztest/rhino.txt > > A test/lztest/lztest-mixins.lzx > > A test/smoke/mixin-simple.lzx > > M WEB-INF/lps/schema/lfc-undeclared.lzx > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassCompiler.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ViewCompiler.java > > M > WEB-INF/lps/server/src/org/openlaszlo/compiler/ToplevelCompiler.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ViewSchema.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/Compiler.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/DebugCompiler.java > > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassModel.java > > > > Changeset: > http://svn.openlaszlo.org/openlaszlo/patches/20091120-hqm-i.tar > > > > _______________________________________________ > > Laszlo-reviews mailing list > > [email protected] > > http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews > > -- Henry Minsky Software Architect [email protected]
_______________________________________________ Laszlo-reviews mailing list [email protected] http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews
