[commits-cosmo] (br) [3041] some enhancements to ModalDialog:

Matthew Eernisse mde at osafoundation.org
Mon Dec 11 16:07:59 PST 2006


Bobby,

The function call chain needs to go back the way it was. At bare minimum 
you need to wait for a return value from the code that does the 
rendering and centering stuff before you move on.

I coded it like that to fix a bug where the dialog box would sometimes 
flash visible briefly before getting properly positioned in the center 
of the screen. (I've also seen this happen with the stock Dojo modal 
dialog widget -- ask Adam about his Stickies app.)

This problem is easily fixed by making sure that the different DOM 
operations occur serially -- i.e., that the box is completely rendered 
and centered before the 'display' attribute is set to 'block.' One way 
to do that is to wait for a return value from the function calls in 
question.

So that code was not trying to check if the code fails, it's making sure 
stuff is really done before moving on.

Using that final 'return true', the only time the functions will not 
actually return at all is if the code in the function cannot 
successfully execute -- in which case the JS interpreter will error out 
-- exactly the same way it would without return values.

Waiting for return values doesn't make it any more (or less) likely that 
the code will silently fail than just calling them one by one like in 
your rewrite. All it does is make sure that one is actually completely 
done before you start the next.

I'm guessing from your commit message that you have a misunderstanding 
of how function call return values work in JavaScript. If a function 
does not explicitly return either true, or an object that autocasts to 
true, the function will evaluate to false.

Not returning anything means the function returns a value of 
'undefined,' which also evaluates to false.

js> typeof function () {}();
undefined
js> (function () {}()) ? true : false;
false

You don't have to return false explicitly for the function to evaluate 
to false.

If you see something in the code that does not immediately make sense to 
you, it might actually just be badly written code -- or there might be a 
reason that it's written that way. Unless it's something seriously 
critical, it might be a good idea wait until you can ask the responsible 
party what's going on in that part of the code before you start 
rewriting stuff.


Matthew



