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

Added ponoko-specific changes to color and line width. #105

Closed
wants to merge 1 commit into from

Conversation

wayn3w
Copy link
Contributor

@wayn3w wayn3w commented Dec 12, 2018

This feature adds support for creating boxes destined for Ponoko. The user's workflow is to specify
scripts/boxes --format=svg_Ponoko
The user will then import the resulting svg into one of the three Ponoko-supplies templates.

In order to support this feature, I used a python generator for saving and restoring the cairo context around color changes so there won't be a need to remember the previous color. I kept other refactoring to a minimum.

@florianfesti
Copy link
Owner

That's clearly going in the right direction.

I would prefer if self.ctx would be encapsulated with both the saved() context manager and the new color handling.
While I have not yet decided on replacing cairo as a graphics library I would like to see cairo specific being removed from the API. Those two are basically the only place left where the ctx object needs to be used by generators. Everything else is hidden in some Boxes methods for drawing stuff.

This should not be too difficult. Just make saved a method of the Boxes class and add a method for setting the color.
I would probably also have this as three separate patches: new saved method, color handling and new format. But I am fine to split that up myself if you don't feel comfortable with the git magic needed to do that.

I would probably move all the generator over to the new saved method. But this is out of scope here and can be done later on.

@florianfesti
Copy link
Owner

saved() should probably also do the get_current_point() move_to() dance. cts.restore() can't as it's just a cairo function. For most cases a Boxes.moveTo() call is following the ctx.restore(). So the ctx.move_to() is not needed. But just a few days a go I had someone running into this pitfall.

@wayn3w
Copy link
Contributor Author

wayn3w commented Dec 14, 2018 via email

@florianfesti
Copy link
Owner

https://stackoverflow.com/questions/6217156/break-a-previous-commit-into-multiple-commits explains how to split up patches.
After the git reset you can git add -i the pieces you want in the commit and then just commit the new patches one after the other. It's not hard. Just a bit scary at first. I often create a new branch that I split up and keep the old one around to be able to easily look at the original patch.

@wayn3w
Copy link
Contributor Author

wayn3w commented Jan 22, 2019 via email

@florianfesti
Copy link
Owner

Ok, this PR is superseded by #112

Closing here.

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.

None yet

2 participants