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.

Sign up for GitHub

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

Jump to bottom

introduce TARGET_COLLECTOR for target-counter, -counters and -text #572

Closed
wants to merge 14 commits into from
Closed

introduce TARGET_COLLECTOR for target-counter, -counters and -text #572

wants to merge 14 commits into from

Conversation

Tontyna
Copy link
Contributor

@Tontyna Tontyna commented Feb 11, 2018

See #23 -- implements target-counter, target-counters and target-text. But (due to the current limits of WeasyPrint) a TOC-with-page-numbers is not included...

After at least 5 different approaches (oh my! Python is not my favorite language!) I eventually came up with a TARGET_COLLECTOR -- new module weasyprint.css.targets -- which is prepared by anchor() and content() in css.computed_values.

In formatting_structure.build the anchor elements call TARGET_COLLECTOR.store_target() to store their counter_values in the TARGET_COLLECTOR and the pseudo-elements call TARGET_COLLECTOR.lookup_target() to fetch them.
If the target comes later in the document -- aka `PENDING' -- the creation of the content box is suppressed. Instead the stuff required to create it later is kept.

Finally TARGET_COLLECTOR.check_peding_targets() is called to fill the PENDING content boxes.

The fist step of course was impelmenting the target-* tokens in css.validation.validate_content_token().

There is not (yet) a unit test...in the meantime you may run it with LOGGER.setLevel(logging.DEBUG)

Tried my best to conform to the coding style of WeasyPrint, not shure whether I succeeded...

@liZe liZe added the feature New feature that should be supported label Feb 16, 2018
@liZe liZe added this to the 43 milestone Feb 16, 2018
@liZe
Copy link
Member

liZe commented Feb 16, 2018

Thanks a lot for your contribution!

The overall logic is OK and there's a lot of interesting code in this PR! The main problem is the use of global variables, because it's not safe when WeasyPrint's used to render multiple documents as a library.

I'll merge it in a separate branch, clean a couple of things and add tests (and check the W3C testing suite). Thanks again!

@Tontyna
Copy link
Contributor Author

Tontyna commented Feb 16, 2018 •
edited
Loading

Don't like the global variable either.
Only resorted to one cause it was the least "code-disturbing" way to make the TargetCollector available to the 6 places where it actually is used. Of course a TargetCollector should be created in Document._render() an handed down, like the styles and element etc. via an additional function parameter.

When assimilating the source code I often was tempted to create something like the computer in computed_values.compute() or enhance the LayoutContext and use it not only in the page margin layout. I guess it's historically grown that build.py doesn't use a LayoutContext?

Concerning counter manipulation in @page context, counter(page) in document's pseudo elements and access to document based counters in page margins, the LayoutContext could be the candidate to collect and provide everything that's needed to control pending content and repeated pagination.

@Tontyna
Copy link
Contributor Author

Tontyna commented Feb 17, 2018

This PR, besides being just a raw proof of concept, makes the target-functionality available for content only. A final implementation should introduce it for string-set and bookmark-level as well.
That would not only lead to PENDING content boxes but also to PENDING strings (aka Box.string_set and Box.bookmark_label).

At the moment the validation of content and strings and the layout/computation of involved elements is done by different functions, doing almost the same.

Volunteering to eliminate the redundant code, I'm uncertain: Shall I wait until you merged and cleaned up my PR or would you like me to do that beforehand or do you prefer to do it by yourself?

@liZe
Copy link
Member

liZe commented Feb 17, 2018

Volunteering to eliminate the redundant code, I'm uncertain: Shall I wait until you merged and cleaned up my PR or would you like me to do that beforehand or do you prefer to do it by yourself?

You can update the PR, I'll merge it as soon as you feel it's ready to be cleaned.

@Tontyna
BugFix: Logging Errors
cce1f39 
caused by wrong parameters for LOGGER.debug -- relicts from
using print()
@Tontyna
Copy link
Contributor Author

Tontyna commented Feb 17, 2018

Ok. Gimmi a few days...

@Tontyna
Merge branch 'master' into target-counter
2b2eebd
@Tontyna
cleanup code to pass Travis CI check
087cb3a
@Tontyna
once again: cleanup code to pass Travis CI check
d35f455
@Tontyna
Reduce duplicate code in css validation
57f3234 
Modified function `validate_content_list_token()` in
weasyprint/css/validation.py so that it can be used to validate
the css properties `content`, `string-set` and `bookmark-label`.
@Tontyna
consolidate (tuples) and [lists]
71cee36
@Tontyna
Copy link
Contributor Author

Tontyna commented Feb 26, 2018

OMG! I really hate this nitpicking Travis!

@Tontyna
Prepare removal of duplicate code
f68a5fa 
Preparation for code-reduction in the box-building steps and
allowing target-*() not only in `content` but also in `string-set`
and `bookmark-label`.

Therefore weasyprint/css/computed_values.py now computes the
`<content-list>`s of `string-set` and `bookmark-label` in the same
way as it already does for `content`.
@Tontyna
Copy link
Contributor Author

Tontyna commented Mar 3, 2018

What's wrong with "dejavu.tar.bz2" in Travis Build Job for OSX?

BTW: The WeasyPrint unit test suite isn't suitable for my Win64 Laptop, running with 120 dpi screen and having no "ahem" font. Indeed, I was shocked, when pytest reported "32 failed" for the first time...

@liZe
Copy link
Member

liZe commented Mar 3, 2018

What's wrong with "dejavu.tar.bz2" in Travis Build Job for OSX?

The Sourceforge site is currently in "Disaster Recovery mode" (sic), static files cannot be downloaded anymore without JavaScript. If the problem doesn't disappear soon, we'll have to change the testing script to download it from somewhere else.

The WeasyPrint unit test suite isn't suitable for my Win64 Laptop, running with 120 dpi screen and having no "ahem" font. Indeed, I was shocked, when pytest reported "32 failed" for the first time...

The 120 dpi screen shouldn't be a problem or it's a bug. Ahem needs to be installed, we could use @font-face rules but that's not supported on Windows anyway. If you could install Ahem and DejaVu, and tell me what's not working on Windows, I'd be happy to help!

@Tontyna
Copy link
Contributor Author

Tontyna commented Mar 3, 2018

With Ahem installed only 22 tests fail (instead of 32). Most errors seem to originate from _check_status() in cairocffi:

E  cairocffi.CairoError: cairo returned 41: b'error occurred in the Windows Graphics Device Interface'

Python is Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40) on Windows (64 bits).
Installed it about a month ago, everything should be quite up-to-date. So the root of evil might be Windows 7...
Any ideas? Shall I create a new issue and attach the full pytest log (>10700 lines, even with --quiet)?

@liZe
Copy link
Member

liZe commented Mar 3, 2018

Any ideas?

You can try to update cairo to the current unstable version (1.15.10) and see if it fixes the problem. (I don't know how to do this, or even if it's easily possible.)

Shall I create a new issue and attach the full pytest log (>10700 lines, even with --quiet)?

If you almost always get the same error, having one traceback ending with cairo returned 41: b'error occurred in the Windows Graphics Device Interface' should be enough.

@Tontyna
Remove duplicate code: target-* for all
c0c706f 
In the box building and page layout steps, the CSS properties
`content`, `string-set` and `bookmark-label` now use the same code
to evaluate their <content-list> values.
As a concequence the target-* functions work for all three properties.
@Tontyna
Copy link
Contributor Author

Tontyna commented Mar 3, 2018

Next step should be the replacement of the global TARGET_COLLECTOR with something multi-thread-safe.

Promoting a local instance through all the functions called during box building and layout is a fiddly task. I guess, my 10 fingers won't be enough to count all the defs that must be altered.

The only sensible solution I can think of -- making the whole formatting thing an object-oriented job of the Document -- would be a big redesign too... not mentioning the unit tests 😏

@Tontyna
Copy link
Contributor Author

Tontyna commented Mar 3, 2018

Ok. Will create an issue and try my luck with Cairo.

@liZe liZe mentioned this pull request Mar 27, 2018
Merged
@liZe
Copy link
Member

liZe commented Mar 27, 2018

Replaced by #604.

@liZe liZe closed this Mar 27, 2018
@Tontyna Tontyna deleted the target-counter branch May 1, 2018 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature New feature that should be supported
Projects
None yet
Milestone
43
Development

Successfully merging this pull request may close these issues.
2 participants
@Tontyna @liZe