Skip to content

Conversation

Flamenco
Copy link
Contributor

@Flamenco Flamenco commented Nov 9, 2014

@Flamenco
Copy link
Contributor Author

Flamenco commented Nov 9, 2014

added parameter check to link options
@Flamenco
Copy link
Contributor Author

@MrRio
Copy link
Member

MrRio commented Nov 11, 2014

This looks pretty good to me at first glance. What do you reckon @diegocr ?

Will just need to test in a few more clients and check against the spec, but initial thoughts are – nice work!

@Flamenco
Copy link
Contributor Author

@MrRio Here are some comments regarding issues I encountered working on jsPDF. I am posting them here for thoughts before I submit any bug reports.

When trying to overlay a link rectangle on text I ran into the issue of needing to find out the baseline height of the text. The text() method draws on the baseline. So I need the baseline height for the font. HTML5 canvas gives the option of drawing on the top, bottom, middle, or baseline; This can be easily implemented if we know that specification. Thus, also having a context stack for the drawing mode would be very useful to push and restore that and other settings. I started working on an ICanvas interface in GWT. It uses the same signatures as HTML5 canvas, but can also have jsPDF doing the work. This way I can convert all my existing canvas code to non-rastered PDF format. So I would +1 the approach of all new APIs to follow the canvas.context2d signatures if they exist.

To create the table of contents, I needed to get id numbers before the object was output, and provide all sorts of links to each other in the PDF source. I provided a hack for this, but in generaI I would suggest a lazy rendering scheme, and only output after the document has been loaded into an AST. You can see this kind of approach in the the outline plugin’s render method.

I am unable to get a callback for the first page created, because it is created before my plugin is initialized. I think it would be useful to statically register a plugin, and have a 'new document callback' before the first page is created. Perhaps this exists already and I missed it. Also I could not figure out how to statically register for the ‘initialized’ event. This is why I have the initialize…() methods in my test file. IMHO that is just silly and should fixed before making this public.

Some of the text measuring methods migrated out of the main codebase and into plugins. I think they should be moved into utility package, or back into the main file. I had to include 2 other plugins just to get the text width.

Overall, I found the source code to be well organized and quite readable. Kudos!

Fire a plugin event on the static PubSub before the first page is added.
@Flamenco
Copy link
Contributor Author

This issue was handled in the latest commit.

I am unable to get a callback for the first page created, because it is created before my plugin is initialized. I think it would be useful to statically register a plugin, and have a 'new document callback' before the first page is created. Perhaps this exists already and I missed it. Also I could not figure out how to statically register for the ‘initialized’ event. This is why I have the initialize…() methods in my test file. IMHO that is just silly and should fixed before making this public.

@diegocr
Copy link
Contributor

diegocr commented Nov 16, 2014

I am unable to get a callback for the first page created, because it is created before my plugin is initialized.

Why to you need to "initialize" and "install" your plugins? None of our other plugins requires this, basically because the inclusion of them is the initialization itself (Ie, setup stuff at runtime)

Shouldn't that work? That way, you can properly listen to "AddPage" events, i think the fromHTML plugin for example does this to add header/footers.

@Flamenco
Copy link
Contributor Author

My plugin adds information when pages are created. The current PubSub creates the first page before the callback is invoked. Correct me if I am wrong, but the first page never invokes the current callback.

This would not be problem if the ctor did not create the first page before the plugins had a chance to subscribe.

I am 'initializing my plugin because there is currently no 'static PubSub' that allows for a callback to a registered plugin before a pdf has been created, initialized, or first page created.

@diegocr
Copy link
Contributor

diegocr commented Nov 16, 2014

You should be able to do it this way - at runtime - and your function will get invoked for the first page, too.

(function (jsPDFAPI) {
    'use strict';
    jsPDFAPI.events.push(['addPage', function() {
        // ...handle addPage event...
    }]);
    jsPDFAPI.myFunction = function (args) {
        // ...
    };
})(jsPDF.API);

@Flamenco
Copy link
Contributor Author

@diegocr Thanks for that. I will remove plugin registration, as that approach worked well.

@Flamenco
Copy link
Contributor Author

Initialize is still called after addPage, and that is causing me issues, as I want to set up my plugin before any pages are added.

changed event name to match conventions
@Flamenco Flamenco force-pushed the master branch 2 times, most recently from 8afaa2a to 981d249 Compare November 17, 2014 14:53
@diegocr
Copy link
Contributor

diegocr commented Nov 23, 2014

The easier workaround could be rather than listening to initialized add this to your addPage event:

