Form Widget labels disappearing
Locked
12
12
 
|
After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work.
Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) Here's another: https://localhost:8443/accounting/control/findPayments Thanks Scott HotWax Media http://www.hotwaxmedia.com |
smime.p7s (3K)
Re: Form Widget labels disappearing
|
Hi this seems to be caused by r904592
to revert this change: svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 Regards, Hans On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: > After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. > > Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) > Here's another: https://localhost:8443/accounting/control/findPayments > > Thanks > Scott > > HotWax Media > http://www.hotwaxmedia.com > > Antwebsystems.com: Quality OFBiz services for competitive rates |
Re: Form Widget labels disappearing
|
|
In reply to this post by Scott Gray-2
Could be related: the form widget example page is broken. It was working 2 days ago, no time to look further for now
Jacques From: "Scott Gray" <[hidden email]> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) Here's another: https://localhost:8443/accounting/control/findPayments Thanks Scott HotWax Media http://www.hotwaxmedia.com |
Re: Form Widget labels disappearing
|
|
In reply to this post by hans_bakker
Thanks Hans, but I'm pretty sure you know that I know how to revert :-)
Regards Scott On 2/02/2010, at 12:35 AM, Hans Bakker wrote: > Hi this seems to be caused by r904592 > > to revert this change: > svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 > > Regards, > Hans > > > On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >> >> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >> Here's another: https://localhost:8443/accounting/control/findPayments >> >> Thanks >> Scott >> >> HotWax Media >> http://www.hotwaxmedia.com >> >> > -- > Antwebsystems.com: Quality OFBiz services for competitive rates > |
Re: Form Widget labels disappearing
|
|
In reply to this post by hans_bakker
Hans,
How is that commit affecting UI labels? -Adrian Hans Bakker wrote: > Hi this seems to be caused by r904592 > > to revert this change: > svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 > > Regards, > Hans > > > On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >> >> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >> Here's another: https://localhost:8443/accounting/control/findPayments >> >> Thanks >> Scott >> >> HotWax Media >> http://www.hotwaxmedia.com >> >> |
Re: Form Widget labels disappearing
|
|
In reply to this post by hans_bakker
Hans Bakker wrote:
> Hi this seems to be caused by r904592 > > to revert this change: > svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 > > Regards, > Hans > > > On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >> >> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >> Here's another: https://localhost:8443/accounting/control/findPayments Scott, you use git now for ofbiz work. Based on Hans' message, you should try out git bisect. Pick a git revision from a month go(git co $hash), clean-all/run-install/run/check site, then start git bisect. I think you will like what it does. |
Re: Form Widget labels disappearing
|
|
In reply to this post by Adrian Crum
Okay, I found the problem and committed a fix. We really need to go
through the framework and fix the code that uses FlexibleStringExpander in inappropriate ways. FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to null. Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. -Adrian Adrian Crum wrote: > Hans, > > How is that commit affecting UI labels? > > -Adrian > > Hans Bakker wrote: >> Hi this seems to be caused by r904592 >> >> to revert this change: >> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >> >> Regards, >> Hans >> >> >> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>> After taking an update today, a lot of form widget field labels seem >>> to be missing from various screens. If any committers are aware of >>> any changes they may have made which could have impacted this please >>> take a look and see if it's related to your work. >>> >>> Here's an example: >>> https://localhost:8443/catalog/control/FindProductConfigItems (only >>> one of the 3 search option fields are presenting a field label) >>> Here's another: https://localhost:8443/accounting/control/findPayments >>> >>> Thanks >>> Scott >>> >>> HotWax Media >>> http://www.hotwaxmedia.com >>> >>> > |
Re: Form Widget labels disappearing
|
|
There's the fun of changing the framework... -David On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: > Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses FlexibleStringExpander in inappropriate ways. > > FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to null. > > Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. > > -Adrian > > Adrian Crum wrote: >> Hans, >> How is that commit affecting UI labels? >> -Adrian >> Hans Bakker wrote: >>> Hi this seems to be caused by r904592 >>> >>> to revert this change: >>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>> >>> Regards, >>> Hans >>> >>> >>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >>>> >>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>> >>>> Thanks >>>> Scott >>>> >>>> HotWax Media >>>> http://www.hotwaxmedia.com >>>> >>>> |
Re: Form Widget labels disappearing
|
|
I looked through some example uses of FlexibleStringExpander and there
is a lot of inconsistency. Some client code expects getOriginal() to always return a String - it doesn't check for null. Other code checks for null. The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains something. If it's empty it will do nothing. -Adrian David E Jones wrote: > There's the fun of changing the framework... > > -David > > > On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: > >> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses FlexibleStringExpander in inappropriate ways. >> >> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to null. >> >> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. >> >> -Adrian >> >> Adrian Crum wrote: >>> Hans, >>> How is that commit affecting UI labels? >>> -Adrian >>> Hans Bakker wrote: >>>> Hi this seems to be caused by r904592 >>>> >>>> to revert this change: >>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>>> >>>> Regards, >>>> Hans >>>> >>>> >>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >>>>> >>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >>>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>>> >>>>> Thanks >>>>> Scott >>>>> >>>>> HotWax Media >>>>> http://www.hotwaxmedia.com >>>>> >>>>> > > |
Re: Form Widget labels disappearing
|
|
And that matters because... ? The problem is that whenever you change the framework there may be unintended side-effects because the way you think it should be used may not be the way it is being used (or even the way it was originally intended to be used). Even if you test a lot there is always the risk of this. So, the point is that you shouldn't be surprised by things like this. Yes, it is disappointing that people fail to use obvious things like an isEmpty method. Whatever the case, if you change how the code operates you ALWAYS risk these sorts of issues. It's still to say the problem was caused by how the class was being used... maybe convenient, but not really correct or useful. Yes, if only everyone would always develop things the way that I think they should be developed! Well, if that were the case for me, OFBiz would be a lot different right now... :) -David On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote: > I looked through some example uses of FlexibleStringExpander and there is a lot of inconsistency. Some client code expects getOriginal() to always return a String - it doesn't check for null. Other code checks for null. > > The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains something. If it's empty it will do nothing. > > -Adrian > > David E Jones wrote: >> There's the fun of changing the framework... >> -David >> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: >>> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses FlexibleStringExpander in inappropriate ways. >>> >>> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to null. >>> >>> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. >>> >>> -Adrian >>> >>> Adrian Crum wrote: >>>> Hans, >>>> How is that commit affecting UI labels? >>>> -Adrian >>>> Hans Bakker wrote: >>>>> Hi this seems to be caused by r904592 >>>>> >>>>> to revert this change: >>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>>>> >>>>> Regards, >>>>> Hans >>>>> >>>>> >>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >>>>>> >>>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >>>>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>>>> >>>>>> Thanks >>>>>> Scott >>>>>> >>>>>> HotWax Media >>>>>> http://www.hotwaxmedia.com >>>>>> >>>>>> |
Re: Form Widget labels disappearing
|
|
I think you're reading too much into what I said.
I didn't assign blame to anyone else - I'm using this as an opportunity to instruct. I haven't forgotten the history of the code. A lot of those unnecessary checks are left over from the older version of FSE that used the new operator. So, for the benefit of the other developers I'm explaining why that code is no longer necessary. And I'm not surprised by framework changes causing unintended side effects. I usually time my framework commits so I have time available to fix any problems that pop up. -Adrian David E Jones wrote: > And that matters because... ? > > The problem is that whenever you change the framework there may be unintended side-effects because the way you think it should be used may not be the way it is being used (or even the way it was originally intended to be used). > > Even if you test a lot there is always the risk of this. So, the point is that you shouldn't be surprised by things like this. > > Yes, it is disappointing that people fail to use obvious things like an isEmpty method. Whatever the case, if you change how the code operates you ALWAYS risk these sorts of issues. It's still to say the problem was caused by how the class was being used... maybe convenient, but not really correct or useful. > > Yes, if only everyone would always develop things the way that I think they should be developed! Well, if that were the case for me, OFBiz would be a lot different right now... :) > > -David > > > On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote: > >> I looked through some example uses of FlexibleStringExpander and there is a lot of inconsistency. Some client code expects getOriginal() to always return a String - it doesn't check for null. Other code checks for null. >> >> The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains something. If it's empty it will do nothing. >> >> -Adrian >> >> David E Jones wrote: >>> There's the fun of changing the framework... >>> -David >>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: >>>> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses FlexibleStringExpander in inappropriate ways. >>>> >>>> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to null. >>>> >>>> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. >>>> >>>> -Adrian >>>> >>>> Adrian Crum wrote: >>>>> Hans, >>>>> How is that commit affecting UI labels? >>>>> -Adrian >>>>> Hans Bakker wrote: >>>>>> Hi this seems to be caused by r904592 >>>>>> >>>>>> to revert this change: >>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>>>>> >>>>>> Regards, >>>>>> Hans >>>>>> >>>>>> >>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. >>>>>>> >>>>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are presenting a field label) >>>>>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>>>>> >>>>>>> Thanks >>>>>>> Scott >>>>>>> >>>>>>> HotWax Media >>>>>>> http://www.hotwaxmedia.com >>>>>>> >>>>>>> > > |
Re: Form Widget labels disappearing
|
|
Then we should have this kind of advices in the API or the documentation somewhere. We can't expect that future devs will pick this
message easily. It would be easier for us to refer to somewhere as well. My 2 cts Jacques From: "Adrian Crum" <[hidden email]> >I think you're reading too much into what I said. > > I didn't assign blame to anyone else - I'm using this as an opportunity to instruct. I haven't forgotten the history of the code. > A lot of those unnecessary checks are left over from the older version of FSE that used the new operator. So, for the benefit of > the other developers I'm explaining why that code is no longer necessary. > > And I'm not surprised by framework changes causing unintended side effects. I usually time my framework commits so I have time > available to fix any problems that pop up. > > -Adrian > > David E Jones wrote: >> And that matters because... ? >> >> The problem is that whenever you change the framework there may be unintended side-effects because the way you think it should be >> used may not be the way it is being used (or even the way it was originally intended to be used). >> >> Even if you test a lot there is always the risk of this. So, the point is that you shouldn't be surprised by things like this. >> >> Yes, it is disappointing that people fail to use obvious things like an isEmpty method. Whatever the case, if you change how the >> code operates you ALWAYS risk these sorts of issues. It's still to say the problem was caused by how the class was being used... >> maybe convenient, but not really correct or useful. Yes, if only everyone would always develop things the way that I think they >> should be developed! Well, if that were the case for me, OFBiz would be a lot different right now... :) >> >> -David >> >> >> On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote: >> >>> I looked through some example uses of FlexibleStringExpander and there is a lot of inconsistency. Some client code expects >>> getOriginal() to always return a String - it doesn't check for null. Other code checks for null. >>> >>> The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains >>> something. If it's empty it will do nothing. >>> >>> -Adrian >>> >>> David E Jones wrote: >>>> There's the fun of changing the framework... >>>> -David >>>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: >>>>> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses >>>>> FlexibleStringExpander in inappropriate ways. >>>>> >>>>> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to >>>>> null. >>>>> >>>>> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. >>>>> >>>>> -Adrian >>>>> >>>>> Adrian Crum wrote: >>>>>> Hans, >>>>>> How is that commit affecting UI labels? >>>>>> -Adrian >>>>>> Hans Bakker wrote: >>>>>>> Hi this seems to be caused by r904592 >>>>>>> >>>>>>> to revert this change: >>>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>>>>>> >>>>>>> Regards, >>>>>>> Hans >>>>>>> >>>>>>> >>>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers >>>>>>>> are aware of any changes they may have made which could have impacted this please take a look and see if it's related to >>>>>>>> your work. >>>>>>>> >>>>>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields >>>>>>>> are presenting a field label) >>>>>>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>>>>>> >>>>>>>> Thanks >>>>>>>> Scott >>>>>>>> >>>>>>>> HotWax Media >>>>>>>> http://www.hotwaxmedia.com >>>>>>>> >>>>>>>> >> >> > |
FlexibleStringExpander (was: Form Widget labels disappearing)
|
|
In August 2008, FlexibleStringExpander class was redesigned to help
conserve memory. The main change needed to use the redesigned class was to replace FlexibleStringExpander fse = new FlexibleStringExpander("someExpression"); with FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); The getInstance method was designed to ALWAYS return an instance, even when the method was called with a null or empty argument. Doing things this way makes coding simpler - just treat the instance as if it contains something. There is no need to check for null or empty expressions. A simple project-wide search-and-replace was done to use the redesigned class. The problem is, doing that left behind a lot of unnecessary code that was needed for the old design. It would be nice if we could get some of that legacy code cleaned up. Here are a couple of suggested changes... Replace FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); if (fse != null) { // some code } with FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); // some code (getInstance never returns null) Replace FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var); if (fse.getOriginal() != null) { // some code } with FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var); if (!fse.isEmpty()) { // some code } -Adrian Jacques Le Roux wrote: > Then we should have this kind of advices in the API or the documentation > somewhere. We can't expect that future devs will pick this > message easily. It would be easier for us to refer to somewhere as well. > > My 2 cts > > Jacques > > From: "Adrian Crum" <[hidden email]> >> I think you're reading too much into what I said. >> >> I didn't assign blame to anyone else - I'm using this as an >> opportunity to instruct. I haven't forgotten the history of the code. >> A lot of those unnecessary checks are left over from the older version >> of FSE that used the new operator. So, for the benefit of >> the other developers I'm explaining why that code is no longer necessary. >> >> And I'm not surprised by framework changes causing unintended side >> effects. I usually time my framework commits so I have time >> available to fix any problems that pop up. >> >> -Adrian >> >> David E Jones wrote: >>> And that matters because... ? >>> >>> The problem is that whenever you change the framework there may be >>> unintended side-effects because the way you think it should be >>> used may not be the way it is being used (or even the way it was >>> originally intended to be used). >>> >>> Even if you test a lot there is always the risk of this. So, the >>> point is that you shouldn't be surprised by things like this. >>> >>> Yes, it is disappointing that people fail to use obvious things like >>> an isEmpty method. Whatever the case, if you change how the >>> code operates you ALWAYS risk these sorts of issues. It's still to >>> say the problem was caused by how the class was being used... >>> maybe convenient, but not really correct or useful. Yes, if only >>> everyone would always develop things the way that I think they >>> should be developed! Well, if that were the case for me, OFBiz would >>> be a lot different right now... :) >>> >>> -David >>> >>> >>> On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote: >>> >>>> I looked through some example uses of FlexibleStringExpander and >>>> there is a lot of inconsistency. Some client code expects >>>> getOriginal() to always return a String - it doesn't check for null. >>>> Other code checks for null. >>>> >>>> The whole point of the null instance was to eliminate all of the >>>> checking for this or that. Just use it as if it contains >>>> something. If it's empty it will do nothing. >>>> >>>> -Adrian >>>> >>>> David E Jones wrote: >>>>> There's the fun of changing the framework... >>>>> -David >>>>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: >>>>>> Okay, I found the problem and committed a fix. We really need to >>>>>> go through the framework and fix the code that uses >>>>>> FlexibleStringExpander in inappropriate ways. >>>>>> >>>>>> FlexibleStringExpander.getInstance will ALWAYS return an instance, >>>>>> so there is no need to check if an instance is equal to >>>>>> null. >>>>>> >>>>>> Checking FlexibleStringExpander.getOriginal() for null or empty is >>>>>> bad coding style. There is an isEmpty() method for that. >>>>>> >>>>>> -Adrian >>>>>> >>>>>> Adrian Crum wrote: >>>>>>> Hans, >>>>>>> How is that commit affecting UI labels? >>>>>>> -Adrian >>>>>>> Hans Bakker wrote: >>>>>>>> Hi this seems to be caused by r904592 >>>>>>>> >>>>>>>> to revert this change: >>>>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk >>>>>>>> -r904592:904591 >>>>>>>> >>>>>>>> Regards, >>>>>>>> Hans >>>>>>>> >>>>>>>> >>>>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>>>>>> After taking an update today, a lot of form widget field labels >>>>>>>>> seem to be missing from various screens. If any committers >>>>>>>>> are aware of any changes they may have made which could have >>>>>>>>> impacted this please take a look and see if it's related to >>>>>>>>> your work. >>>>>>>>> >>>>>>>>> Here's an example: >>>>>>>>> https://localhost:8443/catalog/control/FindProductConfigItems >>>>>>>>> (only one of the 3 search option fields >>>>>>>>> are presenting a field label) >>>>>>>>> Here's another: >>>>>>>>> https://localhost:8443/accounting/control/findPayments >>>>>>>>> >>>>>>>>> Thanks >>>>>>>>> Scott >>>>>>>>> >>>>>>>>> HotWax Media >>>>>>>>> http://www.hotwaxmedia.com >>>>>>>>> >>>>>>>>> >>> >>> >> > > |
Re: Form Widget labels disappearing
|
|
In reply to this post by Scott Gray-2
I just tried after Adrian fix (r905696): it's not related
Jacques From: "Jacques Le Roux" <[hidden email]> > Could be related: the form widget example page is broken. It was working 2 days ago, no time to look further for now > > Jacques > > From: "Scott Gray" <[hidden email]> > After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any committers are > aware of any changes they may have made which could have impacted this please take a look and see if it's related to your work. > > Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields are > presenting a field label) > Here's another: https://localhost:8443/accounting/control/findPayments > > Thanks > Scott > > HotWax Media > http://www.hotwaxmedia.com > > > |
Re: FlexibleStringExpander (was: Form Widget labels disappearing)
|
|
In reply to this post by Adrian Crum
Thanks Adrian,
When I will get a chance I will consider to document this. I guess at least in the API and I hope to begin a section for stuff like that in the wiki. Jacques From: "Adrian Crum" <[hidden email]> > In August 2008, FlexibleStringExpander class was redesigned to help conserve memory. The main change needed to use the redesigned > class was to replace > > FlexibleStringExpander fse = new FlexibleStringExpander("someExpression"); > > with > > FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); > > The getInstance method was designed to ALWAYS return an instance, even when the method was called with a null or empty argument. > Doing things this way makes coding simpler - just treat the instance as if it contains something. There is no need to check for > null or empty expressions. > > A simple project-wide search-and-replace was done to use the redesigned class. The problem is, doing that left behind a lot of > unnecessary code that was needed for the old design. > > It would be nice if we could get some of that legacy code cleaned up. Here are a couple of suggested changes... > > Replace > > FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); > if (fse != null) { > // some code > } > > with > > FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression"); > // some code (getInstance never returns null) > > Replace > > FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var); > if (fse.getOriginal() != null) { > // some code > } > > with > > FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var); > if (!fse.isEmpty()) { > // some code > } > > -Adrian > > > Jacques Le Roux wrote: >> Then we should have this kind of advices in the API or the documentation somewhere. We can't expect that future devs will pick >> this >> message easily. It would be easier for us to refer to somewhere as well. >> >> My 2 cts >> >> Jacques >> >> From: "Adrian Crum" <[hidden email]> >>> I think you're reading too much into what I said. >>> >>> I didn't assign blame to anyone else - I'm using this as an opportunity to instruct. I haven't forgotten the history of the >>> code. >>> A lot of those unnecessary checks are left over from the older version of FSE that used the new operator. So, for the benefit of >>> the other developers I'm explaining why that code is no longer necessary. >>> >>> And I'm not surprised by framework changes causing unintended side effects. I usually time my framework commits so I have time >>> available to fix any problems that pop up. >>> >>> -Adrian >>> >>> David E Jones wrote: >>>> And that matters because... ? >>>> >>>> The problem is that whenever you change the framework there may be unintended side-effects because the way you think it should >>>> be >>>> used may not be the way it is being used (or even the way it was originally intended to be used). >>>> >>>> Even if you test a lot there is always the risk of this. So, the point is that you shouldn't be surprised by things like this. >>>> >>>> Yes, it is disappointing that people fail to use obvious things like an isEmpty method. Whatever the case, if you change how >>>> the >>>> code operates you ALWAYS risk these sorts of issues. It's still to say the problem was caused by how the class was being >>>> used... >>>> maybe convenient, but not really correct or useful. Yes, if only everyone would always develop things the way that I think they >>>> should be developed! Well, if that were the case for me, OFBiz would be a lot different right now... :) >>>> >>>> -David >>>> >>>> >>>> On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote: >>>> >>>>> I looked through some example uses of FlexibleStringExpander and there is a lot of inconsistency. Some client code expects >>>>> getOriginal() to always return a String - it doesn't check for null. Other code checks for null. >>>>> >>>>> The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains >>>>> something. If it's empty it will do nothing. >>>>> >>>>> -Adrian >>>>> >>>>> David E Jones wrote: >>>>>> There's the fun of changing the framework... >>>>>> -David >>>>>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote: >>>>>>> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses >>>>>>> FlexibleStringExpander in inappropriate ways. >>>>>>> >>>>>>> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to >>>>>>> null. >>>>>>> >>>>>>> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that. >>>>>>> >>>>>>> -Adrian >>>>>>> >>>>>>> Adrian Crum wrote: >>>>>>>> Hans, >>>>>>>> How is that commit affecting UI labels? >>>>>>>> -Adrian >>>>>>>> Hans Bakker wrote: >>>>>>>>> Hi this seems to be caused by r904592 >>>>>>>>> >>>>>>>>> to revert this change: >>>>>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591 >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Hans >>>>>>>>> >>>>>>>>> >>>>>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote: >>>>>>>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any >>>>>>>>>> committers >>>>>>>>>> are aware of any changes they may have made which could have impacted this please take a look and see if it's related to >>>>>>>>>> your work. >>>>>>>>>> >>>>>>>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields >>>>>>>>>> are presenting a field label) >>>>>>>>>> Here's another: https://localhost:8443/accounting/control/findPayments >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> Scott >>>>>>>>>> >>>>>>>>>> HotWax Media >>>>>>>>>> http://www.hotwaxmedia.com >>>>>>>>>> >>>>>>>>>> >>>> >>>> >>> >> >> > |
Re: Form Widget labels disappearing
|
|
In reply to this post by Jacques Le Roux-2-2
Jacques Le Roux wrote:
> Then we should have this kind of advices in the API or the documentation > somewhere. We can't expect that future devs will pick this > message easily. It would be easier for us to refer to somewhere as well. Create an annotation that marks such things, then use apt to verify it at the source level. |
|
|
In reply to this post by Adrian Crum
Adrian Crum wrote:
> FlexibleStringExpander fse = > FlexibleStringExpander.getInstance("someExpression"); > if (fse != null) { > // some code > } This was always broken. Type foo = new Type(); foo will *never* be null, period. There are existing places in the code where this new Type() compared to null takes place, unrelated to FSE. |
|
|
Adam Heath wrote:
> Adrian Crum wrote: >> FlexibleStringExpander fse = >> FlexibleStringExpander.getInstance("someExpression"); >> if (fse != null) { >> // some code >> } > > This was always broken. Type foo = new Type(); foo will *never* be > null, period. There are existing places in the code where this new > Type() compared to null takes place, unrelated to FSE. I was trying to provide a simplified example. You are correct - the simplified example would never work to begin with. What the simplified example might really look like: protected FlexibleStringExpander description = null; ... public MyClass(String expression) { if (expression != null) { description = FlexibleStringExpander.getInstance(expression); } } ... public String getDescription(Map context) { if (description != null) { return description.expandString(context); } ... } -Adrian |
Re: Form Widget labels disappearing
|
|
In reply to this post by Adam Heath-2
Adam Heath wrote:
> Jacques Le Roux wrote: >> Then we should have this kind of advices in the API or the documentation >> somewhere. We can't expect that future devs will pick this >> message easily. It would be easier for us to refer to somewhere as well. > > Create an annotation that marks such things, then use apt to verify it > at the source level. I plan on going through the project code tonight and change the code that tests getOriginal() for null. After that I will get rid of the NullExpr class I just created to fix the problem. Then I will clearly document the FSE methods and what they return. Maybe include some example uses in the class description. You used the term contract in a previous thread. I like that term. The contract with FSE will be clearly defined when I am finished. -Adrian |
|
|
In reply to this post by Adrian Crum
Adrian Crum wrote:
> Adam Heath wrote: >> Adrian Crum wrote: >>> FlexibleStringExpander fse = >>> FlexibleStringExpander.getInstance("someExpression"); >>> if (fse != null) { >>> // some code >>> } >> >> This was always broken. Type foo = new Type(); foo will *never* be >> null, period. There are existing places in the code where this new >> Type() compared to null takes place, unrelated to FSE. > > I was trying to provide a simplified example. You are correct - the > simplified example would never work to begin with. What the simplified > example might really look like: > > protected FlexibleStringExpander description = null; Just for reference, the correct fix here would remove the null default value, make it final, and remove the condition check below. > ... > public MyClass(String expression) { > if (expression != null) { > description = FlexibleStringExpander.getInstance(expression); > } > } > ... > public String getDescription(Map context) { > if (description != null) { > return description.expandString(context); > } > ... > } > > -Adrian > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |