Manual invocation of FunctionsExecutor#notify() is broken

Martin Grigorov added a comment - 22/Feb/13 07:38 Hi Didier, Can you share your ideas in the mailing lists ? With as much details as possible. If you are not subscribed please send them to mgrigorov at apache org and I'll send them on behalf of you. Or see https://cwiki.apache.org/confluence/display/WICKET/Ideas+for+Wicket+7.0#IdeasforWicket7.0-Maketransitionspossible%2FeasywhenreplacingcomponentsusingAjax%2FPush - it sounds like what you provide.

Didier Villevalois added a comment - 21/Feb/13 21:56 Martin, Thank you very much for your very fast action, that will help us a lot! If you'd like some ideas to enhance all this for 7.0, i'd say a must-have would be to optionally be able to manually trigger individual DOM replacements directly from the produced Javascript code. Having all the components replaced in one shot is often a problem when you write applications with lots dynamic effects.

Martin Grigorov added a comment - 21/Feb/13 15:14 A blog article with demo app: http://wicketinaction.com/2013/02/replace-components-with-animation/

Martin Grigorov added a comment - 21/Feb/13 15:13 @Ernesto: I've updated the comments in the JS code. @Sven: I agree that this will look nicer but since it is an API break we can do it as early as 7.0. And still the code in wicket-ajax-jquery.js will have to check for "function()" vs. "function(functionName)" and pass 'notify' only in the second case and call it in the first case.

Sven Meier added a comment - 21/Feb/13 15:05 Couldn't we just let go of this strange "|" syntax? On the server side If #appendJavaScript()'s argument matches a function declaration, just leave it that way. In the browser If this function has a parameter, pass the notify method to it and let is trigger it.

Ernesto Reinaldo Barreiro added a comment - 21/Feb/13 14:42 Martin, Thanks for fixing this! Just one comment. Would it make sense to put some more info on JavaScript so that future developers know why this is needed? Something like // test if the javascript is in form of identifier|code // if it is, we allow for letting the javascript decide when the rest of processing will continue // by invoking identifier(); // This is used for the cases when control of AJAX processing want to be transferred to user's code. // E.g. Users want to execute an effect and after that effect finishes continue with AJAX processing. // see WICKET-5039 for more explanations. So, future developers will not have to loose time wondering why this "cryptic" feature is needed. Cheers, Ernesto

Martin Grigorov added a comment - 21/Feb/13 14:20 The code is fixed and javadoc is added to AjaxRequestTarget #prepend and #appendJavaScript

Ernesto Reinaldo Barreiro added a comment - 20/Feb/13 09:15 Martin. Ok... Maybe what is missing is improving the JavaScript doc to state this directly...

Martin Grigorov added a comment - 20/Feb/13 08:20 I think Ernesto's patch should not be applied. Wicket gives the control to the application code by giving it the 'notify' function as argument. The application code should assure that it is called. I.e. it should wrap its code in try/finally. The problem with the broken functionality in Wicket 6.x will be addressed soon.

Didier Villevalois added a comment - 18/Feb/13 15:11 We could also revert the concatenation stuff and enhance the processing of evaluations in wicket-ajax-jquery.js so that it can concatenate several evaluations that do not use notify callbacks at runtime. Also a good thing would be to not use a regexp but use an attribute to convey the notify callback name.

Didier Villevalois added a comment - 18/Feb/13 15:04 Hi, I did not see this issue and commented on the issue that broke the use of identifier|jscode as permited by wicket-ajax-jquery.js (see https://issues.apache.org/jira/browse/WICKET-4881) Let me give an explanation of how I use this feature: Lets say you have a open modal window (ala bootstrap) you want to close it, change its content and reopen it in a single ajax request. For that you need to delay the component replacement until the javascript effect that closes the modal window. That animation is asynchronous, so the only way to do that is to register for the event triggered when the modal is effectively closed and call the notify() callback so that FunctionsExecuter can continue its processing. The identifier|jscode feature can not be called anymore since https://github.com/apache/wicket/commit/cbf76d0e8c355876e302699c0975971773941ae9 as the code is concatenated and each evaluation encapsulated in a '(function() { jscode })();' statement. A good solution for me would be: 1) Introduce a protected JavaScriptFragment object in AbstractAjaxResponse protected class JavaScriptFragment { private String notifyCallbackName; private String script; // + constructors and getters } 2) Provide new APIs on IJavascriptResponse: void prependJavaScript(String script); void prependJavaScript(String notifyCallbackName, String script); void appendJavaScript(String script); void appendJavaScript(String notifyCallbackName, String script); and deprecate addJavascript(..) as this is not very descriptive of where the script goes in the response (also its current implementation does not take profit of script concatenation). 3) Make AbstractAjaxResponse collect JavaScriptFragments and decide itself what it can concatenate (it is not the responsability of AjaxXmlResponse to do so is a json-based response would have the same problems with browser-specific limited call stacks). We can concatenate scripts without callback use but can't concatenate scripts with explicit callbacks, right ? 4) Make AbstractAjaxResponse define protected abstract void writePriorityEvaluation(Response response, String script, String callbackName (may be null)) protected abstract void writeNormalEvaluation(Response response, String script, String callbackName (may be null)) and use that in writeTo(..) to pass concatenated scripts or individual script with callback name. What do you think ? We could also (not disjunctive with the solution above) introduce additional APIs in wicket-ajax-jquery.js processEvaluation() to handle something like this: <[priority-]evaluate> function(scripts) { scripts.queueAsync(function() { jscode }); scripts.queueAsync(function() { jscode }); scripts.queueSync(function(notify) { jscode that calls notify }); scripts.queueAsync(function() { jscode }); scripts.queueAsync(function() { jscode }); // .... } </[priority-]evaluate> Such a queue would then be processed serially avoided the IE problem and still providing the possibility to delay component replacement. Any thoughts ?

Ernesto Reinaldo Barreiro added a comment - 18/Feb/13 11:13 Sven. It is a pity that no comment was put on code about why this special case was needed... or a reference to a JIRA issue. Maybe someone can remember why was this introduced? I guess "original problem" is also present on 1.5.x... I will try to test that later on today.

Martin Grigorov added a comment - 18/Feb/13 11:12 I also don't know the roots of something|code feature. I've just migrated it from 1.5.x and even added JS tests for it. And yes, it seems I broke it with the introduction of the anonymous function which is used to minimize the number of eval's to 1 and to keep the variables local for each prepend/append script.

Sven Meier added a comment - 18/Feb/13 11:05 Thanks Ernesto for analysing this. I wonder what this feature was used for, but we should track this in a new issue.

Ernesto Reinaldo Barreiro added a comment - 18/Feb/13 10:43 wickstart

Ernesto Reinaldo Barreiro added a comment - 18/Feb/13 10:39 Sven, I think code var res = text.match(new RegExp("^([a-z|A-Z_][a-z|A-Z|0-9_]*)\\|((.|\\n)*)$")); if (res !== null) { var f = jQuery.noop; text = "f = function(" + res[1] + ") {" + res[2] + "};"; try { // do the evaluation eval(text); f(notify); } catch (exception) { Wicket.Log.error("Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: " + exception + ", text: " + text); } } will never get called on Wicket 6.x. I have tried the following <p>     <a wicket:id="correctexpression">Correct Expression</a> </p> and AjaxLink<Void> correctexpression = new AjaxLink<Void>("correctexpression") { private static final long serialVersionUID = 1L; @Override public void onClick(AjaxRequestTarget target) { target.appendJavaScript("A|alert('Correct');A();"); target.prependJavaScript("A|alert('Correct');A();"); target.addListener(new AjaxRequestTarget.IListener() { @Override public void onBeforeRespond(Map<String, Component> map, AjaxRequestTarget target) { } @Override public void onAfterRespond(Map<String, Component> map, IJavaScriptResponse response) { response.addJavaScript("A|alert('Correct');A();"); } }); } }; add(correctexpression); but then generated JavaScript will be INFO: <?xml version="1.0" encoding="UTF-8"?><ajax-response><evaluate><![CDATA[(function(){A|alert('Correct');A();})();]]></evaluate><priority-evaluate><![CDATA[(function(){A|alert('Correct');A();})();]]></priority-evaluate><evaluate><![CDATA[(function(){A|alert('Correct');A();})();]]></evaluate></ajax-response> ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();})(); ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();})(); ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: ReferenceError: A is not defined, text: (function(){A|alert('Correct');A();})(); INFO: Response processed successfully. INFO: refocus last focused component not needed/allowed INFO: focus removed from correctexpression2 INFO: focus set on wicketDebugLink INFO: focus removed from wicketDebugLink Then because of the "anonymous" function (function(){ ...})(); around expression it will never match regular expression. So, feature A|...A(); is not really supported on 6.x

Ernesto Reinaldo Barreiro added a comment - 16/Feb/13 16:42 I will try to produce a quickstart tomorrow morning... I have also asked my client if he can provide an example for this "use case". Or maybe some of the other core developer can shed some light about what this "special" case was introduced... I find it dangerous because of that you mention above and also because nothing prevent users from calling notify(); several times e.g. expression A|{A();A();A();}...

Sven Meier added a comment - 16/Feb/13 15:57 A quickstart would be nice, yes. I'm just wondering how to handle the case when the evaluation already called notiy(), then fails with an exception and processEvaluation() would call notify once again?

Ernesto Reinaldo Barreiro added a comment - 16/Feb/13 06:04 @Sven, The problem appears on the site of a customer of mine: users complained of screens getting frozen. They have pinpointed that screen got frozen when exception happened on code above. I have no idea of what expressions of that kind they are using: I will ask them if they can provide an example of failing expressions. Do you need me to provide a quick-start? It is very easy to produce one.

Sven Meier added a comment - 15/Feb/13 22:45 Out of curiosity, what are you using that "identifier|code" evaluation for?

Ernesto Reinaldo Barreiro added a comment - 13/Feb/13 12:32 patch fixing the issue for 6.x branch