Thanks Scott - those are my feelings exactly.
I like the way the design worked previously, and changing it because a
user might accidentally leave the comments enabled in production seems
silly. That is a user's QC problem, not a widget comment design problem.
-Adrian
On 9/13/2011 8:55 AM, Scott Gray wrote:
Sorry I'm a bit lost, is the latest proposal something like this:
if (widget.properties.enableBoundaryComments == null) {
// widget.properties not set, only show where specified as true
if (context.enableBoundaryComments != null) return
context.enableBoundaryComments;
return false;
} else if (widget.properties.enableBoundaryComments) {
// widget.properties set to true, show everywhere unless specified as false
if (context.enableBoundaryComments != null) return
context.enableBoundaryComments;
return true;
} else {
// widget.properties set to false, show nowhere
return false;
}
As opposed to what was originally implemented:
if (context.enableBoundaryComments != null) return
context.enableBoundaryComments;
if (web.xml.enableBoundaryComments != null) return
web.xml.enableBoundaryComments;
if (widget.properties.enableBoundaryComments != null) return
widget.properties.enableBoundaryComments
return false;
I still prefer the original solution, in my experience the 99% use case for
this setting is:
- widget.properties.enableBoundaryComments=Y for development (ideally comes
this way OOTB)
- developer uses web.xml or context to set to N for apps/screens where they
NEVER want the comments to show up (maybe some CSV screen or something)
- widget.properties.enableBoundaryComments=N for production
I just can't imagine a reason why widget.properties would be set to N and a
developer would decide to turn it on via the web.xml or context and
subsequently cause it to be shown in production (i.e. the reverse of the use
case I've described above). It seems like we're complicating an issue that is
very unlikely to arise and only did arise because the example app's web.xml was
set to Y when it shouldn't have been (which confused Hans and caused him to
make sweeping changes instead of just commenting out the web.xml setting).
I really don't think there is a perfect solution but I'd rather advocate a
simple one that can easily be remembered.
Regards
Scott
On 13/09/2011, at 6:41 PM, David E Jones wrote:
Yes, there is some value in being able to have boundary comments always on… so
Adrian are you coming around to agree with the approach Hans introduced?
Why don't we use the widget.properties setting if there is one, otherwise look
at the parameters Map (i.e. request parameters and web.xml context params). By
default, i.e. in SVN, the value will NOT be set.
-David
On Sep 12, 2011, at 6:45 PM, Adrian Crum wrote:
So we would do away with the ability to specify that boundary comments are
always on?
Having them on by default during development is very helpful - I use them all
the time. I can view the page source of any screen to see where the widget XML
file is located that generated the screen. It seems to me with the method you
proposed, I will not be able to do that - because comments will be off. I would
have to hack the URL and add a parameter to see them, or I would have to modify
the application's web.xml file and restart the server.
-Adrian
On 9/13/2011 2:22 AM, David E Jones wrote:
Based on this I'm actually reconsidering my position, however the current
implementation is still not adequate.
It sounds like the goal for the widget.properties is to make it easy to go into
production and make sure that no boundary comments/etc are added anywhere in
the system. To do that effectively you need a single setting for the whole
system, and that setting should override everything else (i.e. not even allow
for a parameter to be manually added which may expose implementation details
that you want to keep hidden).
For that purpose a property would make sense, but the logic has to be carefully
done (not the shallow logic that has been discussed so far). It would need to
be something like: if (widgetVerbose property == false) then don't show else if
(widgetVerbose parameter (using default OFBiz parameters Map, takes into
account both URL parameters and web.xml context parameters) == true) then show
else don't show.
In other words, if the widget.properties setting is false, then never show
boundary comments. Otherwise, ignore it and use the parameters value if true,
and overall default to false.
Wow, is this really that hard?
-David
On Sep 12, 2011, at 5:03 PM, Hans Bakker wrote:
As i wrote before i am fine with this if in the trunk the setting of
widget.properties is not overridden by default in web.xml for some
component what was the case originally.
Regards,
Hans
On Mon, 2011-09-12 at 20:02 +0100, Adrian Crum wrote:
David,
Keep in mind that the original design is one that you participated in.
The agreement on the setting precedence in the original Jira issue was this:
widget.properties -> web.xml -> URL parameters
where widget.properties is the global default, which can be overridden
by a setting in web.xml, which can be overridden by screen widgets or
scripts or whatever (via the current context Map).
The design worked great. Then Hans changed it due to a misunderstanding
of how the design works. Despite repeated explanations of how the design
works, and requests from three PMC members to revert his change, he
refused to change it and threatened the community with a commit war.
Since then we have had a number of issues reported on the mailing list
describing how his change makes the setting unusable.
It amazes me that a single -1 vote vetoes a change in the Apache
community, but three -1 votes from PMC members can't revert this obvious
break in software design.
-Adrian
On 9/12/2011 7:24 PM, Adrian Crum wrote:
No. The approach suggested by (and committed by) Hans is that the
setting in the widget.properties file overrides any other setting.
-Adrian
On 9/12/2011 6:19 PM, David E Jones wrote:
No one agrees with which approach? The approach that if you pass a
widgetVerbose=true HTTP parameter that it should override the
widget.properties setting? I agree with that approach…
-David
On Sep 12, 2011, at 6:59 AM, Adrian Crum wrote:
That's the problem - no one agrees with that approach.
-Adrian
On 9/12/2011 1:53 PM, Jacques Le Roux wrote:
I think I forgot to forward Hans's answer
Jacques
Hans Bakker wrote:
On Wed, 2011-08-31 at 05:15 +0200, Jacques Le Roux wrote:
widget.properties's widget.verbose setting has precedence over
web.xml's widgetVerbose setting. So you can't use
parameters.widgetVerbose to override widget.verbose to false. Is
ModelWidget.widgetBoundaryCommentsEnabled() written this way for
some reasons?
there was a lengthly discussion of this. As long as by default the
properties file is not overridden in web.xml is fine either way.
Another issue is that these HTML boundary comments get outputted
even though the view handler is set to "screencsv". In the
widget-screen.xsd, the only way to invoke a template to produce
CSV is using<html><html-template />, but this always adds HTML
comments even if the output is CSV (see HtmlWidget class). Maybe
we could introduce a<csv> element or something like that?
Anyway, both of those problems combined mean that there are no
apparent clean ways to remove the HTML "template begin/end"
boundary comments from the CSV output if you try to draw it with
an *.ftl template. A workaround kludge for now is to invoke
the FTL manually through a Groovy script.
Thanks
Jacques
--
Ofbiz on twitter: http://twitter.com/apache_ofbiz
Alternative ofbiz website: http://www.ofbiz.info
http://www.antwebsystems.com : Quality services for competitive rates.
