Reunite with jsPDF?

[Uzlopak]

Hi,

is there interest to unite this fork with jsPDF?

Best Regards Aras

[yGuy]

We would be open to this, however we had to do some major -slightly API incompatible- refactorings regarding the transformation handling in jsPDF. That's why we created the fork in the first place. I guess it would be possible but it would require a major version step in the original jsPDF - I am positive that this change would be for good.

If someone manages to do the merge, we would definitely be willing to help maintain "our" code parts in the original repo, but we are currently not willing to adjust our code to the (in our opinion) broken original implementation. If someone else wants to hop in, that would be highly appreciated, though.

[Uzlopak]

Nice. I am aware that this is not a one day job. I am right now trying to get first the codecoverage of jsPDF to about 80%. Then I would try to merge other successful forks into the main project :)

Can you give some information, which implementation is broken?

[Uzlopak]

Da ich grade sehe, dass du Tübinger bist: Können wir nicht direkt auf deutsch schreiben?

Viele Grüße aus Düsseldorf

[yGuy]

I could communicate in German, however on GitHub I would definitely prefer English so that others can join in the discussion.

The one major "broken implementation" was mainly conceptually broken. See the early commits done by @HackbrettXXX - jsPDF reapplies a transform for each and every element whereas our fork applies a global initial transform at the root level to cater for the differences between the PDF and screen/SVG coordinate systems once and for all. Without this change adding nested transformations quickly becomes a maintenance and coding nightmare.

[HackbrettXXX]

Any more thoughts on this topic?

I, too, think it would be a good idea to merge the two repos again. However, I agree with @yGuy that we need to adopt "our" way of transforming between the two coordinate systems, as nested transformations are nearly impossible when transforming each individual y-coordinate.

All in all I think the effort for the merge should not be too big. The trouble is only that we would need to perform the merge all by hand as the diff is too large. Some of the plugins might need to be modified, as well, as we don't use them and thus never bothered updating them.

Another suggestion by me: When we're already making such big changes, we might as well do some refactoring and code-style cleanup, as at least the jspdf.js file is kind of messy.

What are your thoughts?

[yGuy]

Let's discuss this face to face some time in July. I am open to your suggestions. Let me know your availability!

[Uzlopak]

Hi, right now I have exam time and actully total busy. I will come back to this topic after I am finished with the exams

[yGuy]

@arasabbasi Actually I meant to answer @HackbrettXXX - we'll keep the public updated.

[yGuy]

We'll be working on minimizing our patches so that we still have a fork, but that fork will only add new functionality and should be backward compatible with the original jspdf. That would make it theoretically possible for the original project to merge this project's enhancements. Our code will basically use the new API, because the original API is just too limiting/broken with respect to transformations.

[Uzlopak]

Today we released a new version of jsPDF. The next milestone will be to extend jsPDF with your forks features :). So we should have in version 1.6.0 all your changes in jsPDF. :)

[yGuy]

I guess you've seen that we basically just reformatted the original code using (our preferred settings of) Prettier.io . After reformating, the diff will be much smaller. But maybe you already had the idea of applying a sane and consistent code style in the original branch ;-)

[Uzlopak]

No. I didnt see that. :) I tried to get at first a good percentage in unit testing in jspdf.js . It is now at about 92 %. I will try to merge step by step your code into the origin branch. At the end i can check how prettier.io works and run it over jspdf.

Is this a deal? :)

[yGuy]

I think this will make it a lot more difficult for you. It's probably a lot easier to first run prettier (with the same settings) over the master branch and then do the diff and merge. The diff is actually extremely small. We took special care about that. Basically after running prettior on the mrRio "branch" we are only a few commits behind and the diff is really very small. If we had to merge in your changes it would only take us a few hours at the most, I guess. And the end-results should be the same. So if you consider running prettier over MrRio's code, then you should really do that as a first step as the second step will be vastly simplified. Just my 2 cents.

[Uzlopak]

I started the merge https://github.com/MrRio/jsPDF/pull/2214

[Uzlopak]

I made some API changes, so that maybe yWorks can use jsPDF by modifying some method-calls.

[Uzlopak]

I think there are some huge merge errors. Can you please provide some test cases, so that i can do some integration tests?

[yGuy]

What's wrong with our set of test files?
I would also like to remind you that the diff was actually minimal and the merge was almost trivial (see my previous comment). I don't understand why you made it so difficult for you (and now us). Please understand that we are currently not willing to invest more time into this specific merge, knowing that from our point of view this is just a waste of time and there are better options to solve this. Maybe later in the year when there is more time. Thanks!

[Uzlopak]

What's wrong with our set of test files?

The tests in your fork simply don't pass. Just clone your project, npm install and run test-local. Most tests are red, which means they fail. The same with the tests in svg2pdf.js.

Nobody asked you for more "investments". Just working tests.

[HackbrettXXX]

When I run the tests, they are all green:

[Uzlopak]

Do you use linux or any other non-windows OS?

[HackbrettXXX]

I use Windows. I already thought that this might be an encoding problem. Would be great if you could investigate this. Pull requests are welcome.

[Uzlopak]

Committig with git removes \r\n to \n thus resulting in the errors.

Anyhow... I am getting closer to the goal of merging the two repos. ;).

I am testing the merge against the svg2pdf library. Probably will add the tests and svgs of svg2pdf to jsPDF to keep them as integrity tests. Is that ok with you?

[bernhardreiter]

Please keep this efforts going, I think everyone will benefit in the end, especially https://github.com/yWorks/svg2pdf.js .

[HackbrettXXX]

There are news on our side in case you are still interested in a reunion.

I made the effort to merge all changes from MrRio/master. The diff is now very small. Our fork is once again fully compatible with MrRio's version and all tests are green.

The following differences remain:

• Two API modes on our side (one that is compatible with MrRio but has restricted functionality, one that offers full functionality)
• The default float precision is 16 on our side while removing trailing zeros (MrRio uses different values between 2 and 3). Note that I modified some of the reference files because of the float precision changes.

I also fixed some issues that are also occur upstream:

• The build process creates files that don't work with requirejs script loading
• Fixed https://github.com/MrRio/jsPDF/issues/2518
• Fixed jsPDF#save() (getArrayBuffer was broken)
• Fixed some issues in the parts that were merged from our fork (e.g. matrix multiplication order)
• See my recent commits for more

Note also that some parts you merged into MrRio from our fork don't work properly there. One example is the setCurrentTransformationMatrix(), which, when called with e.g. rotation matrices, will mess up all future coordinates passed to the drawing operations.

So in order to reunite the two forks only the following steps would remain:

• run prettier on MrRio's side
• merge the changes from the yWorks fork. This should currently work without

What are your thoughts @arasabbasi? I still think both sides would hugely benefit from a reunion.

[Uzlopak]

@HackbrettXXX

Hi, unfortunately I am totally busy with my current occupation. If you make a PR I am going to merge it.

[HackbrettXXX]

That are great news. I'm very glad you are open to this merge.

Here is the PR: https://github.com/MrRio/jsPDF/pull/2545

[1valdis]

I believe the issue should now be closed? And the jspdf-yworks NPM package should be marked deprecated and lead to jspdf in the Readme somewhere? I'm just evaluating if I should use jspdf and the fork makes things a little more confusing ;)

[HackbrettXXX]

Yes, that's the ultimate plan ;)