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 #34

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

Refactor #34

wants to merge 2 commits into from

Conversation

diegomura
Copy link
Collaborator

No description provided.

@devongovett
Copy link
Member

You don’t like classes? 😜

@diegomura
Copy link
Collaborator Author

diegomura commented Nov 28, 2018

Haha it's not that. Maybe I shouldn't have pushed this yet. It's a WIP, but that thrown very good results so far. Let me explain a bit what this is 😄

The original cause of this refactor are some issues I was having with the original solution:

  • Ignoring chars after glyph generation: Since glyph generation was occurring very early on the process, it was pretty hard to consider some scenarios in which you wanted to ignore or remove some chars of the string. One case was breaking words based on soft-hyphens. Glyph generation created glyphs for these chars, so the only way of dealing with this was iterate and remove them later in the process.
  • High coupled classes: I like classes (actually react-pdf is full of them), but I encounter many issues whatever I wanted to fix something in the core, and was in part because the classes were not very independent from each other (specially after adding the dependency injection feature).
  • Hard to add new features: Related to the previous point, but in the sense that the old algorithm was a bit fragile. Adding new features easily broke old stuff.
  • Hard to test: Wasn't easy to write accurate tests for the core algorithm, specially because all the logic was inside one main big loop statement

Don't get me wrong. The current solution is awesome and has proven to have value on react-pdf. However, I wanted to give it a shot on a refactor that follows two main heuristics:

  • Try to calculate as many things as early as possible (specially before glyph generation). Word breaking can be one example of this. It's faster, easier and more reliable to operate through strings rather than GlyphStrings. Makes it possible to create more simple and powerful engines that mutates the strings without having to worry about glyphs yet.
  • Try to divide the problem (which it's crazy hard in some ways) into small and testable bits. And imo following a functional approach in a problem such as typesetting can be very beneficial. Each part is now independent (immutable in the future. Some already are) and easy to test. This has proven already to be so much simple to debug and add new features on the layout pipeline.

I didn't want to change the API of the lib (which is not immutable), so there are still some things that can be confusing, but based on some tests (with some other enhancements on the react-pdf side) I observed a performance boost of ~25%. It's pretty visible on large documents.

Anyways, would love to hear your thoughts! There's still some work to do on this repo, but I plan to release a react-pdf with this solution hopefully soon, after testing it a bit further. I'm now in a point in which 100% of the documents I tried (some of them very complex), passed successfully.

@diegomura
Copy link
Collaborator Author

@devongovett would yo be ok to merge this? It's being tested on react-pdf for awhile now and it's working very good. Having all functional like this really makes the entire problem more easy to maintain and adding new features. For me, having this merged would make the development process (both for react-pdf and textkit) way more easy 😄

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