Bug 271837

Comment 1 Neil Hauge 2009-04-09 15:34:29 EDT

That last example should look like this:

<em ...>
        <entity class="asdf.Pet">
                <attributes>
                        <m-t-o name="type"></m-t-o></attributes></entity>
        <entity class="asdf.Employee">
                <attributes>
                        <basic name="endDate"></basic></attributes></entity>
        <entity class="asdf.Owner">
                <attributes>
                        <basic name="city"></basic></attributes></entity></em>

Comment 2 Carl Anderson 2009-04-13 00:57:08 EDT

The question here seems to be which is better, too many line breaks (which breaks some ill-behaved tools), or too few?

This patch was discussed in both bug 205371 and bug 182480.

Does the current lack of line breaks cause tooling issues?  Or is your concern moreso of human readability?

Comment 3 Neil Hauge 2009-04-13 18:18:16 EDT

(In reply to comment #2)
> The question here seems to be which is better, too many line breaks (which
> breaks some ill-behaved tools), or too few?
> 
> This patch was discussed in both bug 205371 and bug 182480.
> 
> Does the current lack of line breaks cause tooling issues?  Or is your concern
> moreso of human readability?
> 


This does not cause tooling issues that I am aware of, but yeah, it just looks ugly.  I personally think it is worse than the too many breaks issue.

Comment 4 Shaun Smith 2009-04-17 10:24:07 EDT

Human readability is critical for Dali. 

Developers can interact with XML either directly or through rich UI.  In the case of the JPA orm.xml file, users can manipulate the open XML file though views.  Because of this bug, as users change setting in a view the visible XML file gets reformatted strangely.

I cannot stress enough how unpleasant this bug is.

Comment 5 Carl Anderson 2009-04-22 20:51:08 EDT

Created attachment 132863 [details]
Switch all display-name to not split into lines

This patch undoes the change for bug 205371, and sets the GenericTranslator for all <display-name> to always use the format:

<display-name>Name</display-name>

instead of:

<display-name>
     Name
</display-name>

Comment 6 Carl Anderson 2009-04-22 20:57:36 EDT

I would like Chuck, Jason, and Nitin to review this- I am a little leery about this change.  

First, the good: it undoes the horrendous formatting introduced by bug 205371, so that all of the end tags are now back on separate lines, when appropriate.  Second, this now handles the most common case exactly as one would expect it to be handled- the display name entry that is most commonly used is nicely wrapped in its start and end tags on the same line.

Now, what worries me:  A display name list is actually used.  The main usage for multiple entries is to allow for xml:lang to be defined so that the display name can be put into different languages.  In that rare case, this does seem to be a poor formatting choice.  (And while that is moreso uncommon to have for the display name of the main Deployment Descriptor object, this change affects all display names, such as the display name of a security role, where multiple entries for different languages could be used and are more visible.)

However, given the horrendous state of our xml formatting at present, I feel that this trade-off is worthwhile.

Comment 7 Nitin Dahyabhai 2009-04-22 22:32:07 EDT

Carl, what would a display name list look like before and after this patch?

Comment 8 Carl Anderson 2009-04-22 23:05:41 EDT

It has been a bit since I dealt with this (infrequent) scenario.  If I recall correctly, ideally, it should look like:

<security-constraint>
	<display-name>Visitor</display-name>
	<display-name xml:lang="es">Visitante</display-name>
	<web-resource-collection>
		<web-resource-name>Public locations</web-resource-name>
		<url-pattern>/Public</url-pattern>
	</web-resource-collection>
</security-constraint>

Which means that my coding should be correct.  The original coding would have looked like:

<security-constraint>
	<display-name>
		Visitor
	</display-name>
	<display-name xml:lang="es">
		Visitante
	</display-name>
	<web-resource-collection>
		<web-resource-name>Public locations</web-resource-name>
		<url-pattern>/Public</url-pattern>
	</web-resource-collection>
</security-constraint>

Whereas the current code formats it as:

<security-constraint>
	<display-name>Visitor</display-name>
	<display-name xml:lang="es">Visitante</display-name>
	<web-resource-collection>
		<web-resource-name>Public locations</web-resource-name>
		<url-pattern>/Public</url-pattern></web-resource-collection</security-constraint>

Comment 9 Neil Hauge 2009-05-05 14:43:57 EDT

It would be great it we could get this change done in time for 3.1 RC1 (and of course 3.0.5).

Comment 10 Carl Anderson 2009-05-05 15:22:31 EDT

This is a stop-ship as it has a negative effect on end user readability.
The only workaround is for the end user to reformat the xml.
This has been extensively tested to make sure it complies with Java EE 1.2 through 5.0 for all module types.
Nitin and Jason have both reviewed this fix.  The fix undoes the removal of the carriage return that was introduced with bug 205371, and correctly causes the display-name to have its beginning and end tags on the same line. (see comment #8 for examples of how it used to look and how it wil look now.)
This is a low risk fix.

Comment 11 Carl Anderson 2009-05-05 20:20:13 EDT

Committed to R3_0_maintenance for WTP 3.0.5 and HEAD for WTP 3.1

(And in case it wasn't clear, I asked for PMC approval since WTP 3.1 is now in RC1.)