Re: Re: Problem in doSubmitAndLock()

At 04:17 PM 3/25/2005 -0500, you wrote:
>Hi Jake,
>
>Jacob Kjome wrote:
>>
>>Hi Christian,
>>
>>The fix in HTMLActionRenderer is probably fine, but I'm not particularly 
>>satisfied with the fix to FormControl.js.  It is now coupled to the JSRS 
>>stuff.  FormControl.js is meant to live on its own.  It is reusable.  It 
>>should not have any direct knowledge of any other external script.  Any 
>>fix that makes it have knowledge is wrong.  While your fix makes things 
>>work for you, it isn't clean.  As I mentioned in a direct email to you 
>>(since the list was down for a while yesterday and today), there are two 
>>possible fixes....
>Well I wasn't aware that FormControl.js was meant to live on its own. The 
>fact that our renderer is embedding calls like this:
>
>         doSubmitAndLock(this.form,cshl_SubmitAndReplace)into the action 
> seems to have more or less the same effect in my opinion. I guess I'm not 
> overly concerned about having the 2 pieces of scripting logic connected - 
> what I am explicitly trying to handle is the situation where a form gets 
> submitted in one and then resubmitted in the other. And what I coded 
> basically does that.

No, your code checks for a specific case and doesn't solve the general 
case.  cshl_SubmitAndReplace just happens to be a method that we have 
chosen to call using the code in FormControl.js.   The doSubmitAndLock code 
need have no knowledge of this.  Are you aware of the API where you can add 
your own functions to this?  See...
http://barracudamvc.org/Barracuda/javadocs/org/enhydra/barracuda/core/comp/BAction.html#addScriptFunction(java.lang.String)

   What begins as doSubmitAndLock(this.form,cshl_SubmitAndReplace) can 
become...

doSubmitAndLock(this.form, customFormChecker1, customFormChecker2, 
anotherArbitraryFuntion3, cshl_SubmitAndReplace)

Don't believe me?  Look at the code...

         Collection scriptFunctions = comp.getScriptFunctions();
         if (scriptFunctions != null) {
             for (Iterator iter = scriptFunctions.iterator(); 
iter.hasNext();) {
                 sb.append(",").append((String)iter.next());
             }
         }
         if (comp.getDisableBackButton()) sb.append(",cshl_SubmitAndReplace");
         sb.append(");");

Note what I say in the javadoc...

<quote name="Jake" methodref="addScriptFunction(String)">
The constraints for these functions is that they take a form element as an 
argument (or no arguments) and that it returns a boolean. These functions 
are called in the order given and can be used for custom client-side form 
validation.
</quote>

So, I actually specified that they all must return a boolean, but the 
FormControl.js code did not originally take into account when a method 
might break this contract.  That's when I added the "undefined" check.  Of 
course then there came up the situation where one of those functions might 
submit() on its own which, with the existing behavior, would cause a double 
submit if the code didn't return a boolean value of "false".  You took care 
of the situation when it happens for the JSRS stuff, but it doesn't fix the 
general case where, say, "anotherArbitraryFunction3" submits without 
returning a boolean "false".

To tell you the truth, I never really thought about the fact that the JSRS 
stuff was submitting before you brought it up, and I definitely assumed 
that even if there was a submit, that it wouldn't bother any other .js code 
because control would move to submitting rather than further .js code.  So 
a double submit was the furthest thing from my mind.  Of course that 
assumption is clearly wrong, which is why we are having this discussion now.

In any case, I disagree strongly that some Java code writing javascript to 
an HTML page that uses a function in the FormControl code and just happens 
to pass along one JSRS function as a parameter by default constitutes 
coupling of FormControl code and JSRS code.  The FormControl stuff *must* 
be there to support the addScriptFunction() functionality of BAction.  The 
JSRS stuff can be removed and FormControl will continue working with no 
knowledge that JSRS stuff is missing or ever existed in the first 
place.  The code you added truly couples FormControl and JSRS and it also 
doesn't solve the general case where an arbitrary function might submit() 
without returning a boolean "false", so the double submit problem still 
exists in the code.

My solution actually follows the BAction.addScriptFunction() documentation 
and solves the general case.  And it does it cleanly!  Note that I use the 
FormControl.js code outside of Barracuda.  It shouldn't know about 
Barracuda or any other framework or external script.  Those currently using 
BAction.addScriptFunction(), if they followed the javadoc, shouldn't be 
bitten at all.  It was an oversight in FormControl.js that we let code that 
didn't return a boolean work in the first place.  So, although it was 
allowed, it went against the documentation and it is more than valid to 
support the documented behavior.



