Re: svn commit: r1835869 - in /ofbiz/ofbiz-framework/trunk: framework/widget/dtd/ framework/widget/src/main/java/org/apache/ofbiz/widget/model/ framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/ framework/widget/src/main/java/org/apache/ofbiz...

[Nicolas Malin-2]

Hello Suraj,

On 14/07/2018 07:10, [hidden email] wrote:

> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java
> URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java?rev=1835869&r1=1835868&r2=1835869&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java Sat Jul 14 05:10:00 2018
> @@ -147,7 +147,7 @@ public abstract class ModelForm extends
>       private final String formWidgetAreaStyle;
>       private final boolean groupColumns;
>       private final String headerRowStyle;
> -    private final boolean hideHeader;
> +    private boolean hideHeader;

With this commit you change the model during ofbiz execution and this
break the thread safe. I will try to found an other to correct this

Nicolas

[Gil Portenseigne]

Hello Suraj,

Regarding Nicolas feedback and mine onto Jira https://issues.apache.org/jira/browse/OFBIZ-7598, did you find time to look into it ?

Thanks,

Gil

Le mardi 17 juil. 2018 à 14:04:06 (+0200), Nicolas Malin a écrit :

> Hello Suraj,
>
> On 14/07/2018 07:10, [hidden email] wrote:
> > Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java
> > URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java?rev=1835869&r1=1835868&r2=1835869&view=diff
> > ==============================================================================
> > --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java (original)
> > +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java Sat Jul 14 05:10:00 2018
> > @@ -147,7 +147,7 @@ public abstract class ModelForm extends
> >       private final String formWidgetAreaStyle;
> >       private final boolean groupColumns;
> >       private final String headerRowStyle;
> > -    private final boolean hideHeader;
> > +    private boolean hideHeader;
> With this commit you change the model during ofbiz execution and this break
> the thread safe. I will try to found an other to correct this
>
> Nicolas

[Michael Brohl-3]

There's also a remark by Deepak.

I'm in favor of reverting and providing a new solution once it covers
all remarks. We should not leave it in the codebase in this state.

Best regards,

Michael Brohl
ecomify GmbH
www.ecomify.de

Am 30.07.18 um 10:14 schrieb Gil Portenseigne:

> Hello Suraj,
>
> Regarding Nicolas feedback and mine onto Jira https://issues.apache.org/jira/browse/OFBIZ-7598, did you find time to look into it ?
>
> Thanks,
>
> Gil
>
> Le mardi 17 juil. 2018 à 14:04:06 (+0200), Nicolas Malin a écrit :
>> Hello Suraj,
>>
>> On 14/07/2018 07:10, [hidden email] wrote:
>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java
>>> URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java?rev=1835869&r1=1835868&r2=1835869&view=diff
>>> ==============================================================================
>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java (original)
>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelForm.java Sat Jul 14 05:10:00 2018
>>> @@ -147,7 +147,7 @@ public abstract class ModelForm extends
>>>        private final String formWidgetAreaStyle;
>>>        private final boolean groupColumns;
>>>        private final String headerRowStyle;
>>> -    private final boolean hideHeader;
>>> +    private boolean hideHeader;
>> With this commit you change the model during ofbiz execution and this break
>> the thread safe. I will try to found an other to correct this
>>
>> Nicolas

[Suraj Khurana]

Hello,

Thanks Deepak for pointing this out.
I noted down your feedback and soon will update things accordingly.

@Nicolas, Gil
Yes, I think you are suggesting right. It would be better to change things
as per your suggestion. I am working on it as soon provide patch for review.

@Michael,
Please suggest, what should be the right way to proceed, IMO, if it is not
breaking something functional (yes, it is not), we can do required
improvements in another commit.

Current patch has been there for quite enough time for review, and many of
us have discussed on it on various aspects. These are small enhancements in
process of code re-view which is actual beauty of working in community and
I guess these improvements should be honored in another commit.

I agree on both things whatever we choose to follow either to commit
suggested enhancements or to revert, do changes and than again commit.

--
Thanks and Regards
Suraj Khurana | Omni-channel OMS Technical Expert



On Mon, Jul 30, 2018 at 2:22 PM, Michael Brohl <[hidden email]>
wrote:

