Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add ability to roll back a transaction that started a new page #268

Closed
bradediger opened this Issue · 19 comments

6 participants

@bradediger
Collaborator

See this mailing list thread:

https://groups.google.com/group/prawn-ruby/browse_thread/thread/e7a57074b4c6fe28

For snapshots to be fully generic, we would need to store a lot more information in the snapshot than we currently do -- naïvely, we could deep-copy the entire page array. We could potentially develop a "copy-on-write" scheme that only adds data to the snapshot when a new page is created. When pages are dirtied w.r.t. the current snapshot, save the old state into the snapshot. We'd have to get a bit clever to handle multiple nested transactions, but it can be done.

Alternatively, we could prohibit start_new_page, go_to_page, etc. inside transactions, but that seems unfriendly.

@bradediger bradediger was assigned
@irongaze

+1 on this issue - without it, using #group is not feasible for user-generated content.

@skandragon

+1 -- having problems using group as well.

@skandragon

I know this is a very old ticket, but it is a showstopper for me currently. Can someone provide a few hints about what might be needed to fix this, so that I can use group to ensure a block of user-submitted text will fit on a page, using #group?

@nathansamson

Is this bug the cause for #501 ?

@practicingruby

@nathansamson: I'll take a closer look at this today and let you know.

@practicingruby

@nathansamson Based on the mailing list conversation (particularly the discussion of repeaters towards the end), I think the bug you saw is based on same issue as what is described here.

I don't think I'm going to be able to personally investigate a fix for this very soon, but please do look into it yourself if you're interested in helping out!

@practicingruby

@bradediger: How terrible of a solution would it be to deep copy the entire page array? Is performance the main implication?

@bradediger
Collaborator

Performance was the reason it hasn't been done that way yet, but I can't prove that it's a terrible idea. From a quick look at the code, we're using transactions a lot less in Prawn's internals than I thought we were. It might be worth some benchmarking to see what breaks with this approach (intellectually impure as it may be).

@practicingruby

Anyone want to volunteer to look into a deep-copy based solution to this issue?

/cc @irongaze, @skandragon, @nathansamson

@nathansamson

I can confirm that taking a deep_copy of @internal_state fixes the problem.

Note that the changes to the code results in 6 other tests failing. (:()
Problem currently lies in taking the deep_copy. I do this naively with the ruby_deep_copy gem, and I believe it is not able to correcly clone everything (especially Fonts & some other object stuff).

I guess we can take a more conservative approach and not taking a deep_copy of everything but just the fields that are deep_copyable.. (especially not streams, Files, procs & alike).

@practicingruby

@nathansamson Sounds like an approach worth investigating. Let me know if you can get a green build by doing a selective deep copying, and if so, open a pull request!

@practicingruby

It would appear that this issue causes all sorts of different problems, so it's probably one of the highest priority bugs to fix. Anyone have time to investigate further?

@nathansamson

I looked at it again, but I realy don't have the knowledge of the PDF spec (not sure if it is needed), or the Prawn codebase to do this

I still feel the clue lies in correctly deep copying (in a sane way) of the internal_state object. But since it is using things like references and stuff, I am actually not sure what the expectation of the API is, and how safe it is to do..

If someone else wants to have a look at defining the "clone" feature of GraphicsState, or guiding me that would be very helpful.

@practicingruby

Thanks for taking a closer look. I'll take a closer look at problem when I return from winter break in a few days. Anyone else who wants to give it a shot between now and then is welcome to do so!

@practicingruby

@cheba: You still thinking about investigating this one? If not I'll try to fit it into my backlog, which already has a few things on it. :frowning:

@cheba
Owner

I'm not working on this one. Not in near future.

@practicingruby

OK. I'll leave it on my own TODO pile for now then, but anyone is welcome to take a shot at it!

@practicingruby

This is still one of the biggest outstanding bugs we have, and I'd love to see a fix before 1.0. I will comment here if and when I start working on it, but anyone else want to look into it sooner?

@practicingruby practicingruby referenced this issue from a commit
@practicingruby practicingruby Add several test cases to pin down #268
This includes examples based on bug reports in #297, #606, #611.
There are still other issues in the tracker related to transactions
but if we can get ALL of these to pass, the others will probably
come with it, as long as our solution is a general fix.
7273144
@practicingruby

Closing as no longer relevant, since we've dropped transaction support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.