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 <[email protected]> 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, [email protected] 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
smime.p7s
Description: S/MIME Cryptographic Signature