jsPDF.API.events.push([
 'addPage', function(info) {
    if(info.pageNumber == 1) {
           // ...your former _initialized_ code...
    }
    this.annotationPlugin.annotations[info.pageNumber] = [];
  }
]);

If that is not enough, feel free to add events.publish('pre-initialization'); before the _addPage() on the jsPDF constructor.

@Flamenco
Copy link
Contributor Author

Have you had a chance to review the recent changes? I am on the fence of rewriting the entire library in Java/GWT vs taking the time to work with jsPDF.

@Flamenco
Copy link
Contributor Author

If that is not enough, feel free to add events.publish('pre-initialization'); before the _addPage() on the jsPDF constructor.

I personally feel you should call 'initialized' before addPage'.

@diegocr
Copy link
Contributor

diegocr commented Nov 23, 2014

Have you had a chance to review the recent changes?

981d249? briefly checked it, and looked ok.

I am on the fence of rewriting the entire library in Java/GWT vs taking the time to work with jsPDF.

Well, as with any OpenSource project, forks are always welcome. I think JavaScript would still have a bigger user-base, though :)

I personally feel you should call 'initialized' before addPage'.

Certainly i think that too, but i'm afraid of breaking existing code. That's why i suggested a harmless new event being dispatched.

@Flamenco
Copy link
Contributor Author

@diegocr Thanks for speedy reply. The only reason I bring up GWT is that it takes me extra time to fork/merge/adapt, so I seek assurance that jsPDF will welcome my contributions in the main codebase.

I would also know how you feel about building an AST and not 'writing' until the document has been built.

Thanks!

@diegocr
Copy link
Contributor

diegocr commented Nov 23, 2014

To be honest i won't feel very comfortable moving jsPDF to GWT/AST/Whatever since i personally like coding in plain JavaScript. You're of course free to maintain your own repo with that approach if you want, and ideally sending JS PRs back with your progress :-)

It's true that jsPDF has several weak areas on how it works, but i think nothing we can't resolve without moving away from the way we code it atm.

Also, there's no doubt someone who comes to contribute to a repo for the first time with a long awaited feature will be taken seriously, and very welcome. It's normal we may take a little to adapt ourselves to new codebases, said by experience, so i'm just trying to be a mentor to make a perfect contribution :)

@Flamenco
Copy link
Contributor Author

I would never expect the javascript project to convert. I actually love the language. My concern is that the current codebase does not extend itself well to outline and annotations among other things. My plugin uses a workaround as you can see, but it is not ideal.

So I plan on rewriting some of the internal code to delay the rendering of pages. I can do it in GWT 4x faster, but that won't help the greater community. So it would be helpful to know if you are open to this approach. The engine would build a tree, and the document would not be rendered until the first call to output(). This would be internal and will not affect the current API.

I wrote a GWT wrapper around jsPDF API, so either way I can leverage either approach in my own code. If I start rewriting javascript internal code and it is not welcomed in the main branch, it will be a maintenance nightmare for me, so that is why I bring it up.

@diegocr
Copy link
Contributor

diegocr commented Nov 23, 2014

I see. Well, if you're sure that won't break existing code, and deploy the generated JavaScript tested against our online demos and examples, i think it might be fine.

However, i never used GWT nor saw what code it generates, i just got aware it can generate JavaScript from Java code... perhaps you could post some example code for some of our functions ported that way (Ie, jsPDF-JS -> Java -> JS will be that?)

I'm afraid if it radically changes our internal "structure" we might make the nightmare to our current users/devs, dunno.

@Flamenco
Copy link
Contributor Author

I would not use GWT if you were OK with internal changes to the pdf rendering. I would stick with javascript in that case, as it is more universal. I will start a wiki page for future discussion of the benefits and issues with of an alternate rendering approach, and perhaps we can take it from there. Anyway, the 2 plugins I added work fine with the 'workaround'.

GWT in a nutshell is simply java compiled to browser specific javascript. (Similiar to C compiled to native assemby.) You can also 'wrap' raw javascript and but call it from java. I have 10mil+ lines of code in apps and libraries, and use GWT extensibly to leverage the refactoring and type checking that java offers. Its faster coding, and more maintainable for me vs raw javascript.

I should probably post my wrapper so other GWT developers can harness the goodness of jsPDF :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You should make this function private to your plugin scope.

@diegocr
Copy link
Contributor

diegocr commented Nov 23, 2014

Other than that, just tested your plugins and everything seems fine, so let's merge! :)

Thanks for your excellent work.

diegocr added a commit that referenced this pull request Nov 23, 2014
Initial support for annotations (links, etc.)
@diegocr diegocr merged commit adb4e49 into parallax:master Nov 23, 2014
@eKoopmans eKoopmans mentioned this pull request Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants