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.

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

Preliminary PDF/UA support #1972

Merged

Conversation

hvbtup
Copy link
Contributor

@hvbtup hvbtup commented Nov 14, 2024

As I said yesterday, I think my changes are now in a state that they do no harm to the master branch; creating normal PDFs should work exactly as before.
It is now possible to at least generate valid single-page PDF/UA files.
The smiling sun image is my IP, I created it a few years ago for a game experiment.

I cannot split this into more smaller commits in a reasonable way.

Note: The Java code certainly has some room for improvements.
In several cases I used the instanceof operator, because it was the easiest solution and avoids having to change interfaces.

BTW most of the changes in EngineIRVisitor.java were suggested by the IDE to remove warnings.

@hvbtup hvbtup added Help wanted Enhancement Small change to improve the current supported functionality labels Nov 14, 2024
@hvbtup
Copy link
Contributor Author

hvbtup commented Nov 14, 2024

@wimjongman

One of the tests is failing:
EngineIRIOTest.testIO:39->doTestIO:72 EngineIRIOTest, doTestIO(compare: golden, value) - designName: action_test.rptdesign expected:<... <label id="8" t[ag-type="P" text="uri-expr"> ... (output shortened) ... but got:<... <label id="8" t[ext="uri-expr"> ...

This obviously means that a tag-type attribute got lost. This might be related to one of my changes: AFAIK the tags are case-sensitive, for example it must be P and not p.
I changed this in model/org.eclipse.birt.report.model/src/org/eclipse/birt/report/model/elements/rom.def

Probably now the value of P is not written to the file because P equals the default value.

So I think I have to modify the "golden" file, do I?

@wimjongman
Copy link
Contributor

@hvbtup Yes, go ahead.

@hvbtup
Copy link
Contributor Author

hvbtup commented Nov 14, 2024

Nah. By examining the test, I see that the "golden" image is not a file. It is the report, loaded with ReportParser().parse.
This is then serialized with EngineIRWriter and deserialized with EngineIReader. The first and the final report are both saved with ReportDesignWriter. And obviously ReportDesignWriter actually writes the tags into the rptdesign file, but when using the long way through the IR, they get lost.

@speckyspooky
Copy link
Contributor

@hvbtup
I have dine a first review to understand your changes and I think I understand your steps. But I will do a second read of it. The only thing in generally I saw some comments in German and file name "Sonne.png". It would be good if we have all comments in English and the image name is optional for me.

@hvbtup
Copy link
Contributor Author

hvbtup commented Nov 18, 2024

I kept some German in the example reports and the file name, but I removed the German comments and cleaned up the code.

@merks
Copy link
Contributor

merks commented Nov 18, 2024

I hope this can be squashed into a single commit rebased commit rather than these 25 commits with merge commits before this is completed...

@hvbtup
Copy link
Contributor Author

hvbtup commented Nov 18, 2024

@speckyspooky
The build fails with errors that seem unrelated to my changes. I don't know why.

@wimjongman
Copy link
Contributor

I hope this can be squashed into a single commit rebased commit rather than these 25 commits with merge commits before this is completed...

Yes, our strategy is to SQUASH.

@merks
Copy link
Contributor

merks commented Nov 18, 2024

@hvbtup

FYI, this is broken because of this breaking change in GEF:

eclipse-gef/gef-classic#617

I think that change needs to be reverted. I'll see if I can push this through quickly, and restart this build when that's fixed.

@merks
Copy link
Contributor

merks commented Nov 18, 2024

BTW, this is one of the reasons why during most of the develop cycle I use the most recent available dependencies:

image

I.e., to catch problems as soon as possible after they occur upstream.

Very soon the 2024-12 cycle will come to a close; rc1 is this week and rc2/final is next week. At that point I will try to lock down the repos to permanent location so that release tags can be built in the future. And hopefully BIRT can release shortly after the December 4th release...

@wimjongman
Copy link
Contributor

wimjongman commented Nov 18, 2024

BTW, this is one of the reasons why during most of the develop cycle I use the most recent available dependencies

Yes. I am impressed by the speed you caught this. 👍

@merks
Copy link
Contributor

merks commented Nov 18, 2024

@hvbtup

FYI, the PR is functional again with the GEF breakage fixed now.

@hvbtup
Copy link
Contributor Author

hvbtup commented Nov 21, 2024

For the release notes, it should be clearly stated that PDF/UA support is a Work-In-Progress which just started.

In particular, it is not yet possible to generate accessible PDFs where items (eg tables) span more than one page, even though the PAC 2024 says the generated PDF is valid.

Furthermore, existing BIRT reports won't conform to PDF/UA just like that. It is always necessary to carefully set advanced properties in many places.

@hvbtup hvbtup requested a review from speckyspooky November 21, 2024 10:05
Copy link
Contributor

@speckyspooky speckyspooky left a comment

Choose a reason for hiding this comment

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

Go from my side.
Yes, we should address the prototype character for that feature.
But it is a very good starting point and more like starting point!

@wimjongman
Copy link
Contributor

Thanks, Henning!

@wimjongman wimjongman merged commit 8099b94 into eclipse-birt:master Nov 22, 2024
3 of 4 checks passed
@hvbtup hvbtup deleted the preliminary-PDF-UA-support branch November 22, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Small change to improve the current supported functionality Help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants