You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ja...@apache.org on 2012/05/17 09:25:13 UTC
svn commit: r1339504 - in /ofbiz/trunk/framework:
base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Author: jacopoc
Date: Thu May 17 07:25:13 2012
New Revision: 1339504
URL: http://svn.apache.org/viewvc?rev=1339504&view=rev
Log:
Based on suggestion from Daniel Dekany (committer/maintainer of the Freemarker's project):
"Since BeansWrapper.getDefaultInstance() returns an instance that is
shared in the scope of the class-loader that defined the FreeMarker
classes, and since possibly multiple independently developed
components in your system use FreeMarker and thus getDefaultInstance
(and you may don't even know about it), and since the returned
BeanWrapper can be configured (not read-only), you never know how the
returned BeanWrapper is configured. So you add a 3rd party component,
and if you are unlucky, suddenly your other component is broken.
As of creating new BeansWrapper, be sure you create it only once
during the application life-cycle and then reuse the same instance, or
else it will have to rebuild the class introspection cache again and
again."
I have replaced the usage of the static instance of the BeansWrapper with a newly created object.
Modified:
ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java?rev=1339504&r1=1339503&r2=1339504&view=diff
==============================================================================
--- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java (original)
+++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java Thu May 17 07:25:13 2012
@@ -76,8 +76,7 @@ public class FreeMarkerWorker {
// use soft references for this so that things from Content records don't kill all of our memory, or maybe not for performance reasons... hmmm, leave to config file...
public static UtilCache<String, Template> cachedTemplates = UtilCache.createUtilCache("template.ftl.general", 0, 0, false);
- protected static BeansWrapper defaultOfbizWrapper = BeansWrapper.getDefaultInstance();
- protected static Configuration defaultOfbizConfig = makeConfiguration(defaultOfbizWrapper);
+ protected static Configuration defaultOfbizConfig = makeConfiguration(new BeansWrapper());
public static Configuration makeConfiguration(BeansWrapper wrapper) {
Configuration newConfig = new Configuration();
Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java?rev=1339504&r1=1339503&r2=1339504&view=diff
==============================================================================
--- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java (original)
+++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java Thu May 17 07:25:13 2012
@@ -58,8 +58,7 @@ public class HtmlWidget extends ModelScr
public static final String module = HtmlWidget.class.getName();
public static UtilCache<String, Template> specialTemplateCache = UtilCache.createUtilCache("widget.screen.template.ftl.general", 0, 0, false);
- protected static BeansWrapper specialBeansWrapper = new ExtendedWrapper();
- protected static Configuration specialConfig = FreeMarkerWorker.makeConfiguration(specialBeansWrapper);
+ protected static Configuration specialConfig = FreeMarkerWorker.makeConfiguration((BeansWrapper)new ExtendedWrapper());
// not sure if this is the best way to get FTL to use my fancy MapModel derivative, but should work at least...
public static class ExtendedWrapper extends BeansWrapper {
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 20, 2012, at 9:51 AM, Jacopo Cappellato wrote:
>
> On May 20, 2012, at 2:07 AM, Adam Heath wrote:
>
>>>
>>> I have replaced the usage of the static instance of the BeansWrapper with a newly created object.
>>
>> You didn't go far enough. There are enough other getDefaultInstance() calls in our code that need to be fixed.
>>
>> I'll correct those, because I also need to have a central location to properly configure the BeansWrapper, based on the ftl breakage.
>
> Thank you Adam, you are completely right abut this and my commit rev. 1340631 should have fixed them all; however I would appreciate as I have made private and final a couple of fields in FreeMarkerWorker.
> I hope this will make your work easier.
To summarize, now we should have two instances of beans wrapper objects in OFBiz:
* FreeMarkerWorker.defaultOfbizWrapper
* HtmlWidget.specialBeansWrapper
Jacopo
>
> Kind regards,
>
> Jacopo
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 21, 2012, at 8:31 AM, Adam Heath wrote:
> Why does everything have to be coated with sugar with you?
I don't care if you don't understand or if you consider this optional or superfluous; be polite when you communicate with me, change your tone and don't act as if you are giving lessons; maybe one day you will understand why but for now just do it.
> You made a mistake(committing something that I said I was working on).
I apologize if my changes caused you troubles (I was actually hoping that the opposite would happen); in fact, if you would have asked me something like: "hey Jacopo, I was actually going to commit a different fix, would you mind reverting this commit?" I would have been more than happy to accommodate this.
Jacopo
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/21/2012 12:55 AM, Jacopo Cappellato wrote:
> Hi Adam,
>
> please see inline:
>
> On May 20, 2012, at 6:02 PM, Adam Heath wrote:
>
>> You just made my job harder. I said I would do it, and I already had it done.
>
> You actually wrote:
>
> "I'll correct those..."
>
> and you didn' say that you already had local changes and since I was already working on this refactoring (that is a result of a research I started some weeks ago in the Freemarker list), I had some time allocated in sunday and so I decided to implement.
At time I sent that, I didn't have any changes. I did later that day.
I started a test run locally, against 5 changes, then 'went out'. Just
as I was about to leave my place of fun, I got an email notification on
my phone, and saw that you ignored my email.
Unlike some people(I'm not saying you), I do extensive testing of a
whole series of changes before committing; that takes time. You ignored
my email, and went full-steam ahead. That caused me additional work,
and even delayed me by a day, as I had to rework the set of changes I
had to deal with what you did.
I *tried* to communicate ahead of time. But that was ignored. You just
committed straight away, without giving me any chance to respond.
"I'll correct those" means I will do it soon, ie, in the future, not
that I already have it done. That would be "I have those corrected".
>> Now I have to fix my code to interact with your changes. And, due to the following, almost undo your code.
>>
>> You didn't read your diff, you left one instance of accessing the static variable, instead of calling the static accessor method; granted, it's inside a comment, but still, you should have double-checked your diff. I do.
>>
>> This next one is my opinion: Putting 'ofbiz' into the method name is superfluous. We already know it is an ofbiz class. No need to duplicate that into the name of the method.
>>
>> The method returns a static BeansWrapper. The name doesn't reflect that fact. My local code has it named 'singletonBeansWrapper()'.
>
> This is awful name, imo, but it is just a personal opinion as it is yours on defaultOfbizWrapper (that I actually don't like much too, but it was the one before my changes). As a side trivial note my preference would be: defaultBeansWrapper.
I had it called that, because my series of changes also allowed for
other classes to maintain their own instance of a BeansWrapper; in fact,
ExtendedWrapper is one such case. I also called it singleton because it
more accurately reflects what it is for, then default.
I also didn't like the name singletonBeansWrapper; I probably would have
changed that before my final commit. I never got the chance, as you
invalidated it. I've decided to go with yours, even tho I disagree with
the name.
>> In summary, I'm rather annoyed.
>
> I don't care about this; and I am actually very annoyed by the fact that the applications are broken since a few days because of your commit, that you clearly didn't even test and I have to report the issue to you. But before now I didn't mention this because I understand that people do errors and the direction of your work was good (even if you did errors that are causing problems) and so I didn't stress this fact; and instead I am helping you to have this issue fixed.
I gave plenty of warning ahead of time, and that it was not possible for
me to test everything thoroughly. I apologize for missing the example
that has been given(looking at any order).
>> Normally, I wouldn't be. But in this case, I said I would do this change, and you went and did it anyways. I even gave a good reason for wanting to do it, as I needed it anyways to implement the findByAnd fix.
>
> Now, Adam, please read carefully what follows: I am asking you to immediately change attitude whehe tin you interact with me (you can continue with others if you like, I don't care); the tone you are using is annoying and unacceptable to me; be polite, do not pretend you can give me lessons (you can't as the history of our interactions actually shows), but rather focus on specific details, starting the email with "Hi Jacopo" could also help (and when applicable end it with "Thank you").
> Just to give you a clear example of something that you will have to change in your style when you interact with me, instead of:
>
>> You didn't read your diff, you left one instance of accessing the static variable, instead of calling the static accessor method; granted, it's inside a comment, but still, you should have double-checked your diff. I do.
>
> you could have written:
>
> "There is a commented out code that has a direct reference to the variable, could you please fix it?"
Why does everything have to be coated with sugar with you? You made a
mistake(committing something that I said I was working on). That error
is a completely separate issue with regard to anything else I might have
done(breaking ftl files). Except, that I didn't break ftl files,
freemarker has always been broken. I've actually fixed that now, so
that method calls in freemarker actually are more sane.
And, with regard to the sugar coating that you seem to so desperately
desire, you never apologized to me for stomping on my work.
The "Hi Adam" and signature are superfluous, imho. We know who the
email is from, that's part of the email header. The target of the mail
is either anonymous(most of the time), or the previous author of the
email being responded to. I don't repeat information that is quite
apparent, imho.
I don't generally ask others to do stuff. This is a volunteer project,
if I can do it myself, then I'll do it. You can't always expect a
timely response to requests. That's why I didn't ask you to continue
fixing the other BeansWrapper references, and that is why I didn't ask
you to fix that commented out version. I did them myself, because they
were trivial. But I said I would fix them publically, before I did it.
What kind of additionaly paperwork do I need to sign to make you
understand that?
If you would have responded to my mail from yesterday, saying you
already have a change done, that would have been fine. Instead, I just
see a commit from you, then I see your response. No indication that you
were also going to work on it, nor that you already had it local, or
anything of that nature.
> Kind regards,
>
> Jacopo
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
Hi Adam,
please see inline:
On May 20, 2012, at 6:02 PM, Adam Heath wrote:
> You just made my job harder. I said I would do it, and I already had it done.
You actually wrote:
"I'll correct those..."
and you didn' say that you already had local changes and since I was already working on this refactoring (that is a result of a research I started some weeks ago in the Freemarker list), I had some time allocated in sunday and so I decided to implement.
> Now I have to fix my code to interact with your changes. And, due to the following, almost undo your code.
>
> You didn't read your diff, you left one instance of accessing the static variable, instead of calling the static accessor method; granted, it's inside a comment, but still, you should have double-checked your diff. I do.
>
> This next one is my opinion: Putting 'ofbiz' into the method name is superfluous. We already know it is an ofbiz class. No need to duplicate that into the name of the method.
>
> The method returns a static BeansWrapper. The name doesn't reflect that fact. My local code has it named 'singletonBeansWrapper()'.
This is awful name, imo, but it is just a personal opinion as it is yours on defaultOfbizWrapper (that I actually don't like much too, but it was the one before my changes). As a side trivial note my preference would be: defaultBeansWrapper.
> .
> In summary, I'm rather annoyed.
I don't care about this; and I am actually very annoyed by the fact that the applications are broken since a few days because of your commit, that you clearly didn't even test and I have to report the issue to you. But before now I didn't mention this because I understand that people do errors and the direction of your work was good (even if you did errors that are causing problems) and so I didn't stress this fact; and instead I am helping you to have this issue fixed.
> Normally, I wouldn't be. But in this case, I said I would do this change, and you went and did it anyways. I even gave a good reason for wanting to do it, as I needed it anyways to implement the findByAnd fix.
Now, Adam, please read carefully what follows: I am asking you to immediately change attitude when you interact with me (you can continue with others if you like, I don't care); the tone you are using is annoying and unacceptable to me; be polite, do not pretend you can give me lessons (you can't as the history of our interactions actually shows), but rather focus on specific details, starting the email with "Hi Jacopo" could also help (and when applicable end it with "Thank you").
Just to give you a clear example of something that you will have to change in your style when you interact with me, instead of:
> You didn't read your diff, you left one instance of accessing the static variable, instead of calling the static accessor method; granted, it's inside a comment, but still, you should have double-checked your diff. I do.
you could have written:
"There is a commented out code that has a direct reference to the variable, could you please fix it?"
Kind regards,
Jacopo
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/20/2012 02:51 AM, Jacopo Cappellato wrote:
>
> On May 20, 2012, at 2:07 AM, Adam Heath wrote:
>
>>>
>>> I have replaced the usage of the static instance of the BeansWrapper with a newly created object.
>>
>> You didn't go far enough. There are enough other getDefaultInstance() calls in our code that need to be fixed.
>>
>> I'll correct those, because I also need to have a central location to properly configure the BeansWrapper, based on the ftl breakage.
>
> Thank you Adam, you are completely right abut this and my commit rev. 1340631 should have fixed them all; however I would appreciate as I have made private and final a couple of fields in FreeMarkerWorker.
> I hope this will make your work easier.
You just made my job harder. I said I would do it, and I already had it
done. Now I have to fix my code to interact with your changes. And,
due to the following, almost undo your code.
You didn't read your diff, you left one instance of accessing the static
variable, instead of calling the static accessor method; granted, it's
inside a comment, but still, you should have double-checked your diff.
I do.
This next one is my opinion: Putting 'ofbiz' into the method name is
superfluous. We already know it is an ofbiz class. No need to duplicate
that into the name of the method.
The method returns a static BeansWrapper. The name doesn't reflect that
fact. My local code has it named 'singletonBeansWrapper()'.
.
In summary, I'm rather annoyed. Normally, I wouldn't be. But in this
case, I said I would do this change, and you went and did it anyways. I
even gave a good reason for wanting to do it, as I needed it anyways to
implement the findByAnd fix.
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 20, 2012, at 2:07 AM, Adam Heath wrote:
>>
>> I have replaced the usage of the static instance of the BeansWrapper with a newly created object.
>
> You didn't go far enough. There are enough other getDefaultInstance() calls in our code that need to be fixed.
>
> I'll correct those, because I also need to have a central location to properly configure the BeansWrapper, based on the ftl breakage.
Thank you Adam, you are completely right abut this and my commit rev. 1340631 should have fixed them all; however I would appreciate as I have made private and final a couple of fields in FreeMarkerWorker.
I hope this will make your work easier.
Kind regards,
Jacopo
Re: svn commit: r1339504 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
widget/src/org/ofbiz/widget/screen/HtmlWidget.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/17/2012 02:25 AM, jacopoc@apache.org wrote:
> Author: jacopoc
> Date: Thu May 17 07:25:13 2012
> New Revision: 1339504
>
> URL: http://svn.apache.org/viewvc?rev=1339504&view=rev
> Log:
> Based on suggestion from Daniel Dekany (committer/maintainer of the Freemarker's project):
>
> "Since BeansWrapper.getDefaultInstance() returns an instance that is
> shared in the scope of the class-loader that defined the FreeMarker
> classes, and since possibly multiple independently developed
> components in your system use FreeMarker and thus getDefaultInstance
> (and you may don't even know about it), and since the returned
> BeanWrapper can be configured (not read-only), you never know how the
> returned BeanWrapper is configured. So you add a 3rd party component,
> and if you are unlucky, suddenly your other component is broken.
> As of creating new BeansWrapper, be sure you create it only once
> during the application life-cycle and then reuse the same instance, or
> else it will have to rebuild the class introspection cache again and
> again."
>
> I have replaced the usage of the static instance of the BeansWrapper with a newly created object.
You didn't go far enough. There are enough other getDefaultInstance()
calls in our code that need to be fixed.
I'll correct those, because I also need to have a central location to
properly configure the BeansWrapper, based on the ftl breakage.
>
> Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java?rev=1339504&r1=1339503&r2=1339504&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java Thu May 17 07:25:13 2012
> @@ -76,8 +76,7 @@ public class FreeMarkerWorker {
>
> // use soft references for this so that things from Content records don't kill all of our memory, or maybe not for performance reasons... hmmm, leave to config file...
> public static UtilCache<String, Template> cachedTemplates = UtilCache.createUtilCache("template.ftl.general", 0, 0, false);
> - protected static BeansWrapper defaultOfbizWrapper = BeansWrapper.getDefaultInstance();
> - protected static Configuration defaultOfbizConfig = makeConfiguration(defaultOfbizWrapper);
> + protected static Configuration defaultOfbizConfig = makeConfiguration(new BeansWrapper());
>
> public static Configuration makeConfiguration(BeansWrapper wrapper) {
> Configuration newConfig = new Configuration();
>
> Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java?rev=1339504&r1=1339503&r2=1339504&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java (original)
> +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/HtmlWidget.java Thu May 17 07:25:13 2012
> @@ -58,8 +58,7 @@ public class HtmlWidget extends ModelScr
> public static final String module = HtmlWidget.class.getName();
>
> public static UtilCache<String, Template> specialTemplateCache = UtilCache.createUtilCache("widget.screen.template.ftl.general", 0, 0, false);
> - protected static BeansWrapper specialBeansWrapper = new ExtendedWrapper();
> - protected static Configuration specialConfig = FreeMarkerWorker.makeConfiguration(specialBeansWrapper);
> + protected static Configuration specialConfig = FreeMarkerWorker.makeConfiguration((BeansWrapper)new ExtendedWrapper());
>
> // not sure if this is the best way to get FTL to use my fancy MapModel derivative, but should work at least...
> public static class ExtendedWrapper extends BeansWrapper {
>
>