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

Refactor interface in PDF #234

Merged
merged 2 commits into from Feb 5, 2019

Conversation

Projects
None yet
4 participants
@vojtechkral
Copy link
Contributor

vojtechkral commented Jan 23, 2019

Hi,
this is a follow-up to the discussion in d39b674
This PR is not meant to be merged as-is, but as a place to discuss the interface of the additional surfaces (PDF, PS, SVG, ...).

I did the refactoring on PDF so far as an example (the others are analogous I believe).

I tried my best to reconcile the requirements mentioned in the talk under the commit, let me know if I forgot something.
Hopefully none of the changes are too offensive.

Basically there's the basic File type, I think that one is unchaged as far as public interface is concerned. Then there's Writer and RefWriter, the naming is inspired by Cell and RefCell but is open for bikeshedding - let me know if you prefer different naming. These two additional surface types Deref to the File. RefWriter is basically the same as the former Writer and Writer is the same thing except taking the ownership of the io::Write thing passed in.

In the end I decided to leave the W: io::Write type as part of the Writer and RefWriter types, this allows for single boxing and no boxing respectively and also allows to get the W back from the Writer using finish().

Please see the tests to get a feeling of how the interface is used.

cc @meh @sdroege

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Jan 23, 2019

Also we should remember to remove the old pdf_surface.rs 🙂

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 24, 2019

@meh please also review this :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 24, 2019

Also we should remember to remove the old pdf_surface.rs slightly_smiling_face

Can you add a commit to this PR, doing exactly that?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 24, 2019

It would also be good to summarize in the commit message what the goal of this refactoring is (what it should enable, for example) and what the exact changes are :) I didn't follow the old discussion closely so the changes don't tell me much as is currently.

vojtechkral added some commits Jan 23, 2019

Refactor interface in PDF:
- Unify the former Stream, Buffer, and Writer type as RefWriter
- Add the Writer type to enable taking io::Write by move
- Share implementation by having Writer and RefWriter Deref to File
- Add AsRef<File> on them as well to enable genericity
- Reduce number of closure layers

@vojtechkral vojtechkral force-pushed the vojtechkral:pdf-iface branch from a0edd71 to 6d8bdc0 Jan 25, 2019

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Jan 25, 2019

@sdroege Sure, I added and explanation to the commit message, hope it makes sense. Also added the commit removing pdf_surface.rs. For me the most important differences are:

  • I can move an io::Write into a surface (rather than reference it with a lifetime) and get it back once drawing is done (this was sort-of possible with the old interface as well but required boilerplate / wasn't straightforward)
  • We can add more function bindings to the surface in a normal way without having to add them to 4 distinct types via a macro or something

vojtechkral referenced this pull request Jan 25, 2019

@meh

This comment has been minimized.

Copy link
Contributor

meh commented Jan 25, 2019

I need to look more carefully at this next week, have some concerns about stuff related to writing to a buffer, can someone ping me on Hangouts or IRC next week if I forget please?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 30, 2019

@meh you forgot

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 5, 2019

@meh No release until you review this!

@meh

This comment has been minimized.

Copy link
Contributor

meh commented Feb 5, 2019

Okay, I don't mind the changes wrt. Buffer, the stuff I have still works with this refactor, with minor changes.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 5, 2019

Since @meh agrees, let's merge. Thanks @vojtechkral !

@GuillaumeGomez GuillaumeGomez merged commit 9b8caae into gtk-rs:master Feb 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 5, 2019

@meh I hope I didn't mess up anything for you :) Thank you for looking into this.

@GuillaumeGomez I didn't mean for this to be merged directly, but I guess it's ok if everyone else is ok with it. In any case, I should follow up with equal refacroring in PS & SVG and write docs. I will look into that sometimes in the next couple of days. Thank you as well.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Feb 6, 2019

@GuillaumeGomez yeah this was merged before it was actually finished :) Now we have to either wait for it to be finished or revert it before the release, I guess.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 6, 2019

Damn, I was sure it was done. Well, let's wait. :-.

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 6, 2019

I'll speed it up. Got SVG done on train today, will do the rest later on...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 6, 2019

Awesome!

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Feb 6, 2019

GuillaumeGomez added a commit that referenced this pull request Feb 10, 2019

Merge pull request #236 from vojtechkral/pdf-iface
Finalize PDF, SVG, PS work from #234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.