change font id generation

[bvogel]

this PR is related to an issue I'm facing with a PR from prawn-templates (see prawnpdf/prawn-templates#3)

[gettalong]

This could also lead to problems. The best way would be to generate the key, check if the font_registry has such a key and if it has, modify the key, then check again...

[bvogel]

BTW: the failing spec failed even before this PR...

[bvogel]

I'm now checking against the @document.state.page.fonts hash. font-repository didn't look right.

[packetmonkey]

We want to avoid inserting random data into PDFs if at all possible. It means if we run the same PDF generation script twice we will get different files out the other side which leads to other issues with packaging files generated by Prawn.

Could we tweak the key generation to check for existence, and if so increment our counter?

I'm thinking something like this

def generate_unique_id
  init = @document.font_registry.size + 1
  key = :"F#{init}"
  while @document.state.page.fonts.keys.include?(key) do
    init += 1
    key = :"F#{init}"
  end
  key
end

This way it's deterministic. Does that solve the problem?

Also can we add a spec that would have previously added a duplicate font id so we can ensure that the no-duplicate font key code keeps working?

Thanks!

[bvogel]

Thanks Evan, I'll look into this tomorrow.

[bvogel]

@practicingruby, I had to change your suggestion as the key will contain a subset part (see https://github.com/prawnpdf/prawn/blob/master/lib/prawn/font.rb#L370). I think current version should run just fine (specs are passing)

I'm a bit at a loss as I don't know how a existing font should exist within the realm of prawn where no templates exist. Without this code specs of prawn-templates will fail, but I can't devise a form how test this in prawn only.

[crazymykl]

Enumerable#any? might be good here.

[packetmonkey]

I'm not sure I understand why the inclusion in the subset causes a problem with my suggested implementation. I may not be paying close enough attention.

What does this proposed diff have that my proposed diff doesn't?

[bvogel]

@packetmonkey - if I read the code correctly fonts are added by add_to_current_page(L365) to the page's font hash. The key for this hash is created by identifier_for(L370) which takes the identifier generated by generate_unique_id and adds .#{subset}. IMO the first font on the page will result in a key F1.0, and include? does a full string comparison so "F1" == "F1.0"will always return false.
Am I making any sense?

[packetmonkey]

I keep going back and fourth on this issue. You totally have a legitimate problem that needs to be resolved in order to provide functionality I want to let you provide, however this is adding complexity and functionality that prawn natively doesn't need to do it's job on it's own.

I think the correct answer here may be dependency injection.

If Prawn provides a way (as an experimental non-semver binding API while we work though this) to provide your own font ID generator, we can keep the simple use case as our default, but allow you in prawn-templates to set your own more complex generator, do you think that could help?

It also means as your use case evolves or you find other bugs, you can fix it yourself without worrying about keeping Prawn up to date.

I'm not sure what this would look like yet but I'm throwing the idea out there. I suspect this won't be the only time we need to do this kind of integration, and moving in that direction could allow us to provide similar hooks for other deeply integrated plugins people way want to add to prawn.

Thoughts?

[bvogel]

Evan, I completely agree with you on this. My first try to fix this issue would have been to add the fonts from the template to the pages font array so that the current id generation would continue to work as expected. Your approach would make this a lot easier. A related issue with the page size/orientation would be easier to treat if some method of injection would be in place.

[bvogel]

Hi Evan @packetmonkey, I read in another PR that you will cut another release within a couple of days - how do we go about this issue? Implementing some kind of dependency injection seems a larger task, on the other hand I want to get a new version of prawn-templates out of the door to get that (apparently still frequently used feature) up to par with the current prawn release. How about merging this in and creating a separate issue to implement DI?

[practicingruby]

I had a conversation w. @packetmonkey about this and was thinking that although Prawn never needs to think about IDs potentially being non-unique, a future version of PDF::Core may need to. And I was unsure the DI route was needed as it seems like it wouldn't provide an extension point as much as fix a limitation in Prawn.

So my recommendation (DI or not) was to move this down to the PDF::Core level and get it implemented there. It's a private method on Prawn currently, so that means we can change it whenever we want as long as no client code needs to be changed and no visible changes happen in the PDFs (binary compatibility has never been guaranteed).

So @packetmonkey... would you be open to a patch from @bvogel that extracts this into PDF::Core and implements it as-is for now, leaving the DI conversation for later?

(Just throwing this in in hopes that it may help move things along, I defer to @packetmonkey on the decision making!)

[packetmonkey]

In thinking about the discussion @practicingruby referenced, I too now think this may be better solved in PDF::Core, for the mentioned reasons.

Part of my thoughts for why this makes sense is like above, you can't really write a test in prawn for the new behavior, but you could totally write the test for PDF::Core for the new behavior, which suggests to me that's the correct spot for this.

In general I'm also in favor of more substantial plugins like prawn-templates using more of the lower-level PDF::Core methods than trying to thread everything though Prawn which is supposed to be a higher level abstraction over PDF::Core, so this helps keep Prawn in the realm of 'easy to use PDF Generation' and out of the "Solve everybody's PDF implementation needs" realm.

So this plan is currently the top "Feels good" option to me at least.

What do you think @bvogel ? I also agree lets get this wrapped up before I cut the next release so I don't leave you blocked for a whole new release cycle.

[bvogel]

I'm OK with fixing this in PDF:Core. If you move this into Core, I'll prepare a PR against that our you change Core based on this PR, all is fine by me. Even if you are swamped, just point me to where you would see it in Core and I implement it there.

[packetmonkey]

I am fairly swamped right now so the best solution probably would be you to take a swing at this.

A good first step may be to define a PDF::Core::Fonts module and include it in core.rb

For now PDF::Core is just an internal dependency of Prawn, and no API is guaranteed between releases so we can be a little more flexible getting something going and changing it in the future if we figure out a better way to go about things.

[bvogel]

Ok, just to be clear: I'd start by moving prawn/font.rb and prawn/font/ to pdf/core/font.rb and pdf/core/font/ (and related specs) and then fix the references in prawn?

[packetmonkey]

I'm going to open with that I'm not 100% comfortable with the font code here, so some of these ideas may turn out to be horrible :)

