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

Don't copy styles only to change margins, borders and paddings #497

Open
liZe opened this issue Aug 17, 2017 · 4 comments
Open

Don't copy styles only to change margins, borders and paddings #497

liZe opened this issue Aug 17, 2017 · 4 comments
Labels
performance Too slow renderings

Comments

@liZe
Copy link
Member

liZe commented Aug 17, 2017

Many situations force to override (often remove) margins, borders and paddings, including:

  • table cells with collapsing borders,
  • blocks split between pages,
  • inline boxes split by block boxes.

Depending on the situation, the style is copied and modified, or the computed values are applied to the box (box.padding_right and its friends). There's no perfect solution, as the first one greatly increases memory usage, and the second one breaks min/max-width calculation and box copy.

We have to find a solution to:

  • never copy nor modify style dicts,
  • override margins, borders and paddings on styles without modifying the dict,
  • avoid mixing box.style['margin_left'] and box.margin_left (as it's done in mix/max-width calculation on boxes after dimensions are already set on the box),
  • keep speed (i.e. don't use the copy-on-write method used before, as it was overriding the __getitem__ method that's over-optimized, see 9abc034).
@liZe liZe added the feature New feature that should be supported label Aug 17, 2017
@liZe liZe added this to the v0.41 milestone Aug 17, 2017
@liZe
Copy link
Member Author

liZe commented Aug 23, 2017

In this wonderful blog entry, the solution I was thinking of is given in the chapter called "A sidenote: style struct sharing".

liZe added a commit that referenced this issue Sep 8, 2017
Style dicts are now standard dicts, with better performance and less memory
consumption. Styles are not copied when possible and replaced by ChainMaps.

This breaks Python2 compatibility.

Related to #497.
@liZe
Copy link
Member Author

liZe commented Sep 8, 2017

4057a92 uses real dicts for performance and use ChainMaps to keep original dicts when changing padding/borders/margins, breaking the Python2 compatibility.

@liZe liZe removed this from the v0.41 milestone Oct 4, 2017
@liZe
Copy link
Member Author

liZe commented Dec 27, 2019

See #948 (comment) for more information about where to remove style edition. #950 removed the worse parts, but it’s not finished yet.

Having frozen styles with computed values is the way to go.

@Tontyna
Copy link
Contributor

Tontyna commented Dec 28, 2019

Having frozen styles with computed values is the way to go.

Of course. But grepping for modifications of Box.style it looks like since #950 we don't touch the immutable computed values anymore. The modfications rather ensure the correct initialization of computed values for specialized boxes.

What I don't like is the fact that the pagination stores used values (and other properties related to layout) as attributes in the root_box tree and its children. AFAIK it doesn't hurt, not sure whether it's required, but it definitely feels wrong.

A 'style struct sharing' would be great. At the moment each box of the formatted root_box tree has its own complete style dictionary -- style sharing only happens between the formatted box and its paginated counterpart(s).

@liZe liZe added performance Too slow renderings and removed feature New feature that should be supported labels Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Too slow renderings
Projects
None yet
Development

No branches or pull requests

2 participants