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

Backends support #829

Merged
merged 2 commits into from
May 3, 2017
Merged

Backends support #829

merged 2 commits into from
May 3, 2017

Conversation

piranna
Copy link
Contributor

@piranna piranna commented Oct 26, 2016

This is a stripped-down version of pull-request #571 focused only on the
backends support, but adding also support for PDF and SVG backends and
passing the tests. it also retain backwards compatibility with the old API,
so this would be easy to merge since it's mostly to move around the PDF and
SVG code and checks to independent classes. There's some code for
pdfStream and similar methods that I think they should be moved too since
they are mostly backend specific, and make more sense there. I didn't do
that since it would break the API, but maybe it would be added some
functions on the Canvas object that proxy the petitions to the backend to
retain compatibility.

I know it's a big pull-request, so I'm open for comments over it.

This is a stripped-down version of pull-request Automattic#571 focused only on the
backends support, but adding also support for PDF and SVG backends and
passing the tests. it also retain backwards compatibility with the old API,
so this would be easy to merge since it's mostly to move around the PDF and
SVG code and checks to independent classes. There's some code for
`pdfStream` and similar methods that I think they should be moved too since
they are mostly backend specific, and make more sense there. I didn't do
that since it would break the API, but maybe it would be added some
functions on the Canvas object that proxy the petitions to the backend to
retain compatibility.

I know it's a big pull-request, so I'm open for comments over it.
@piranna piranna mentioned this pull request Oct 26, 2016
@piranna
Copy link
Contributor Author

piranna commented Nov 11, 2016

Any update on this one?

@cammanderson
Copy link

This seems promising! 👍

@piranna
Copy link
Contributor Author

piranna commented Dec 2, 2016

This seems promising!

I'm glad you liked it, let's hope if it gets merged soon :-)

@ianmetcalf
Copy link

@piranna I couldn't get the example to work, Canvas.backends was undefined. I got it to work by adding the initialization of backends on the target object in init.cc

NAN_MODULE_INIT(init) {
  Backends::Initialize(target);
  Canvas::Initialize(target);
  Image::Initialize(target);

https://github.com/ianmetcalf/node-canvas/blob/27a48c66f8ad1cd642377d5afd74d230efbab0aa/src/init.cc#L28

@piranna
Copy link
Contributor Author

piranna commented Feb 27, 2017 via email

@ianmetcalf
Copy link

I just pointed it out since this PR includes examples/backends.js which uses this new API. It would probably be good to either update init.cc or change the example to use the exiting API.

I really appreciate the work you did abstracting the backends. I'm currently working with the FBDev backend that I extracted from your master branch.

@piranna
Copy link
Contributor Author

piranna commented Feb 27, 2017

I just pointed it out since this PR includes examples/backends.js which uses this new API. It would probably be good to either update init.cc or change the example to use the exiting API.

You are right. Since it's the new behaviour, I've added the missing namespace.

I really appreciate the work you did abstracting the backends. I'm currently working with the FBDev backend that I extracted from your master branch.

Well, I have just only updated some previous code to use it with static builds for NodeOS :-P So why are you using it? Are you using it on production?

@piranna
Copy link
Contributor Author

piranna commented Apr 3, 2017

Any update on this? Can we merge it?

@piranna
Copy link
Contributor Author

piranna commented Apr 22, 2017

Ping?

@LinusU
Copy link
Collaborator

LinusU commented May 3, 2017

Awesome work! Sorry for the long delay 🎉

@LinusU LinusU merged commit 48eb938 into Automattic:master May 3, 2017
@piranna
Copy link
Contributor Author

piranna commented May 4, 2017

Thank you for merging :-)

@piranna piranna deleted the backends branch May 4, 2017 07:07
@ReneHollander
Copy link

I am a bit saddened by the fact, that there is no mention of my work on this. Just found out through coincidence, that this got merged two years after I made my first commit to my fork.

@chearon
Copy link
Collaborator

chearon commented May 4, 2017

You were mentioned in the original PR #571

@piranna
Copy link
Contributor Author

piranna commented May 4, 2017

@ReneHollander you are right, the foundational code of this pull-request is based on your work, without it I would have not been able to know how to do it, thanks :-)

@piranna
Copy link
Contributor Author

piranna commented May 4, 2017

I hope now that this got merged, if static builds support gets merged too, since now Github better support pull-requests merges without need to do rebases that we could re-use the previous pull-request as a container for several of the backends, so your work would have a direct repercusion on the node-canvas code :-)

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.

6 participants