[Laszlo-reviews] UPDATE For Review: Change 20100604-ptw-7 Summary: Remove earlySetters kludges

[P T Withington]

[UPDATED to address André's comments]

On 2010-06-08, at 18:38, André Bargull wrote:

>> +      } else if (key == 'name') {
>> +        // `name` needs to be set ASAP
>> +        this.$lzc$set_name(args.name);
> 
> Performing this check costs an extra string comparison for each loop iteration. If the name needs to be set as soon as possible, it should be done before entering the loop.

I moved it out of the loop.  I have to delete the name from the args, so it won't get re-processed by the setter clause in the loop.

>> +      var p = this.parent;
>> +      if ($classrootdepth) {
>> +        while (--$classrootdepth > 0) { p = p.parent }
>> +      }
>> +      this.classroot = p;
> 
> Change introduces different semantics here, classroot is set to parent even if args.$classrootdepth is 0 (or any other 'falsy' value).

Mis-placed close brace.  I moved the setting of classroot inside the conditional.  But, this is a private API between the tag compiler and the kernel.  We could allow 0 here, so that it is possible to use `classroot` in the root class (which would also require fixing the node compiler).

>> +    if (('$datapath' in args) && (args.$datapath !== ignore)) {
>> +      var $datapath = args.$datapath;
>> +      args.$datapath = ignore;
> 
> Why do you set args.$datapath to ignore?

Mis-apprehension that it was being processed again.  I have removed that statement.

>> -  /** @access private */
>> -  var handlerMethodNames;
> 
> How did handlerMethodNames actually work? It's filled in lz.State#set_$delegates() which is called in lz.Node#applyArgs() which in turn gets called at the end of lz.State#applyArgs(). But lz.State#applyArgs() is already using handlerMethodNames right at the beginning, at that time it should still be empty, shouldn't it?

I don't believe it did.  That's why I removed it.

>> +  /** @access private */
>> +  public function LzState ( parent:* , attrs:* , children:* = null, instcall:*  = null) {
>> +    // Must init before super because used by __LZapplyArgs override
>> +    // which will be called from super!
>> +    this.heldArgs = {};
>> +    this.appliedChildren = [];
>> +    super(parent,attrs,children,instcall);
>> +  }
> 
> I don't understand the motivation for this change. And also: http://openlaszlo.org/pipermail/laszlo-dev/2008-July/016411.html

I've changed the comment to read:

    // TODO: [2010-06-09 ptw] (LPP-5232) Move these initializations to
    // the declaration when LPP-5232 is fixed.  (Note that super calls
    // our override of __LZapplyArgs, which uses these instance
    // variables.)

>> +  /*
>> +   * Special flags to keep these psuedo-args from being processed
>> +   */
> 
> "pseudo" ;-)

Fixed

>> +  /*
>> +   * Special flags to keep these psuedo-args from being processed
>> +   */
> 
> [...] from being processed "by __LZapplyArgs()"

Fixed

Change 20100604-ptw-7 by ptw at padme.home on 2010-06-04 08:47:02 EDT
    in /Users/ptw/OpenLaszlo/trunk
    for http://svn.openlaszlo.org/openlaszlo/trunk

Summary: Remove earlySetters kludges

Bugs Fixed:  LPP-7354 Presentation Types (partial substrate cleanup)

Technical Reviewer: andre.bargull at udo.edu (pending)
QA Reviewer: max at openlaszlo.org (pending)

Overview:

    `earlySetters` were an attempt by Adam to make a general mechanism,
    but each of the actual usages is a special case.  We don't support
    this generality outside of the kernel (it is handled instead by
    overriding the construct method), so I'm removing it.  It's just a
    lot of extra fragile mechanism that makes the init args more
    difficult to process and interferes with attribute annotation.

Details:

    LzNode, LaszloView, LaszloCanvas:  remove earlySetters lists.

    LzNode: Remove unused `doneClassRoot` flag, unused `defaultSet`
    method.  Handle the previous earlySetters explicitly in
    __LZapplyArgs.  For those that are only used to communicate
    between the compiler and the runtime ($delegates, $classrootdepth,
    $datapath), hoist the former setters inline (but keep the `-1`
    sentinel that indicates they are handled specially in
    __LZapplyArgs).

    LaszloView: Handle `clickregion`, formerly an earlySetter, in
    construct.

    LzState: Clean up class organization to match standard order.
    Remove unneeded initial value for __LZpool.  Make the constructor
    do something useful.  Hide unnecessary documentation of implicit
    event.  Remove 4.1-era deprecated API's.  Handle sorting
    `$delegates` between self and parent explicitly in __LZapplyArgs

Tests:
    smokecheck, selected demos

Files:
M       WEB-INF/lps/lfc/core/LzNode.lzs
M       WEB-INF/lps/lfc/views/LaszloView.lzs
M       WEB-INF/lps/lfc/views/LaszloCanvas.lzs
M       WEB-INF/lps/lfc/helpers/LzState.lzs

Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20100604-ptw-7.tar