Skip to content

Commit

Permalink
Add PDF/UA support for images (WIP), avoid PAC error about missing BBox
Browse files Browse the repository at this point in the history
  • Loading branch information
@hvbargen
hvbargen committed Sep 14, 2024
1 parent f245c16 commit c77b8a4
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
9 changes: 9 additions & 0 deletions ...irt.report.engine.emitter.pdf/src/org/eclipse/birt/report/engine/emitter/pdf/PDFPage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import com.lowagie.text.pdf.PdfDictionary;
import com.lowagie.text.pdf.PdfFileSpecification;
import com.lowagie.text.pdf.PdfName;
import com.lowagie.text.pdf.PdfRectangle;

This comment has been minimized.

Sign in to view

Copy link
@merks

merks Sep 16, 2024

Contributor

@hvbargen

I'm not even sure where to comment when you push branches directly to the main repository without a pull request.

I would really much prefer that you push changes to your own fork and create pull requests to kick off builds rather than push branches into the main repository.

The workflow for how to do that is described here:

https://github.com/orgs/eclipse-simrel/discussions/3

I.e., I don't expect to see a bunch of "personal" branches being built:

image

but rather PR builds:

image

Via the PRs there is a place to review and discuss changes.

You can easily amend your commits and force push to your fork to kick off new builds. That's also described in the discuss above as well.

Your build is failing because it isn't based on master and hence has an older target platform referring to a p2 repository that no longer exists.