>>1.  Figure out how to detect an arbitrary form submit and use that 
>>knowledge to
>>short circuit any further javascript code evaluation.  If this is 
>>impossible,
>>then....
>>
>>2.  Resign ourselves to enforce a contract where if a method does not 
>>return a
>>boolean value, then we short-circuit the rest of the code.  So, we would 
>>set "ok" to false if the return
>>value is undefined (or, actually, if the return value is anything other 
>>than a
>>boolean... although I haven't thought this through too much yet).
>This is what I'm not following - why do you want to enforce this?

1.  Because it is documented that methods added to 
BAction.addScriptFunction(String) return a boolean.

2.  Your code doesn't solve the general case and couples 2 scripts that 
are, and should stay, separate entities.  FormControl.js was designed, 
originally, outside of Barracuda and was only added to Barracuda when it 
was determined that it could provide a useful function.  It was added 
without modification for Barracuda's sake and Barracuda shouldn't be adding 
stuff to it except under exceptional circumstances where it makes 
sense.  It does not here.

3.  The removal of the JSRS script now requires an update to the 
FormControl script.  Modification of one script or the other may or may not 
affect the other.  This complicates maintenance where there need not be 
complication.


>  I could very easily imagine someone (like myself) writing a javascript 
> function which doesn't return a value, which they then invoke before the 
> submit. Wouldn't it be highly likely that someones going to end up 
> spending a serious amount of time trying to debug that sucker to figure 
> out why the form is not getting submitted? Am I missing something here?

This is the unfortunate consequence of option #2.  However, as I have 
explained, it is clearly documented that functions added to 
BAction.addScriptFunction(String) should be returning a boolean.  If one 
doesn't follow documentation, no matter what the case, then all bets are 
off anyway.  I wish we could ease the documented restriction to match what 
has been (via an oversight) lenient existing behavior.  However, if #1 
cannot be accomplished in a generic way (to account for all cases, not just 
the JSRS script), then #2 is the only option.  Luckily, the change simply 
brings the actually behavior in line with the documentation rather than 
adding new constraints (as you may have perceived).

The change should definitely be added to the release notes to alert those 
upgrading.  However, the fix is very simple and doesn't involve 
recompilation of code since changes would be happening to client-side 
scripting.  It is merely a matter of adding a boolean return value to any 
existing code that does not currently return a boolean.  And for code which 
performs its own submit(), it should be noted to add it as the last 
function to be called, just as the JSRS function call happens last 
currently, or else any other functions to be called after the submitting 
function will be skipped.  This should be apparent anyway because if one 
knows a script will submit early, then they can't expect the bulk of any 
javascript coming later to be executed.

I really do not foresee a lot of confusion happening here, nor do I suspect 
that a lot of code out there is taking advantage of the more lenient 
behavior than that which is documented.  This should be a very minor issue 
if it is one at all.

>>Since I don't believe the first is possible (generically), then the 
>>second is necessary.  Pretty much the only thing that needed to be done 
>>if we bite the bullet on #2 is....
>>
>>change this...
>>if (ok == undefined) ok = true;
>>
>>to this...
>>if (ok == undefined) ok = false;
>Yes, I understand what this will do. I'm just not sure what else its going 
>to break (and that's what concerns me).

Pretty much nothing else as I've stated previously.  It is just unfortunate 
that we can't document the more lenient behavior as being what we support 
since it can no longer be supported given the newly found circumstances.


>>Simple fix, no?  Your more involved fix doesn't take care of the generic 
>>case where someone else's function does a submit() of the form and 
>>neglects to return a boolean value back to FormControl.js.
>I'm not worrying about the case where someone is invoking their own 
>function to submit the form - and I don't think we need to. If they are 
>doing that, I don't see why they would be using FormControl.doSubmit() in 
>the first place. Again, am I missing something here? We seem to have very 
>different ideas of how this stuff is being used...

I'm concerned with following the documented contract and working in a 
consistent way with all code.  I'm also concerned with keeping FormControl 
clean and useful outside Barracuda as well as not creating a dependency 
(and maintenance issue) on code that really ought to go away in any case 
(JSRS).  Besides, how we can you predict how others might code their 
javascript?  There are any number of reasons someone might want to 
pre-emptively submit a form, just as JSRS is doing.  Maybe they have their 
own custom actual working version of JSRS and want to preempt our built-in 
broken (in recent browsers) version?  Who knows?  We need to cover all 
cases consitently and there is no reason we shouldn't be allowed to enforce 
the existing documented contract.

>>It only cares about the immediate problem of the JSRS stuff, which I 
>>think is pretty pointless anyway since it no longer works in 
>>Mozilla/Firefox (You end up with a back button history of the JSRS 
>>redirect screens which is very undesirable behavior).  It did at one 
>>point a long while back.   I even know the date when it stopped 
>>working.  It was somewhere between 5/23/2003 and 5/25/2003.  I know this 
>>because I saved copies of MozillaFirebird nightly builds from those 
>>dates.  The 5/23 build works and the 5/25 build doesn't (there was no 
>>5/24/2003 build).  I would suggest getting rid of it altogether.  Back 
>>button disabling simply doesn't work properly across browsers (at least 
>>not the JSRS technique).  Anyway, that was a bit of a tangent.  The point 
>>is, the current fix is too specific to one issue and doesn't work 
>>generically as it should.  The simple fix above adds a new contract to 
>>functions, but I don't imagine it will adversely affect most users and it 
>>will take care of this issue generically.  Plus it is much less 
>>involved.  No change should be required to the JSRS stuff (if we still 
>>really want to keep that, but I'm not sure why?).  I suggest reverting 
>>all javascript code changes and adding the simple change I've mentioned 
>>above as it should take care of everything in one fell swoop.
>I generally agree with this (the disable back button stuff hasn't worked 
>for a long time), but I'm reluctant to change it just for the sake of 
>changing it - I'm very reluctant to do something drastic in this code 
>right now because I don't want to break it. So all of my changes have been 
>aimed at trying not to break existing code (I'm more concerned about that 
>than whether or not FormControl and ClientServerHTTPLib rae linked).

Removing broken support for back button disabling can only help us.  In any 
case, my fix allows the JSRS stuff to continue to work as is.  There is no 
harm done to JSRS, so whether or not JSRS gets removed from Barracuda 
(which it really should!) now or later makes little difference to the 
FormControl discusion.


>>BTW, why change the format of the functions?  That is, you changed 
>>"__function()" to "_function()".  It is a common practice to keep 
>>function names out of the general namespace of user-defined functions.  I 
>>guess either works, but why bother then it is already there?  I suppose, 
>>ultimately, we would want to come up with some prefix that would better 
>>define the namespace we are using to avoid function name collisions, such 
>>as "bcda_function()" or some such naming convention.
>We don't have anyplace in the java code which uses 3 underscores to 
>preface functions. We have lots of places that use 1 underscore to preface 
>private functions. I just changed it to match that (because it appears 
>more standard to me). I'm not sure I'm following your point about user 
>defined functions. It sounds like you are worried here that our javascript 
>names are going to conflict with user defined method names?

Yes, that's exactly the point.  Even the doSubmitAndLock() stuff should 
really be renamed with some semi-unique prefix (or suffix) that puts it in 
its own namespace, thus avoiding collisions with functions in user-defined 
scripts.  My comment on the changing of the existing naming was pretty much 
"why bother, what good does it do to change it"?  Like I said, I use 
FormControl.js outside of Barracuda.  Others might as well.  How do you 
know that I'm not referencing those functions from another 
script?  Renaming the functions can only break things and arbitrarily 
changing the naming to something you like better does no particular 
good.  Again, I might agree to a better standard naming convention that 
actually solves (or gets us 99% of the way there) possible namespace 
issues, but not for an arbitrary reason.  It's just one of those "why 
bother when it only serves to break things?" issue.


>Final Q: can you verify whether or not the changes which are currently in 
>cvs work in Mozilla/Firefox on Linux? Shawn and Heath here at ATMR both 
>use FF on Linux and are having problems, so I'm off to debug that now...

I use WinXP and don't have a Linux box at home.  Sorry.  Maybe someone else 
can verify?  Are you really seeing different behavior in Firefox on Windows 
than Linux?  What version of Firefox?  Wow, that would really suck!!!

>I'm not strongly opposed to the simple change you suggested up above - I 
>just want to make sure I understand it, and that we're not going to 
>require something which is non-obvious and difficult to debug for users.

Nope, like I said previously, it is documented and we were actually 
supporting more lenient behavior than what we documented.  This change 
simply brings us in line with documentation.

>Let me know what you think and we'll figure this thing out...
>
>Christian


Jake

--
Barracuda mailing list
[email protected]
http://www.objectweb.org/wws/lists/projects/barracuda