While the majority of the font code probably should move down to PDF::Core, for your specific need do you think we could move down just the font registry?

If we created a PDF::Core::FontRegistry class, and added a #generate_id method to it, then updated #find_font to use a new API instead of a raw internal hash, we can make a small refactoring in the currently thought to be best direction, get your feature added down there in an easy to write unit test, and give you a clean API in prawn-templates to add a font to the registry.

It may also make sense to move the font_families hash down as well and proxy calls to it. That will be an unfortunate side effect of the official prawn API giving developers access to the internal font_families hash instead of wrapping it in an API, so we will need to preserve that for compatibility for now (maybe another breaking change for Prawn 3).

Then with at least top-level font management happening in PDF::Core, with a real API to add fonts, you can call that from prawn-templates to add the fonts you need, in whatever order/way you need, and we can unit test correct generation of font IDs. And for prawn it's just an implementation detail of PDF::Core so we aren't introducing new complexity to Prawn for a use case it doesn't need to itself support.

Thoughts? I tagged this PR as a blocker, I won't cut a new Prawn + PDF::Core release until this is sorted out.

[bvogel]

Hi Evan, I have been out of town so I only could take a look at the code again today: the font-registry isn't affected by this issue. The identifier thats generated is completely font related and has no direct impact on the document. The identifier is only used when adding the font to the directory of the document. As far as I understand it there is no direct reference to this directory key when using the font within the document. So my take is that what I mentioned in the last question (move all of font?).

[packetmonkey]

If we are going to refactor the font code down to PDF::Core, maybe the fastest way to do this is to accept the complexity into Prawn, knowing it will move to PDF::Core where it will be appropriate, and it just becomes a refactoring we do and when we do that refactor we ensure we preserve the functionality.

That way we can get this merged, get you working, get prawn-templates back functional, and cut this release and get it out the door.

What does everyone thing of that?

[practicingruby]

In my mind this is internal behavior that shouldn't negatively impact users, so 👍

[bvogel]

Hi guys, I'll happily accept. I'll try to shell out some time this month and have a go at refactoring. But for the time being priority would be templates. Good news is that some users have been trying the code and it works for them 😄

[bvogel]

Hi guys, anything I need to do? Or is this now in hands of Evan?

[packetmonkey]

Nope, I had to take a trip to Texas last week on short notice so I was tied up.

All you have to do is celebrate :)

Now that this is merged I'll look at another open branch or two to see about getting it merged this week then try to cut a new release this weekend so you can get a new prawn-templates out the door.

[bvogel]

🎉 🎆 🎈

[bvogel]

@packetmonkey Hi Evan, I figure you are swamped at the moment, so please don't take this the wrong way just a friendly reminder to probably push a release before the end of the year?

[packetmonkey]

Yeah, I hope to release prawn one of these weekends, I have said that to myself the last few weekends. The problem is how complex the release process is for Prawn with some measures of manual testing involved.

At the very least I'll get the process documented and I'll see what I can do to get this going. I would like to make it a more regular process and part of that involves doing it a couple of times then automating what we can.

Sorry about the wait

[practicingruby]

@bvogel Just a heads up... we're planning to cut a release this weekend.

[practicingruby]

@bvogel: I deleted your comment by accident...

Here's what it said, for others:

@practicingruby, @packetmonkey I'm still amazed at the complexity of prawn - I never figured releasing a new version would take 3 months :) (I'm joking). But in all seriousness we should REALLY cut a release. Is there anything I can do to help in the progress?

Release schedule is entirely up to @packetmonkey as lead maintainer on this project, and he's busy.

There's not currently anything anyone else can do to help except be patient. Over the long term, perhaps bringing more maintainers in, as well as simplifying our release process, would be a good thing.