svncheckin at osafoundation.org wrote:
> Revision
>     3041 <http://cvs.osafoundation.org/viewcvs.cgi?rev=3041&view=rev>
> Author
>     br
> Date
>     2006-12-08 17:39:06 -0800 (Fri, 08 Dec 2006)
> 
> 
>       Log Message
> 
> some enhancements to ModalDialog:
> 
> * allow for passing another widget as the content parameter
> * removed function calls that were chained like "doThis() && doThat() && 
> doThose()", and put in conditionals.
> This only makes sense if those things ever return false, but they all 
> always return true, so why put them
> in a conditional? If you want to check if code fails, raise an error and 
> catch it or something.
> This style, you just silently fail.
> 
> 
>       Modified Paths
> 
>     * cosmo/trunk/src/main/webapp/js/cosmo/ui/widget/ModalDialog.js
>       <#cosmotrunksrcmainwebappjscosmouiwidgetModalDialogjs>
>     * cosmo/trunk/src/test/unit/js/cosmo/ui/widget/modalDialogTester.html
>       <#cosmotrunksrctestunitjscosmouiwidgetmodalDialogTesterhtml>
> 
> 
>       Diff
> 
> 
>         Modified:
>         cosmo/trunk/src/main/webapp/js/cosmo/ui/widget/ModalDialog.js
>         (3040 => 3041)
> 
> --- cosmo/trunk/src/main/webapp/js/cosmo/ui/widget/ModalDialog.js	2006-12-08 22:16:22 UTC (rev 3040)
> +++ cosmo/trunk/src/main/webapp/js/cosmo/ui/widget/ModalDialog.js	2006-12-09 01:39:06 UTC (rev 3041)
> @@ -122,19 +122,22 @@
>              }
>              return true;
>          },
> +        _removeChildren: function(node){
> +            while(node.firstChild) {
> +                node.removeChild(node.firstChild);
> +            }
> +        },
>          setContent: function (content) {
>              this.content = content || this.content;
>              // Content area
>              if (typeof this.content == 'string') {
>                  this.contentNode.innerHTML = this.content;
> -            }
> -            else {
> -                var ch = this.contentNode.firstChild;
> -                while(ch) {
> -                    this.contentNode.removeChild(ch);
> -                    ch = this.contentNode.firstChild;
> -                }
> +            } else if (dojo.html.isNode(this.content)) {
> +                this._removeChildren(this.contentNode);
>                  this.contentNode.appendChild(this.content);
> +            } else if (this.content instanceof dojo.widget.HtmlWidget){
> +                this._removeChildren(this.contentNode);
> +                this.contentNode.appendChild(this.content.domNode);
>              }
>              return true;
>          },
> @@ -165,10 +168,11 @@
>              return true;
>          },
>          render: function () {
> -            return (this.setTitle() &&
> -                this.setPrompt() &&
> -                this.setContent() &&
> -                this.setButtons());
> +            this.setTitle();
> +            this.setPrompt();
> +            this.setContent();
> +            this.setButtons();
> +            return true;
>          },
>          center: function () {
>              var w = dojo.html.getViewport().width;
> @@ -194,7 +198,7 @@
>              return true;
>          },
>          
> -        // Lifecycle crap
> +        // Lifecycle functions
>          postMixInProperties: function () {
>              this.toggleObj =
>                  dojo.lfx.toggle[this.toggle] || dojo.lfx.toggle.plain;
> @@ -225,27 +229,25 @@
>                      this.title = title || this.title;
>                      this.prompt = prompt || this.prompt;
>                  }
> +
>                  // Sizing
>                  this.width = this.width || DIALOG_BOX_WIDTH;
>                  this.height = this.height || DIALOG_BOX_HEIGHT;
>                  this.setWidth(this.width);
>                  this.setHeight(this.height);
>                  
> -                // Don't display until rendered and centered
> -                if (this.render() && this.center() && this.renderUiMask()) { 
> -                    this.domNode.style.display = 'block';
> -                    this.domNode.style.zIndex = 2000;
> -                    // Have to measure for content area height once div is actually on the page
> -                    this.setContentAreaHeight();
> -                    // Call the original Dojo show method
> -                    this.showOrig.apply(this);
> -                    this.isDisplayed = true;
> -                }
> +                this.render();
> +                this.center();
> +                this.renderUiMask();
> +                this.domNode.style.display = 'block';
> +                this.domNode.style.zIndex = 2000;
> +
> +                // Have to measure for content area height once div is actually on the page
> +                this.setContentAreaHeight();
> +                this.isDisplayed = true;
> +
>              };
>  
> -            // reference to original hide method
> -            this.hideOrig = this.hide; 
> -
>              // Clear buttons and actually take the div off the page
>              this.hide = function () {
>                 
> @@ -258,9 +260,11 @@
>                      }
>                      this.btnPanel.destroy();
>                  }
> -
> -                // Call the original Dojo hide method
> -                this.hideOrig.apply(this);
> + 
> +                if (this.content instanceof dojo.widget.HtmlWidget){
> +                    this.content.destroy();
> +                }
> +                
>                  this.content = null;
>                  this.btnsLeft = [];
>                  this.btnsCenter = [];
> 
> 
>         Modified:
>         cosmo/trunk/src/test/unit/js/cosmo/ui/widget/modalDialogTester.html
>         (3040 => 3041)
> 
> --- cosmo/trunk/src/test/unit/js/cosmo/ui/widget/modalDialogTester.html	2006-12-08 22:16:22 UTC (rev 3040)
> +++ cosmo/trunk/src/test/unit/js/cosmo/ui/widget/modalDialogTester.html	2006-12-09 01:39:06 UTC (rev 3041)
> @@ -56,6 +56,37 @@
>              }
>          );
>      }
> +
> +    showDOMContentDialog = function(){
> +        var contentNode = document.createElement("span");
> +        contentNode.appendChild(document.createTextNode("I am such a text node!"));
> +        cosmo.app.showDialog({
> +            content: contentNode,
> +            btnsLeft: [ dojo.widget.createWidget(
> +                          "cosmo:Button",
> +                          {  text: "Bye Bye",
> +                             width: "100px",
> +                             handleOnClick: cosmo.app.hideDialog,
> +                             small: false },
> +                           null, 'last')]
> +            }
> +        );
> +    }
> +    
> +    showWidgetContentDialog = function(){
> +        dojo.require("dojo.widget.Clock");
> +        cosmo.app.showDialog({
> +            content: dojo.widget.createWidget("Clock",{},null, "last"),
> +            btnsLeft: [ dojo.widget.createWidget(
> +                          "cosmo:Button",
> +                          {  text: "Bye Bye",
> +                             width: "100px",
> +                             handleOnClick: cosmo.app.hideDialog,
> +                             small: false },
> +                           null, 'last')]
> +            }
> +        );
> +    }
>    </script>
>  
>  
> @@ -66,6 +97,10 @@
>    <div id="showErrorButton"/>
>    <br>
>    <div id="showDialogWithStringContentButton"/>
> +  <br>
> +  <div id="showDialogWithDOMContentButton"/>
> +  <br>
> +  <div id="showDialogWithWidgetContentButton"/>
>  
>    <script language="JavaScript" type="text/javascript">
>         cosmo.app.init();
> @@ -81,6 +116,19 @@
>                      width: "275px",
>                      handleOnClick: showStringContentDialog,
>                      small: false }, dojo.byId("showDialogWithStringContentButton"), 'last');
> +
> +        dojo.widget.createWidget("cosmo:Button",
> +                  { text: "Show a Dialog Box with Content Set By a DOM Node",
> +                    width: "300px",
> +                    handleOnClick: showDOMContentDialog,
> +                    small: false }, dojo.byId("showDialogWithDOMContentButton"), 'last');
> +
> +        dojo.widget.createWidget("cosmo:Button",
> +                  { text: "Show a Dialog Box with Content Set By a Widget",
> +                    width: "300px",
> +                    handleOnClick: showWidgetContentDialog,
> +                    small: false }, dojo.byId("showDialogWithWidgetContentButton"), 'last');
> +
>    </script>
>  
>  </body>
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Commits-Cosmo mailing list
> Commits-Cosmo at osafoundation.org
> http://lists.osafoundation.org/cgi-bin/mailman/listinfo/commits-cosmo



More information about the Commits-Cosmo mailing list