Change the default value for overflow to visible as CSS specs say and…

[hvbtup]

… allow specifying overflow in the style editor.

[hvbtup]

I have no idea why the CI build failed. The code seems to work.

[wimjongman]

The maven log is clear. Please look at BaseMessageFileTest.testDuplicateMessages and check how your code could influence it.

Failures:
MessageFileTest>BaseMessageFileTest.testDuplicateMessages:218 duplicate messages in line 2796

[speckyspooky]

The issue fix should be very simple: It is caused on the message file "i18n/Messages.properties".
And yes, BIRT will run but the test case of duplicate property values is the hint and I see a doubled value of "Element.Style.overflow".

[hvbtup]

I see a doubled value of "Element.Style.overflow".

Ah, ok, that explains it.
I saw the text "Element.Style.overflow" in the UI instead of a human-readable text and so I looked for where the text for showIfBlank was defined. I added a line for Element.Style.overflow to one of the properties files, but that had no effect, so I added it to the other file as well. Obviously one of the files already contained such a line...

[wimjongman]

Please rebase your changes on the latest in master.

[hvbtup]

@wimjongman @speckyspooky @claesrosell @ruspl-afed

What do you think, can I squash and merge this?
Nobody answered to #1261 so I think most people don't care; and even though this might be considered a breaking change, the old behavior was clearly a bug IMHO.

[wimjongman]

Don't you think changing the default would break existing reports? I agree with you, but we should probably leave the default as it was in that case. It is up to you to assess the risks for existing users.

[hvbtup]

Don't you think changing the default would break existing reports?

That's why I have asked for opinions in #1261.

From my personal experience and from my my colleagues experience after > 13 years of developing BIRT reports, I can say that the user expectation is to never automatically cut text because that loses information and can be misleading.

In rare cases, there were exceptions: These were always for generating labels, when available space was precious and descriptive texts sometimes had to be cropped.
But label printing always uses the PDF emitter, which we always used with the emitter options I described in #1261, so the default value for PDF output (in our clients environments) was always equivalent to "overflow: visible".
And thus we always explicitly set "overflow: hidden" in these rare cases anyway.

Reports in English or Asian languages probably never face this issue at all, because the words are just shorter.
So only speakers of languages with a tendency to long words (e.g. German and AFAIK Dutch, Swedish) will care at all.
How do experienced developers use BIRT? Is our configuration and usage (as described above) exotic or common?

BTW newer BIRT releases contained breaking changes in the past several times, most of the times due to newly introduced bugs which we had to work around. As an example, the value of 0 for page-break interval meant "infinite" before 4.3.0 and was forbidden in 4.3.0, so we had to change the setting to 9999 in all our reports.

I also thought of keeping the old (wrong) default "hidden" and make it overridable somehow, to keep backward compatibility.
But I don't see how this should work.
The main reason being that the property is not inheritable.
Otherwise one could just add the property to the "report" style.

The other way round, the only workaround I found to make the existing DOCX emitter not crop text, was to create a generic script in the beforeFactory event which set the overflow property for all data items, labels and dynamic text items.
However, I didn't find a way to do this for the MasterPages, but OTOH in reality such problems do not occur on MasterPages anyway.

I also don't see how setting this as a render option (see point 3 in #1261) should work reasonable.
The problem here is that the default value is already a part of rom.def. And it's just unclear to me how this could be implemented and at the same time allow the user to explicitly specify the property.

Another option which in fact should be possible would be to implement point 4 in #1261: Increment the version number in the design files.
The default value would then depend on that version number, so probably we would have to version the rom.def file as well.
Note that the default value is defined in different places, apart from rom.def it is also in one of the Java files.
I think this would make the source code more complicated.

To summarize:

• I think this is a straightforward solution to a common problem.
• I believe that most of the users will benefit from the change.
• I could not find a better solution (ideas are welcome!)
• Of course in rare cases the change may be unwanted. The solution is then to explicitly set the property directly or using a style.

[hvbtup]

I created a simple test report. The report is just a narrow grid and demonstrates the different ways to create text with BIRT.
Now that the property can be specified in the style editor, it is only necessary to add/adjust 4 styles to existing reports in order to restore the old behavior:

Report design:

Styles (I set Hidden for all 4 styles):

Output:

Note that the Dynamic Text HTML case does not work as intended, but I don't think that this is a new issue (which means that the property never really worked for the DOCX emitter).
EDIT: Confirmed: Overflow Hidden didn't work for HTML text even before this PR.

The same report with the new default setting for overflow: "Visible":

[wimjongman]

Ok you convinced me. Thanks for the patch!

[merks]

@hvbtup @wimjongman

I'm trying to revive many of the tests, including org.eclipse.birt.report.engine.emitter.html.HTMLReportEmitterTest which fails because the expected results are different. But more of a concern is that the new result does not even properly start with a document tag as emitted by openRootTag. Here you can see the new call to the new org.eclipse.birt.report.engine.emitter.html.HTMLReportEmitter.addCellDiagonalSpecial() method is before the call to openRootTag():

birt/engine/org.eclipse.birt.report.engine.emitter.html/src/org/eclipse/birt/report/engine/emitter/html/HTMLReportEmitter.java

Lines 716 to 752 in 15d8a1e

	addCellDiagonalSpecial();
	if (isEmbeddable) {
	outputCSSStyles(reportDesign, designHandle);
	if (needFixTransparentPNG) {
	fixTransparentPNG();
	}
	fixRedirect();
	openRootTag();
	writeBidiFlag();
	// output the report default style
	if (report != null) {
	String defaultStyleName = report.getDesign().getRootStyleName();
	if (defaultStyleName != null) {
	if (enableInlineStyle) {
	StringBuffer defaultStyleBuffer = new StringBuffer();
	IStyle defaultStyle = report.findStyle(defaultStyleName);
	htmlEmitter.buildDefaultStyle(defaultStyleBuffer, defaultStyle);
	if (defaultStyleBuffer.length() > 0) {
	writer.attribute(HTMLTags.ATTR_STYLE, defaultStyleBuffer.toString());
	}
	} else if (htmlIDNamespace != null) {
	writer.attribute(HTMLTags.ATTR_CLASS, htmlIDNamespace + defaultStyleName);
	} else {
	writer.attribute(HTMLTags.ATTR_CLASS, defaultStyleName);
	}
	}
	}
	outputDIVTitle(report);
	outputClientScript(report);
	return;
	}
	openRootTag();

That can't be okay.... I'm going to try to fix this as part of enabling more test for this:

#1291

[hvbtup]

@merks Hmm, I think this the wrong PR for this. If the issue is caused by addCellDiagonalSpecial called in the wrong place, you'd better contact @speckyspooky.

[merks]

I did. I think it’s okay now. The test passes with well-formed html 5.