> There's also a remark by Deepak.
>
> I'm in favor of reverting and providing a new solution once it covers all
> remarks. We should not leave it in the codebase in this state.
>
> Best regards,
>
> Michael Brohl
> ecomify GmbH
> www.ecomify.de
>
> Am 30.07.18 um 10:14 schrieb Gil Portenseigne:
>
> Hello Suraj,
>>
>> Regarding Nicolas feedback and mine onto Jira
>> https://issues.apache.org/jira/browse/OFBIZ-7598, did you find time to
>> look into it ?
>>
>> Thanks,
>>
>> Gil
>>
>> Le mardi 17 juil. 2018 à 14:04:06 (+0200), Nicolas Malin a écrit :
>>
>>> Hello Suraj,
>>>
>>> On 14/07/2018 07:10, [hidden email] wrote:
>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>> rg/apache/ofbiz/widget/model/ModelForm.java
>>>> URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk
>>>> /framework/widget/src/main/java/org/apache/ofbiz/widget/mode
>>>> l/ModelForm.java?rev=1835869&r1=1835868&r2=1835869&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>> rg/apache/ofbiz/widget/model/ModelForm.java (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>> rg/apache/ofbiz/widget/model/ModelForm.java Sat Jul 14 05:10:00 2018
>>>> @@ -147,7 +147,7 @@ public abstract class ModelForm extends
>>>>        private final String formWidgetAreaStyle;
>>>>        private final boolean groupColumns;
>>>>        private final String headerRowStyle;
>>>> -    private final boolean hideHeader;
>>>> +    private boolean hideHeader;
>>>>
>>> With this commit you change the model during ofbiz execution and this
>>> break
>>> the thread safe. I will try to found an other to correct this
>>>
>>> Nicolas
>>>
>>
>
>

[Michael Brohl-3]

Hi Suraj,

my interpretion of the feedbacks was that the commit has problems or is
incomplete.

If it does break anything or introduces functionality which is not
working completely, we should revert.

If this is not the case and you have the chance to implement the
discussed issues in due time, I think it is not a problem and we do not
need to revert.


Why am I so sensible here?

We are struggling with half baked, incomplete or buggy code in several
areas which often shows up a long time after it was committed. In other
cases, even if problems are raised soon after the commit, the code is
left untouched for a long time until it is work on again or simply is
forgotten.

(I do not assume these both szenarios fit in your case).

To avoid this, I am more in favor of reverting immediately instead of
leaving the code in the codebase.


Thanks for your valuable work and best regards,

Michael


Am 30.07.18 um 12:45 schrieb Suraj Khurana:

> Hello,
>
> Thanks Deepak for pointing this out.
> I noted down your feedback and soon will update things accordingly.
>
> @Nicolas, Gil
> Yes, I think you are suggesting right. It would be better to change things
> as per your suggestion. I am working on it as soon provide patch for review.
>
> @Michael,
> Please suggest, what should be the right way to proceed, IMO, if it is not
> breaking something functional (yes, it is not), we can do required
> improvements in another commit.
>
> Current patch has been there for quite enough time for review, and many of
> us have discussed on it on various aspects. These are small enhancements in
> process of code re-view which is actual beauty of working in community and
> I guess these improvements should be honored in another commit.
>
> I agree on both things whatever we choose to follow either to commit
> suggested enhancements or to revert, do changes and than again commit.
>
> --
> Thanks and Regards
> Suraj Khurana | Omni-channel OMS Technical Expert
>
>
>
> On Mon, Jul 30, 2018 at 2:22 PM, Michael Brohl <[hidden email]>
> wrote:
>
>> There's also a remark by Deepak.
>>
>> I'm in favor of reverting and providing a new solution once it covers all
>> remarks. We should not leave it in the codebase in this state.
>>
>> Best regards,
>>
>> Michael Brohl
>> ecomify GmbH
>> www.ecomify.de
>>
>> Am 30.07.18 um 10:14 schrieb Gil Portenseigne:
>>
>> Hello Suraj,
>>> Regarding Nicolas feedback and mine onto Jira
>>> https://issues.apache.org/jira/browse/OFBIZ-7598, did you find time to
>>> look into it ?
>>>
>>> Thanks,
>>>
>>> Gil
>>>
>>> Le mardi 17 juil. 2018 à 14:04:06 (+0200), Nicolas Malin a écrit :
>>>
>>>> Hello Suraj,
>>>>
>>>> On 14/07/2018 07:10, [hidden email] wrote:
>>>>
>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>>> rg/apache/ofbiz/widget/model/ModelForm.java
>>>>> URL:http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk
>>>>> /framework/widget/src/main/java/org/apache/ofbiz/widget/mode
>>>>> l/ModelForm.java?rev=1835869&r1=1835868&r2=1835869&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>>> rg/apache/ofbiz/widget/model/ModelForm.java (original)
>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/o
>>>>> rg/apache/ofbiz/widget/model/ModelForm.java Sat Jul 14 05:10:00 2018
>>>>> @@ -147,7 +147,7 @@ public abstract class ModelForm extends
>>>>>         private final String formWidgetAreaStyle;
>>>>>         private final boolean groupColumns;
>>>>>         private final String headerRowStyle;
>>>>> -    private final boolean hideHeader;
>>>>> +    private boolean hideHeader;
>>>>>
>>>> With this commit you change the model during ofbiz execution and this
>>>> break
>>>> the thread safe. I will try to found an other to correct this
>>>>
>>>> Nicolas
>>>>
>>