Before starting a PR effort, always pull master and create your branch off master...
import com.lowagie.text.pdf.PdfString;
import com.lowagie.text.pdf.PdfTemplate;
import com.lowagie.text.pdf.PdfTextArray;
Expand Down Expand Up @@ -231,6 +232,14 @@ protected void drawImage(String imageId, byte[] imageData, String extension, flo
if (pageDevice.isPDFUAFormat() && !inArtifact) {
PdfDictionary dict = new PdfDictionary();
pageDevice.structureCurrentLeaf.put(new PdfName("Alt"), new PdfString(helpText));

PdfDictionary attributes = pageDevice.structureCurrentLeaf.getAsDict(PdfName.A);
if (attributes == null) {
attributes = new PdfDictionary();
pageDevice.structureCurrentLeaf.put(PdfName.A, attributes);
}
attributes.put(PdfName.BBOX, new PdfRectangle(imageX, imageY, width, height));

contentByte.beginMarkedContentSequence(pageDevice.structureCurrentLeaf);
}

Expand Down
7 changes: 5 additions & 2 deletions ...port.engine.emitter.pdf/src/org/eclipse/birt/report/engine/emitter/pdf/PDFPageDevice.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1194,10 +1194,13 @@ public void pushTag(String tagType) {
if ("Figure".equals(tagType)) {
// Top-Level figure elements must have a placement attribute.
if (PdfName.DOCUMENT.equals(structureCurrentLeaf.getParent().get(PdfName.S))) {
PdfDictionary attributes = new PdfDictionary();
PdfDictionary attributes = structureCurrentLeaf.getAsDict(PdfName.A);
if (attributes == null) {
attributes = new PdfDictionary();
structureCurrentLeaf.put(PdfName.A, attributes);
}
attributes.put(new PdfName("Placement"), new PdfName("Block"));
attributes.put(PdfName.O, new PdfName("Layout"));
structureCurrentLeaf.put(PdfName.A, attributes);
}
}
}
Expand Down

13 comments on commit c77b8a4

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Ed. I asked Wim if it's ok to add a branch to this repo, because this is going to be a lot of work that I'm not willing to do all alone, see the latest comments in #1234. This is explicitly not meant as a personal branch.

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really is possible to do all the same things via a fork, allowing collaborators to push to your fork's branch. Are you expecting folks to actually commit to this branch or just provide reviews and feedback?

In any case, if it's an exception to the rule, no big deal. 😄

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should explain what I did:
During my vacation last week, I started the branch with my hobby account hvbargen and it was based on the master branch then.
I pushed the branch to my company's fork using my work account hvbtup this morning, and from there I pushed to this repo.
It was 10 commits behind master at that time.

Actually I hope that others will contribute code to the branch, but I doubt that would be the case in a branch outside of the official repository, even though it's technically possible.
I expect the branch to exist for several months, with further PRs from me and others targetted to the branch.

I wasn't aware of https://github.com/orgs/eclipse-simrel/discussions/3.
I read it now, but I don't know if it does apply to the situation because it isn't the situation I started from, and more important:

I simply don't know what to do to fix the "does not build", because my knowledge of the build process and Git is limited.

You wrote: "Your build is failing because it isn't based on master".
Well, it was based on master.
Can I just rebase it on master now? How exactly? You know, I don't want to the make the situation worse by doing something wrong.

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not expert at all the git magic either...

I read this tutorial:

https://eclipsesource.com/blogs/tutorials/eclipse-git-tutorial-a-rebase-workflow-with-egit/

But it still wouldn't let me push to your branch because it wasn't fast forward.

So I pushed it to my fork and created a PR:

#1913

If you copy that URL to the clipboard you can then check it out into your clone:

image

image

image

The build is progressing well.

We can throw my PR away, or you can push that PR onto a branch in your own fork. I really have no clue how to update the branch you already created.

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I recently thought that the branch name tagged-pdf doesn't precisely describe what's the goal of the branch, a branch name like PDF-UA would be better anyway.
So I'm fine with removing the branch and creating a new one.

I consider this branch as something which will exist for several weeks or months, and will contain lots of individual commits, as if we created a new major version of BIRT, for example.

I don't know if the goal to keep a linear history is appropriate here.

Meanwhile I'm trying to build on my work PC.
While on my private PC, I started with a fresh install, my work PC had Eclipse 2023-12 installed, which fails to update.
So probably I have to install the IDE from scratch again (for the 7th time or so, which is somewhat annoying).
I think I'll merge the changes from master to the local branch, check that it builds fine, then think about how to continue.

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the setup not work well?

https://github.com/eclipse-birt/birt/blob/master/CONTRIBUTING.md

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this setup originally (in 2023). But the setup tasks failed now.
I then used the "Check for updates" (and install them) manually and now it seems to work.
After some restarts, the IDE now shows 2024-09 on the splash screen.

So this seems to be ok now, except that when starting the All-in-One run configuration, it complains about a missing image:

java.io.FileNotFoundException: /icons/palette_view.gif ...
!MESSAGE The image could not be loaded: URLImageDescriptor(platform:/plugin/org.eclipse.gef/icons/palette_view.gif)
...

But I think I can ignore this.

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally Help -> Perform Setup Tasks does a better job of updating because it also helps update the target platform too Yes, the fact that e4 keeps old URLs around that might have changed and become invalid in newer versions is a known annoying problem.

Running once with this option should fix that:

image

https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fmisc%2Fruntime-options.html

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this fixes the error messaging regarding palette_view.gif.

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my local repo, I merged the changes from the master branch into the tagged-pdf branch.
But the Maven build still fails, with an error that IMHO is unrelated to any of the changes:

Exception in thread "Thread-7" Exception in thread "Thread-3" java.lang.NoClassDefFoundError: org/eclipse/jgit/internal/JGitText
at org.eclipse.jgit.internal.util.ShutdownHook.cleanup(ShutdownHook.java:85)
at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.ClassNotFoundException: org.eclipse.jgit.internal.JGitText
... 2 more
java.lang.NoClassDefFoundError: org/eclipse/jgit/internal/JGitText
at org.eclipse.jgit.internal.util.ShutdownHook.cleanup(ShutdownHook.java:85)
at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.ClassNotFoundException: org.eclipse.jgit.internal.JGitText
... 2 more

This happens when I run the build either by executing the "BIRT Tycho Build" run target in the IDE or after pushing the changes to my fork triestram-partner/birt and executing the CI action.

I found eclipse-jgit/jgit#36 which claims that this is fixed with JGit 6.10.0, but according to the installation details of my BIRT IDE, it is using JGit 7.0.0:

Image

Any ideas?

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, everyone's builds have this annoying stack trace at the end. It doesn't fail the build, but it does look like an error in the build.

Image

It may be fixed in jgit, but the version of jgit that Tycho 4.0.8 uses is the broken one. What you have installed in the IDE has no impact on what is being used by the Tycho build which runs in a separate process using whatever dependencies that Tycho 4.0.8 specifies. I expect Tycho 4..0.9 to be released soon, and then hopefully this problem is gone (and there are no new ones).

@merks
Copy link
Contributor

@merks merks commented on c77b8a4 Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Tycho 4.0.9 release is out with the problem fixed:

eclipse-tycho/tycho#3489 (comment)

I committed these changes to the master branch:

d87640e

I can confirm this fixes the problem.

@hvbtup
Copy link
Contributor

@hvbtup hvbtup commented on c77b8a4 Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for info: Running the tests, I found that the EngineIRIOTest fails, regarding the tagType attribute. I'll have to find out, why this happens.

Please sign in to comment.