https://issues.apache.org/bugzilla/show_bug.cgi?id=48567
--- Comment #4 from Peter Hancock <peter.hanc...@gmail.com> 2010-01-20 08:55:17 UTC --- (In reply to comment #3) > Peter, I've taken a peek at your code. Thanks for reviewing the patch so quicky! > Eclipse had a bit of trouble applying > the patch because you renamed AFPFontReader to CharacterSetBuilder, but that > was not insurmountable. Sorry about that! > You've asked me off-list if it is correct to get the em space width from the > FNO field rather than the FNC field. But I don't see any information on the em > space in the FNC field. At any rate, FNO is the right place to get this from > IMO. > Furthermore, it is a bit strange that you call this "unitPerEm" in > CharacterSetOrientation what is actually the em space increment. Maybe that's > what you confused in FNC with. There's a unit per em there but that has a > different purpose. I was unsure if there was a distinction between 'em space increment' and 'units per em' - now I know. > While going through your changes I wondered if it would not have made sense to > extract the em space in every case, not just in the double-byte case. That > could have reduced the code duplication a bit that is caused by > org.apache.fop.afp.fonts.CharacterSetBuilder.DoubleByteLoader. At least the > processFontOrientation() method looks like unnecessary code duplication to me. That makes sense - we can remove the now redundant CharacterSetOrientation(int) constructor as well. > I'm wondering if you would agree that using "Type 0" (or "CIDKeyed") instead > of > "double-byte" is a more accurate choice for the font type in the user > configuration. CIDKeyed then? > > Do you think that the fallback character business in the configuration is > really necessary? I fear that this is too complicated for users. I don't quite > see what it would help. Can you explain why you added this? using a fallback character was my first approach and the em space, as recommended by yourself, my second. In retrospect I now totally agree that the fallback character is probably useless and certainly reduces usability. > - The restriction of the font and codepage names to exactly 8 characters is > something we should relax eventually (by creating actual > FullyQualifiedNameTriplet instances in MapCodedFont). Probably a remainder > from > the times where the triplets were not encapsulated in classes. At least there > is no such restriction in the AFP specs. I am looking at this now. I thought it may have been an AFP restriction since I seemed to get a badly formed AFP when I forced through a name of length 6. This will have to be resolved at a later time I guess. > If you don't have anything else that you need to address with this patch, I > could commit it later this week. I'd like your feedback on my comments above > before I do that. I could handle the few changes myself. Since I've already > made some local, cosmetic changes locally, that would save my applying the > patch a second time. I agree with all of your suggestions. Before you go ahead and commit I wonder how easy it is to configure fop to not embed the font. Is much dev work required do you think? In http://xmlgraphics.apache.org/fop/trunk/fonts.html it is claimed that not supplying an embed-url attribute directs fop not to embed the font, but this is not so, and in fact, fop never seems to have a chance at setting the embeddable flag on an AFP font. When AFPRendererConfigurator constructs the font instance it could set the flag then based on an attribute, but what other information is needed? When I manually set embeddable to false the rendered AFP only declares the font resource and the code page in the MCF structured field. I have not tried printing yet (Our afp printer is playing up for me today) or checked the spec to see if this is ok. If you happen to know that it should work we could perhaps add a boolean-like attribute to the font attribute (respected when type="CIDKeyed" or all AFP?) and call AFPFont.setEmbeddable() upon creation (or pass a constructor arg). Otherwise I guess this would be a future unit work. >However, if you could update the documentation for the > website once we've finalized the above, that'd be appreciated. I will attach this as a second patch to this bug. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug.
