Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BIRT 4.14 PDF output of dynamic text which does not fit on the page #1443

Closed
reiner-killet opened this issue Sep 29, 2023 · 31 comments
Closed

BIRT 4.14 PDF output of dynamic text which does not fit on the page #1443

reiner-killet opened this issue Sep 29, 2023 · 31 comments
Assignees
@speckyspooky
Labels
BugFix Change to correct issues Help wanted
Milestone
4.14

Comments

@reiner-killet
Copy link

Hi developers,

is the following change in behavior intended or is it a bug?

A grid or table cell contains a dynamic text.
The HTML is longer than the page.
The Page Break properties of the table row is set to Inside : avoid.
The General properties of he Dynamic Text are set to Display : Block.

In BIRT 4.8 the Dynamic Text starts on the current page (if there is no object like an image that prevents that).
In BIRT 4.14 the Dynamic Text starts on the next page.
To let 4.14 start the Dynamic Text on the first page like 4.8 the General properties must be changed to Display : Inline.

Thanks, Reiner
@hvbtup
Copy link
Contributor

hvbtup commented Sep 29, 2023

I think this is a bug.
I stumbled across this myself recently.

The behavior is as if the dynamic text item has page-break-inside: avoid set.

@speckyspooky
Copy link
Contributor

speckyspooky commented Sep 29, 2023 •
edited
Loading

@hvbtup Henning, you are able to provide a fix of that, it seems that you are familiar with that topic.

@reiner-killet Could you provide an example again so that we have the same "error"-template. This would be great.

@hvbtup
Copy link
Contributor

hvbtup commented Sep 29, 2023

@speckyspooky If you mean "are you able to provide a fix for that?" - I don't know. And this is low on my priority list ATM. I hope I find time to investigate further in October.

@reiner-killet You can help us if you perform a binary search by download BIRT releases between 4.8 and 4.14 and see which release exactly introduced the issue.

@speckyspooky
Copy link
Contributor

I have tried to produce the error with a very simple report based only with a "grid" and I tested with the mentioned properties 3 element: text-element (type: html), dynamic-text, label.
My text is very long and I shrinked the page height to 120mm to get the effect faster.

But I didn't get your effect. So I think that there is an additonal component or configuration which cause your behavior.

Result of the test report

dynamic-text

I add my report perhaps you see the difference. The report includes 1 parameter
to switch between the 3 text-elements (controlled through the visibility).

Test report

page_break_report.zip

@hvbtup
Copy link
Contributor

hvbtup commented Oct 4, 2023

I failed to create a simple test case.

The situation where I'm facing the issue is in a very complex report.
grafik

Here I see the list header text "Beurteilung" on one side with huge free space below, and the comment text starts on the next space.
Note that in my case there are no tables or grids involved.

@reiner-killet It would be really helpful if you can tell us the exact BIRT release where this behaviour change started.

@hvbargen
Copy link
Contributor

hvbargen commented Oct 5, 2023

@reiner-killet Is the issue specific to an emitter, eg only with PDF output?

@reiner-killet
Copy link
Author

reiner-killet commented Oct 9, 2023 via email

@hvbtup
Copy link
Contributor

hvbtup commented Oct 9, 2023

@reiner-killet Probably you attached a ZIP file, but this doesn't work when replying by email. Please use the browser to upload the example.

@reiner-killet
Copy link
Author

New trial:
Issue 1443.zip

@hvbtup
Copy link
Contributor

hvbtup commented Oct 9, 2023

OK, I can reproduce the issue with your report.
It will probably be hard to debug this, because the report is quite complex. There's a lot of boilerplate before/above the header "Note:".

@hvbtup
Copy link
Contributor

hvbtup commented Oct 9, 2023

As a first step, I simplified the layout. This version still shows the issue: The note HTML text starts on the second page, but some lines would fit on the first page, so it should start there.
PSA_RPT_APM_BRT_ENG.zip

@hvbtup
Copy link
Contributor

hvbtup commented Oct 9, 2023

Further simplification of the test report.
PSA_RPT_APM_BRT_ENG.zip

@hvbtup
Copy link
Contributor

hvbtup commented Oct 9, 2023

I tested with some older BIRT releases downloaded from https://archive.eclipse.org/birt/downloads/drops/:

  • BIRT 4.2.1 same behavior as 4.13: HTML text starts on page 2
  • BIRT 4.2.1 same as 4.2.1
  • BIRT 4.4.2 correct behavior (IMHO): HTML starts on page 1 and continues on page 2.
  • BIRT 4.8.0 correct behavior, like 4.4.2
  • BIRT 4.9.0 same as 4.13

So this should give us at least an idea where to look in the commit logs...

@speckyspooky
Copy link
Contributor

Short hint I tested the report from your side Henning and if the report will be started with the Viewer so the wrong page break is given too and if the DOCX is created from the viewer (instead) directly we will have the same effect.
If the HTML-emitter is used directly like DOCX (without viewer) the result looks good.

So we have the effect on the HTML-viewer side and on the PDF (in every case).

@hvbtup
Copy link
Contributor

hvbtup commented Oct 10, 2023

This issue might be related to commit 16290e5

The goal of this commit was to fix BIRT bug 562873 PDF Rendering error with dynamic text item after page break.
This bug was introduced by an older commit, which I reverted more or less.

Part of the changes was:

grafik

My gut feeling tells me this is related.
But... Back then, I reverted an older commit without knowing exactly how it works.
Just "Reverting the revert" would introduce bug 562873 again.

A PR which tries to fixes this issue should also be tested with the report from bug 562873.

@hvbtup
Copy link
Contributor

hvbtup commented Oct 10, 2023

Sigh... If I use the previous condition in the if statement shown above, this solves this issue, but results in bug 562873 again.

@hvbtup
Copy link
Contributor

hvbtup commented Oct 10, 2023

Commit history related to this file that might help in understanding:

9827648 (2009)
"Bug 28147 Page Break for contents in grid cells does not work correctly in PDF...":
This commit added the if statement which sets setPageBreakInside = avoid.

2257c41 (2013)
"Fixed Page break added after large dynamic text within a table on PDF 53266":
This commit fixed avoided the page break by sharpening the condition in the if statement.
Unfortunately this resulted in bug 562873.

16290e5 (2020)
"Fix BIRT bug 562873 PDF rendering error with dynamic item after page break":
This commit reverted the aforementioned commit, among others, resulting in the issue at hand.

So the situation is like this:

  • There are two issues: bug 562873 and this one.
  • We have got source code that fixes one of the issues but results in the other issue.
  • Most of the other committers are busy, trying to get rid of outdated 3rd party libraries.
  • ATM the situation with unwanted page breaks is still better than unreadable content.
  • The source code is a djungle for me as well as for the other committers, I think.
  • Unfortunately there is no high-level overview how it actually works.

I assume that setting pageBreakInside = avoid was not the right solution in the beginning.
But I'm out of ideas what to do instead.

@reiner-killet If you need this fixed, you should try to find a solution yourself.
As a workaround, you can try to split your HTML note into lines somehow and use a list item to iterate over them.

@reiner-killet
Copy link
Author

reiner-killet commented Oct 10, 2023 via email

@speckyspooky
Copy link
Contributor

I started a smaller research on the original report from your side Henning which cause the change.
Yes we have there an issue but the reason of the moved first line after page break is not an issue of the page break it is the "margin-top".
Because you set in your report 10mm margin-top on grid level and if I change the value then the first line is moved back.
The same effect is given if the "dynamic text" is used directly (without grid) and there is an margin-top. SO there is the same effect that the first line is moved.
So it seems for me that the original issue of it is the margin-top calculaton of the PDF-emitter at one of the drawText()-methods.

Gird with margin-top and moved 1st line of dynamic text

top-grid-dynamic-text

Danymic text with margin-top and moved 1st line

top-dynamic-text

@hvbtup
Copy link
Contributor

hvbtup commented Oct 11, 2023

I think it is an issue regarding the combination of page-break and margin-top in the calculation of the y position for the first line.
If I comment the if-statement in AreaFactory.java and set the margin-top of the Problem-Grid to 30mm, then the first line is rendered even below the second and third lines:
grafik

@hvbtup
Copy link
Contributor

hvbtup commented Oct 11, 2023

If I add a 1px border to the master page and set margin-top of the Problem-Grid to 30mm, then print the second page, I can see that yellow area starts 58mm below the page border-top, which is correct (28mm page header height + 30mm margin-top for the grid). But the first line is written approx. 25mm below the start of the yellow area. And the 1pt padding-top of the cell seems to be ignored.

@hvbtup
Copy link
Contributor

hvbtup commented Oct 11, 2023

If the "misplaced first line" issue from bug 562873 occurs is depending at least on

  • the space left on the first page
  • the margin-top and padding-top values of the dynamic text item
  • the padding-top value of the cell.

But I don't see a pattern.

@hvbtup
Copy link
Contributor

hvbtup commented Oct 11, 2023

I added a simplified example report demonstrating bug 562873 in extrem form. The first line is written even below the second and third line (assuming the if-statement is commented).
lwk_prb2.zip

@speckyspooky
Copy link
Contributor

I have a suggestion for an solution/fix. The reason at all is the thinking of the caluclation.
Because I figured out a part of the source code at LineArea-class which calculate the start position based on the container which is around the text.
But the problem is that with "page break" the calculation doesn't recognize the new situation and so there is at the end a kind of height-offset given which moves down the first line. This is very specific because on of the points is that the page-break was caused through the margin-top which is the cause of the break and the (logical) area is splitted so into 2 part 1st page & 2nd page but the margin- shouldn't be repeated again.

Long text, short suggestion of LineArea:

  • 2 new lines to reset the height to 0 if the text is splitted and the height is negativ:

line-area

My results looks good. Yes we need some further tests. The risk here is limited at the LineArea and has no effect on other area-container.
If this is working we should reset the AreaFactory to the original if-conditon.

@hvbargen
What do you think about this fix-suggestion?

@hvbtup
Copy link
Contributor

hvbtup commented Oct 12, 2023

Looks good to me.

@speckyspooky
Thomas, I suggest you create a PR (I'm really busy ATM with other things).
Please find attached a patch and two reports I used for testing.

The patch is basically what you suggested, plus:

  • I added a comment referencing this issue and bug 562873 beneath your comment in LineArea.java.
  • Added some text to the method description of ContainerArea.java methode getContentHeight (as I understand it)
  • I removed the if-statement in AreaFactory.java completely because I don't see the point of it.
  • The seemingly large change in BlockContainerArea.java is just result of correcting the indentation.

issue-1443.zip

It tested the two reports with the PDF, DOCX and HTML emitters, and with the web viewer.

  • The results with the PDF emitter look good.
  • The DOCX emitter seems to ignore margin-top for dynamic text objects, but I think this is not a new issue and unrelated to this change.
  • The Web Viewer does not work correctly for 1443-1.rptdesign. Again, I think this is not a new issue.
    grafik
  • And of course, if one exports from the web viewer, the result cannot be better than the result in the web viewer.

hvbtup added a commit to triestram-partner/birt that referenced this issue Oct 12, 2023
@hvbtup
Fix for issue eclipse-birt#1443: page break and HTML text
631c7db
speckyspooky added a commit to speckyspooky/birt that referenced this issue Oct 12, 2023
@speckyspooky
Fix area line margin-top of first line after page break (eclipse-birt…
be51a8e 
…#1443)
@speckyspooky speckyspooky mentioned this issue Oct 12, 2023
Merged
@speckyspooky
Copy link
Contributor

I have created th PR #1449 to fix it.

@speckyspooky speckyspooky added this to the 4.14 milestone Oct 12, 2023
@speckyspooky speckyspooky self-assigned this Oct 12, 2023
@speckyspooky speckyspooky added the BugFix Change to correct issues label Oct 12, 2023
speckyspooky added a commit that referenced this issue Oct 14, 2023
@speckyspooky
Fix area line margin-top of first line after page break (#1443) (#1449)
5c22cbe 
* Fix area line margin-top of first line after page break (#1443)

* Remove debug comment
@speckyspooky
Copy link
Contributor

The issue is solved with PR #1449

@reiner-killet
Copy link
Author

reiner-killet commented Oct 17, 2023 via email

@speckyspooky
Copy link
Contributor

Currently there are some issues with the eclipse-designer/rcp-report-designer. The nightly built has an issue with the viewer component so you won't be able to create a preview.

BUt if you have an own integration you can use the runtime for testing.

@reiner-killet
Copy link
Author

Is it in the most recent RCP designer, meanwhile?

@speckyspooky
Copy link
Contributor

The work is currently concentrated at the Jetty-migration for the web-viewer and if it is done you can test it.
But currently the version isn't finished so you cannot test yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@speckyspooky speckyspooky

Labels
BugFix Change to correct issues Help wanted
Projects
None yet
Milestone
4.14
Development

No branches or pull requests
4 participants
@hvbtup @hvbargen @speckyspooky @reiner-killet