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

Protected surfaces #1539

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

piranna
Copy link
Contributor

@piranna piranna commented Mar 21, 2020

This abstract and protect the actual Cairo surfaces, making them an implementation detail of the backends, preventing to be created or destroyed from outside. This allow for greater isolation of concepts, allowing to access to the actual Cairo surface just only by using the getSurface() method, preventing any other direct access.

  • Have you updated CHANGELOG.md?

@piranna
Copy link
Contributor Author

piranna commented Aug 3, 2020

Can this be reviewed and merged?

@piranna
Copy link
Contributor Author

piranna commented Oct 17, 2020

Code sync'ed with latest master branch and all tests are fixed and passing in all platforms. Can this pull-request be reviewed and merged? :-)

@piranna
Copy link
Contributor Author

piranna commented Sep 23, 2021

Can this be reviewed and merged?

@@ -914,8 +914,6 @@ Canvas::resurface(Local<Object> canvas) {
Nan::HandleScope scope;
Local<Value> context;

backend()->recreateSurface();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind explaining this change? I'm not to familiar with this part of the code 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember the exact details, but reviewing the PR, seems to me like a bug, because recreateSurface() it's not being called anywhere else...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe best to add it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, or test it first as is, to be sure if it crash or fails.

Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Sorry for not getting to this earlier! Just one small comment, otherwise it looks very good 👍

@piranna
Copy link
Contributor Author

piranna commented Jan 18, 2022

otherwise it looks very good

Thank you! :-)

@zbjornson zbjornson force-pushed the master branch 2 times, most recently from 64ed3d8 to ff0f2ab Compare December 28, 2023 23:22
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.

2 participants