Background image with new option of dpi scaling and BIRT-properties #1255

[speckyspooky]

The changes of these request includes the new posibilities that BIRT is able to use the image resolution of the original image dpi to create the correct scaling of the image. In addition to it the base properties background image height and width can be used. The both properties are also able to be used on script level.

The change includes the changed components:

• designer
• preview
• emitter.HTML
• emitter.PDF
• emitter.PPTX

Till now the emitter wasn't able to scale the images. This option is added. To bet a better handling of all, the base class "background image info" is enhanced to implements the main functions and new properties.

Overview of changed classes:
Overview-Changed-Classes-1255.xlsx

Examples of testing:
Test-Examples-1255.zip

Expamples of results:
Result-Examples-1255.zip

[hvbtup]

I didn't look at the code, just at the output.

First of all, great work! This is absolutely OK for my use case.

Do I understand correctly that "auto" (or not specified) the width and height is computed from the image's pixel and its DPI information?

Did you also test with a different image without DPI information (such that the image has intrinsic proportions, but not intrinsci dimensions?

If one dimension is specified and the other is auto, the behavior is not as expected according to https://developer.mozilla.org/en-US/docs/Web/CSS/background-size. Anyway, I don't mind.

[hvbtup]

I downloaded the All-In-One-Designer artifact from this PR and tested myself.
I saved the same image without DPI (I used BMP format for this).
In this case, BIRT still seems to assume that 96 DPI are specified in the image.
I also tried setting the imageDpi property of the report. This had no effect.
OK, so it's the same behavior as before.

Though personally I don't mind the distorted image if only one dimension is specified, I think others might.
According to the Mozilla page mentioned above, the image should not be distorted:

auto
Scales the background image in the corresponding direction such that its intrinsic proportions are maintained.

[speckyspooky]

My target was to get a first step of improvement to get the base handling and I would say it is given.
If I have time I will take a look after your specific constellation:

• width: 100px, height: auto
• width: auto, height: 100px

The property of the DPI-usage directly on report level make no sense because you would say in this case all images must be use the same DPI-value. But in the most cases the it is specific of each image.
The idea later is to add a new property background-image-dpi to handle it directly. But before we can do it we habe to merge the changes of diagonales and this one because we have dependencies on implementation side.
(Yes, the default is same like before on 96dpi.)

[hvbtup]

Definitely this PR is an improvement to the current situation.
I'm +1 for merging it.

[wimjongman]

Looks good to me. Please go ahead and squash and merge.

[speckyspooky]

I will double check the pull request due to the merge of the diagonal-antidiagonal changes.

Independend of the merge I have done additional testings and added 2 changes (locally):

1. I figured out 2 wrong image calculations - I fixed it
2. I implemented a first version of the scaling if only 1 value, width or height is set and the another value is "auto"

I check in the next days my standard test cases and if I can finish the test sucessfully I wil do an additional commit.

[speckyspooky]

@wimjongman Could you please re-check the additional changes before I start the merge. Thanks in advance.

[wimjongman]

This is a bit too big to be reviewed. When looking at the code, I wonder how the images are disposed of. You know that creating images is notorious for memory leaks. Also, I see a lot of code that looks duplicated. There are a lot of new setImage methods. But the code is too big and spread out over too many files for me to be able to do a proper review.

[speckyspooky]

Yes, you are right that there are some codes "seems" to be duplicated. The reason is that the containing elements are completely different objects which are inherit from different parents.
We have for the output emitter the BackgroundImageInfo-class which is the representation of the master lcass.
But for the Grid-, Table-, Row-, Cell-elements of the designer are own classes used from the original concept.
The core functionality of the background-handleing are the same in the most parts but not at all and I cannot centralize the functionality to one class of all.
Especially the HTML-Emitter is the special at all because he use special handling in additonal to all together with the AttributeBuilder.

If you want, I cann add all of my test-reports to the pull request - here my test results like PDF-output.
background_image_testcase-phase02_#1255.zip

Please let me know what I can do to offer you the according/relevant information.

[speckyspooky]

An additonal information, the central handling of background images according with the disposing of images aren't changed from my side. The enhancement is located on the calculation of the image dimenson and there usage.
Yes I know that the images are very sensible of memory leaks. With these changes we haven't more leak, we would have the leaks like before.
I cannot exact say where it is located but during my debugging I saw some point there the dispose will be fetched on cenral points.

[hvbtup]

Wow, coding after midnight!

[hvbtup]

I agree with Wim that a lot of code seems duplicated.

But that is caused by BIRT's internal concepts and code structure, as Thomas explained. I noticed this in my initial attempt (withdrawn PR).
I don't think this code smell can be fixed with reasonable effort.
And the code smell is old, it was not introduced with this PR.

Regarding image disposal, I don't know when it happens but I'm pretty sure that the images are garbage-collected when the design is and the Run/Render/RunAndRenderTasks are gc-ed.

[wimjongman]

Ok, so please merge. No guts, no glory.

[speckyspooky]

Merge is done.