This patch includes quite a few things (apologies for its size but in this case I felt it best to refactor before adding additional functionality). The only consolation is that it should take you less time to apply it than it took me to prepare it! ;-) Firstly it includes a refactoring of FOP configuration so FOP components no longer have a direct dependency on the deprecated avalon configuration framework (renderers no longer implement org.apache.avalon.framework.configuration.Configurable). Every component that requires configuration has an associated configuration class in org.apache.fop.config. Configuration is now achieved by simply calling FopConfig.configure(fopComponent) from the component (factory, renderers etc). Secondly it this patch includes some additional FOP configuration options which allow the user to define a font directory or to automatically detect fonts installed on the native operating system, the results of which can be cached for subsequent FOP executions. This should make font configuration much easier for the standard user. The patch also includes a rewrite of the resolve method in FOURIResolver and a general tightening up of the FOP configuration (including a renaming and an improvement in the unit tests). Here is an example FOP configuration file demostrating the new configuration options :- <!-- by default this option is set to true and is only present here to show its existence in the event that you may wish to turn off font caching --> <cache-fonts>true</cache-fonts> <!-- you can specify where the cache file should be, but by default you shouldn't need to configure or worry about this, it will be set automatically for you--> <cache-file>C:\MyDir\fop.cache</cache-fonts> <renderers> <renderer mime="application/pdf"> <fonts> <font metrics-url="arial.xml" kerning="yes" embed-url="arial.ttf"> <font-triplet name="Arial" style="normal" weight="normal"/> <font-triplet name="ArialMT" style="normal" weight="normal"/> </font> <!-- any fonts found in this directory will be automatically added --> <directory recursive="true">C:\MyFonts</directory> <!-- your native o/s will be searched and any fonts found will be automatically added --> <auto-detect/> </fonts> <renderer> </renderers> This new font configuration/caching functionality is excluded from the specialised AFP font configuration. Native font finding and font triplet info indentification may need to be tweaked but seems to work pretty well from my tests on the standard true type fonts included with windows. Other changes :- org.apache.fop.fonts.Font.BOLD renamed to org.apache.fop.fonts.Font.WEIGHT_BOLD org.apache.fop.fonts.Font.NORMAL renamed to org.apache.fop.fonts.Font.WEIGHT_NORMAL org.apache.fop.fonts.Font.STYLE_NORMAL added org.apache.fop.fonts.Font.STYLE_ITALIC added Please try the patch out, let me know if I have missed or broken anything, feedback is welcome :-).
Created attachment 19702 [details] List of updated/new files
Created attachment 19703 [details] Patch file
Hi Adrian, Wow, looks like a big patch! A few quick comments: - when you submit bugs, please don't assign them to one of the developers, but rather to fop-dev, so that everyone can follow them - given the size of the patch, and if you've not already done so I'll ask you to send a signed ICLA (and a CCLA, if necessary) to the ASF's secretary. See here for details: http://www.apache.org/licenses/#clas I'll try to have a look ASAP, but I'm quite busy currently. If anyone beats me to it, all the better. Thanks! Vincent
Hi Vincent, I CC'd fop-dev in on this patch so everyone should have been able to follow the patch and provide feedback - but I will assign it directly in future. I have sent a signed ICLA to Apache so everything should be fine, but I have not received any confirmation from them of its receipt. Please everyone, try the patch out. Cheers, Adrian. (In reply to comment #3) > Hi Adrian, > > Wow, looks like a big patch! A few quick comments: > - when you submit bugs, please don't assign them to one of the developers, but > rather to fop-dev, so that everyone can follow them > - given the size of the patch, and if you've not already done so I'll ask you to > send a signed ICLA (and a CCLA, if necessary) to the ASF's secretary. See here > for details: > http://www.apache.org/licenses/#clas > > I'll try to have a look ASAP, but I'm quite busy currently. If anyone beats me > to it, all the better. > > Thanks! > Vincent
Adrian, I had a quick look at the code (haven't tried it out /yet/ ;-)) Very impressive piece of work! Auto-discovery of fonts is a very long-awaited feature. Considering the ICLA: if you can, fax it to Apache, just to be certain. I didn't even send mine by snail- mail, since I have lost hope in that some time ago... Hmm, I'm wondering how long it will be before Apache enables potential committers to sign and send an ICLA /digitally/. Anyway, I'm going to try out this patch ASAP. Thanks for your continued efforts on improving the quality of FOP! Andreas
(In reply to comment #5) > Adrian, > > I had a quick look at the code (haven't tried it out /yet/ ;-)) > > Very impressive piece of work! Auto-discovery of fonts is a very long-awaited feature. Yep, that's going to be a good step forward in user-friendliness. I can't comment on the patch, though, as I haven't had time, yet. Thanks for working on this, Adrian. > Considering the ICLA: if you can, fax it to Apache, just to be certain. I didn't even send mine by snail- > mail, since I have lost hope in that some time ago... Hmm, I'm wondering how long it will be before > Apache enables potential committers to sign and send an ICLA /digitally/. Actually, that's possible. You can send a scanned ICLA by mail to secretary@apache.org AND legal-archive@apache.org. Ideally, the mail is PGP signed. Just as an aside: No, Adrian's ICLA hasn't popped up in the secretary's records. I'll notify everyone as soon as that has happened. <snip/>
Adrian, After applying the changes in the patch, it /seems/ to work... only, I can't find a TTF file on my system for which I get no complaints (about ascender/descender conflicts, or absent unicode cmaps) Must be something Mac-related (? or did you test on that platform yet?) As for the fine-tuning: I get a few SEVERE errors when the MacFontInfoFinder wants to access directories that don't exist. Those need to be suppressed. I'm also not too sure I like the idea of automatically /loading/ all the present fonts... Take care of what FontLoader does. It reads the whole TTF-file into a byte array, and some Unicode fonts are rather large beasts to read into memory entirely. If you have a few of those installed...? I'd try to limit it to the fonts that will actually be used, i.e. keep the possibly used font-info cached, but only trigger loading the font during property-resolving when the corresponding font-family is encountered. As you indicated, a bit of tweaking and tuning necessary, but all in all 'Nice Job!'. Cheers, Andreas
Hi Andreas, Thanks for trying out the patch :-). (In reply to comment #7) > Adrian, > > After applying the changes in the patch, it /seems/ to work... only, I can't find a TTF file on my system > for which I get no complaints (about ascender/descender conflicts, or absent unicode cmaps) > Must be something Mac-related (? or did you test on that platform yet?) It is good that you are a Mac user as I was unable to test on the Mac. The Mac native font finder should examine the following folders (not recursively) in order to search for available system fonts :- "~/Library/Fonts/", // user "/Library/Fonts/", // local "/Network/Library/Fonts/", // network "/System/Library/Fonts/", // system "/System Folder/Fonts/" // classic If I could also have some feedback from unix users that would be great. > As for the fine-tuning: > I get a few SEVERE errors when the MacFontInfoFinder wants to access directories that don't exist. > Those need to be suppressed. I have now suppressed this behaviour and will add it to the next patch submission. > I'm also not too sure I like the idea of automatically /loading/ all the present fonts... Take care of what > FontLoader does. It reads the whole TTF-file into a byte array, and some Unicode fonts are rather large > beasts to read into memory entirely. If you have a few of those installed...? > I'd try to limit it to the fonts that will actually be used, i.e. keep the possibly used font-info cached, but > only trigger loading the font during property-resolving when the corresponding font-family is > encountered. The fonts should only be loaded the first time you run FOP in autodetect and cache mode. After this first run, all the font info should be stored in the fop.cache (should be created in fop/conf/fop.cache) and subsequent FOP executions should make use of the cache. Even though this detection is only expensive at the first time of execution, I take your point on board and am considering lightening the font loading for this autodetect purpose - maybe a call along the lines of FontLoader.loadLightFont(fontFile, resolver) - I may add this in the next patch update. > As you indicated, a bit of tweaking and tuning necessary, but all in all 'Nice Job!'. Yes I think there will be some more tweaking, the more people try this patch out the better - I am sure there will be some things I haven't considered. Cheers, Adrian.
Created attachment 19712 [details] List of updated/new files
Created attachment 19713 [details] Patch file Now suppress error messages on failed native font finding as requested by Andreas. Also tidied up the FontLoader so the read() method does the work on calls to load() method and the implementing constructors are nice and dumb as they should be :-).
(In reply to comment #8) > > It is good that you are a Mac user as I was unable to test on the Mac. The Mac > native font finder should examine the following folders (not recursively) in > order to search for available system fonts :- > > "~/Library/Fonts/", // user > "/Library/Fonts/", // local > "/Network/Library/Fonts/", // network > "/System/Library/Fonts/", // system > "/System Folder/Fonts/" // classic Only local and system worked with me. FOP claimed the other three didn't exist, but: -> user: the ~ does not get resolved correctly; "/~/Library/Fonts" indeed does not exist, since I have no directory "/~", that is "cd /~" doesn't work. I think if you could somehow avoid the slash from being prepended, then it would work. -> network: the Library alias exists with me, but the corresponding device [/automount/Library] is not mounted... -> classic: indeed does not exist, but I think we don't even need to consider supporting Classic anymore. The latest available JVM for that platform was 1.2, IIRC, so if we consider 1.4 as minimal target JVM for the next release, FOP wouldn't run on it anyway. > > The fonts should only be loaded the first time you run FOP in autodetect and > cache mode. After this first run, all the font info should be stored in the > fop.cache (should be created in fop/conf/fop.cache) and subsequent FOP > executions should make use of the cache. Good point. I was only thinking about a recent post on fop-users@ where Abel Braaksma pointed out that loading a 23MB font-file into memory just to use one or two characters was slight overkill. I agree with his point, and I would certainly want to avoid blindly loading three or four of these fonts into memory if they aren't used at all. Besides that, it's not even during the property-resolving stage that the font needs to be loaded. All the FO tree needs to know is whether the supplied font exists in the specified style/weight. It's only during layout that more info about the font is needed (metrics, kerning pairs...). If the FO isn't valid, then the font never needs to be loaded. I agree with the point that FOP only needs to determine the first time whether a font /exists/ and store that info in a cache, but if no document ever uses that font, it should never be loaded completely, which is exactly what fop.fonts.truetype.FontFileReader does behind the scenes, when you call loadFont (). Maybe fop.fonts.LazyFont can be used for this. It seems to fit the purpose, no?
Something else just occurred to me (In reply to comment #7) > ... only, I can't find a TTF file on my system for which I get no complaints > (about ascender/descender conflicts, or absent unicode cmaps) > Must be something Mac-related (? or did you test on that platform yet?) I would very much like to know what the results are on an Intel Mac. I'm still on the PowerPC. As I was browsing through the font handling code, there's a lot of byte-reading and bit-shifting done... Well, maybe if I find the time, I'll study the TTF-spec, step through the code and have a look at how the bytes are interpreted over here...
Created attachment 19756 [details] Patch file Following feedback from Andreas - thanks v much for taking the time to look at this :-), I have made a very minor modification to the MacFontDirFinder. It should hopefully now be able to detect fonts that have been installed under user home directory (i.e. ~/Library/Fonts). Please try this patch out on your system Andreas. I would encourage, maybe even urge ;-) someone to commit this patch soon as it is a large patch that touches upon a number of files (see file list) and is therefore likely to become out of date quickly with commits to the repository. Kind regards, Adrian.
Hi Adrian, Had some more time to look into this. I'm currently on a more detailed stroll through your code. Just like to know what I commit, nothing personal. ;-) The change to MacFontFinder is working as it should. In the meantime, if you make any more changes, try to post incremental patches only containing the classes you know for certain were altered since the last diff. I'll do my best to keep up. Here's already a few suggestions for change, but there's no immediate necessity to create a new patch. I've already corrected some things locally, mainly style issues. For the javadocs, we like to have the class-level doc right before the class declaration, after the import header. As you have it, Eclipse collapses them with the license header. :( In WindowsFontFinder, there's that hacky fallback you're commenting on: if shelling out is necessary, then note that 'set windir' will immediately return only the line containing 'windir=', so there's no need to iterate over all environment variables. Still looking further, but these are some things I've already stumbled upon. Stay tuned. Cheers Andreas
Adrian, Just to keep you up to date: I'm almost done here. Apart from the nits mentioned earlier, everything looks A-OK to me. I'll wrap things up later this week, and will then first attach a slightly revised patch here. If it meets with your approval, then it will most likely get committed by the weekend. Thanks again for this fine contribution! Andreas
Hi Andreas, Many thanks for reviewing and taking care of commiting my patch. I realise you guys are very busy and I really appreciate you finding the time to thoroughly review my large patch and provide me with feedback :-). I don't think I will be making any more minor tweaks to this patch at this time as I am busy working on resolving some issues with the postscript renderer. Of course if anything crops up after the commit I will be more than happy to look at it. I very much look forward to seeing the new features I've added being part of the FOP trunk :-). All the best, Adrian Cumiskey. (In reply to comment #15) > Adrian, > > Just to keep you up to date: I'm almost done here. Apart from the nits mentioned earlier, everything looks > A-OK to me. I'll wrap things up later this week, and will then first attach a slightly revised patch here. If it > meets with your approval, then it will most likely get committed by the weekend. > > Thanks again for this fine contribution! > > Andreas
I tried out this patch on Ubuntu Linux and found that I needed a few changes to get it to work properly. Since this hasn't been checked in, I'm not sure what to send a patch against, so I'll just describe them for now. org.apache.fop.fonts.autodetect.UnixFontDirFinder It is probably worth adding the .fonts directory in a user's home directory: e.g. UNIX_FONT_PATHS = { System.getProperty("user.home") + "/.fonts", org.apache.fop.fonts.autodetect.FontFileFinder.find should have the recursive argument set to true when called from org.apache.fop.fonts.autodetect.NativeFontFileFinder.find, because the fonts are found several layers deep under /usr/share/fonts. org.apache.fop.fonts.autodetect.FontInfoFinder If you use embedUrl = fontFile.toURI().toURL().toExternalForm(); then it seems to do escaping of spaces, otherwise font filenames containing spaces cause problems. It is probably worth handling UnsupportedOperationException within the org.apache.fop.fonts.autodetect.FontInfoFinder.find() method to prevent one bad font breaking everything. I find "OpenType fonts with CFF data are not supported, yet" errors thrown by org.apache.fop.fonts.truetype.TTFFontLoader break the caching otherwise. I'm not convinced that the cache is working properly for failed fonts, because I always seem to get log messages about them, not just on the first run. However, I haven't had time to investigate that properly. cheers, Keith
Hi Keith, Many thanks for trying out the patch and taking the time to provide me with feedback. My comments are below. (In reply to comment #17) > I tried out this patch on Ubuntu Linux and found that I needed a few changes to > get it to work properly. Since this hasn't been checked in, I'm not sure what to > send a patch against, so I'll just describe them for now. > > org.apache.fop.fonts.autodetect.UnixFontDirFinder > It is probably worth adding the .fonts directory in a user's home directory: e.g. > > UNIX_FONT_PATHS = { > System.getProperty("user.home") + "/.fonts", Ok this is a nice easy addition. Andreas, if you are reading this I believe you have made some small changes. Before I make any of the changes that Keith suggests, it might be a good idea for you to attach an additional patch file with your changes. > org.apache.fop.fonts.autodetect.FontFileFinder.find should have the recursive > argument set to true when called from > org.apache.fop.fonts.autodetect.NativeFontFileFinder.find, because the fonts are > found several layers deep under /usr/share/fonts. I am thinking that I may make the NativeFontFileFinder class a little more flexible so that implementing classes are able to specify whether each individual search file path should be searched recursively or not. > > org.apache.fop.fonts.autodetect.FontInfoFinder > If you use > embedUrl = fontFile.toURI().toURL().toExternalForm(); > then it seems to do escaping of spaces, otherwise font filenames containing > spaces cause problems. This is also a quick easy change. > It is probably worth handling UnsupportedOperationException within the > org.apache.fop.fonts.autodetect.FontInfoFinder.find() method to prevent one bad > font breaking everything. I find "OpenType fonts with CFF data are not > supported, yet" errors thrown by org.apache.fop.fonts.truetype.TTFFontLoader > break the caching otherwise. Yes this is a case I did not cater for. I will change this. > I'm not convinced that the cache is working properly for failed fonts, because I > always seem to get log messages about them, not just on the first run. However, > I haven't had time to investigate that properly. I believe this to be working fine. Bad/failed fonts are also recorded in the cache. The error is still printed to remind the user of the problem but the font is not parsed again unless the file has been changed. Thanks again Keith for taking time to try out the patch and provide feedback :-). Cheers, Adrian Cumiskey.
Created attachment 19890 [details] updated files from org.apache.fop.fonts.autodetect package Following feedback from Andreas and Keith I have updated the autodetect package. I hope this does not cause you too many merge problems Andreas.
Created attachment 19891 [details] updated files from org.apache.fop.fonts.autodetect package Whoops.. This is what I should have submitted. Please ignore the last attached archive file. Andreas, this patch is getting a little out of date with the trunk now - if you need me to create a fresh patch from the latest trunk code let me know.
Hi Adrian, Thanks for making those changes, they seem to work for me. (In reply to comment #18) > I believe this to be working fine. Bad/failed fonts are also recorded in the > cache. The error is still printed to remind the user of the problem but the > font is not parsed again unless the file has been changed. Sorry, yes, the failed fonts are recorded fine. However, I wonder whether it is worth changing this log from info to debug after the first run (FontInfoFinder line 151). If you have a lot of fonts, it can produce several pages of messages (mainly from /usr/share/fonts/type1/gsfonts on my system). thanks for a very useful patch. Keith Stribley
Hi Adrian, Just a little FYI: I did not get around to applying the patch during the weekend (because of a small disagreement with my ISP over the price of my connection... I thought it should be free ;-) As a result they cut me off... Checking my e-mail via Webinterface now) Anyway, this will all be settled later this week, so it will take a bit more time than I had anticipated, but just wanted to let you know that I haven't forgotten about this one. Locally, I've done about all I can. Just need to get to uploading. Cheers, Andreas
(In reply to comment #21) Hi Keith / Adrian, > ... I wonder whether it is worth changing this log from info to debug after the first run > (FontInfoFinder line 151). If you have a lot of fonts, it can produce several pages of > messages (mainly from /usr/share/fonts/type1/gsfonts on my system). Indeed, I tend to agree with Keith here. Adrian, what do you think? Of course, the issue is rendered moot once we're talking about servlet environments. Still, we do have CLI users to take into account... Cheers, Andreas
(In reply to comment #20) Hi Adrian, > > Andreas, this patch is getting a little out of date with the trunk now - if you > need me to create a fresh patch from the latest trunk code let me know. Just a little nit: the last attachment does not include an updated NativeFontDirFinder, while at least the addition of a 'fontPaths' member seems to have changed...? Is it possible this was changed, or did I miss something? Cheers, Andreas
Hi Andreas, I've just checked the zip file and it does contain a NativeFontDirFinder.java file. What I submitted seemed to work ok for Keith.. maybe the problem lies in the merge after your ammendments? I think I renamed and changed the type of fontPaths to fontDirs (it now uses an entity inner class called FontDirInfo defined in NativeFontDirFinder to hold font paths). Hope you get time to commit the patch, wishing you a nice easter holiday period :-) Adrian. (In reply to comment #24) > (In reply to comment #20) > > Hi Adrian, > > > > > Andreas, this patch is getting a little out of date with the trunk now - if you > > need me to create a fresh patch from the latest trunk code let me know. > > Just a little nit: the last attachment does not include an updated NativeFontDirFinder, while at least the > addition of a 'fontPaths' member seems to have changed...? Is it possible this was changed, or did I > miss something? > > Cheers, > > Andreas
*** Bug 41514 has been marked as a duplicate of this bug. ***
Andreas, are you still working on this patch?
Andreas, if you come back online again please do not commit the patch just yet as I have made some significant improvements to it which will be attached soon.
Created attachment 20242 [details] Patch file
Created attachment 20243 [details] List of updated/new files
This patch contains many design and style changes and improvements including (but not limited to) :- * Small URI related JDK1.4 dependencies removed from FOURIResolver. * DirectoryWalker from commons-io-1.3.1.jar is now used for font detection. * <cache-fonts/> has been renamed to <use-cache/> so it may cover other general caching uses in the future. Its a large patch so I may have missed something, so please try the patch out. Feedback welcome :-) Adrian.
I will take this on, since Andreas has mysteriously vanished. Thanks, Adrian, for revising the patch. The new one is an significant improvement over the previous. I'm in the process of reviewing the patch in detail. I will be able to commit it on Friday.
Wow, it took me a lot longer to thoroughly review this patch than I anticipated. I'm almost ready to commit. I only have to test the PDFTrancoder and PDFDocumentGraphics2D. Here are the issues I found and what I did about them: - Javadocs: Still some issues there. I fixed the most important locations. - FopCache: I did a double-check on that to see if I've done anything wrong, but after applying the patch the cache file was always empty. The writing methods were simply not referenced anywhere in the code. Therefore the font lookup performance wasn't improved. I also wondered why it has to be done in such a complicated way (writing and parsing XML using custom code) just for a cache file. So since the code didn't work as expected I simply tore it out (Sorry, Adrian) and used Java's Serializable mechanism. That has the elegant side-effect that there's a lot less code to maintain and it's probably faster. And I renamed FopCache to FontCache and moved it into the fonts package. I didn't like all this cache stuff cluttering the apps package. Now on my machine the speed-up is from 1200ms for the first run, to 70ms with the cache despite over 150 fonts being registered. Another issue in that area was the use of the singleton pattern for the FontCache. I have now attached the FontCache to the FopFactory like every other cache we have. Furthermore, the default location of the cache file is changed to <user-home>/.fop/fop-fonts.cache. I did this because the font auto-detection also checks user directories so this made sense to me. I have not yet added a configuration item to define the cache location and I removed the command-line argument for the cache location as it had no counterpart for embedded use. We can add that later when we see a need for it. - TrueType Collections (*.ttc)and OpenType fonts with an ".otf" extension are currently not found. I'll add that after applying the patch. - TrueType Collections are not properly supported with the auto-detection mechanism. ATM, if someone wants to use a TTC, he has to configure it using a XML font metric file as before. We can improve that later. - The same applies to fonts that should only be referenced but not embedded. For certain environments you have all the fonts already installed on the production printers and you may want to reference those. But I guess that's becoming less important nowadays. - The whole font configuration stuff got lost for PDFDocumentGraphics2D. I've addressed that and took the opportunity to remove some of the Avalon dependencies, too. This as a preparation for the move to Commons/Batik. - I addressed a small problem when the font base URL is no file URL. The font auto-detection will then be skipped for the font base URL. - The RendererContextInfo stuff was quite confusing to me (especially the somewhat abstract naming). I eventually managed to simplify it which resulted in removing RendererContextInfo* and everything that's necessary is now in XMLHanderConfigurator because RendererContextInfoConfigurator really configured the XMLHandler, not the RendererContext. - Another case where I thought I was doing something wrong: Type 1 fonts simply didn't work. All fonts failed because the auto-detection code was accessing the PFB file instead of the PFM. I fixed that. - I made the font code a little less verbose on the log as with auto-detection I'm not really interested in all the details. That's another case where dividing developer feedback from user feedback will be helpful. While working on this patch, I kept thinking that there's still some room (and necessity) for improvement with the whole font handling (not directly related to this patch). The big issue: The whole font configuration is currently triggered during the renderer setup. That means it is done each time a new renderer is created. Plus we still have a font config per renderer which is not beautiful. I keep ending up with my "font source" idea I had back when I discussed the whole font thing with Victor. I guess I will tackle that one day now that we've decided not to work with FOrayFont... Plus another few notes for the future: - A facility for font subsitution is getting more important to have with auto-detection since you don't specify the font names yourself anymore. Suppose you have an FO file that uses "StandardGrotesk" but the actual font is registered by auto-detection as "StandardGroteskBSK-Regular". But part of the problem is our lack of parser for AFM files which contains font family information in contrast to the PFM file. FOrayFont has an advantage here. - Suppose you have a font set with Light, Regular, Medium, Bold, ExtraBold and Super and they should actually be accessible under the same font family but with the integer weights (...400, 500, 600...) from the FO spec. Similar to the point above you will want to be able to specify a mapping which is probably very hard to do automagically. Anyway, thanks a lot, Adrian, for your work and your patience. There's no need to improve anything on the patch for now. I've handled all issues that were important to me. We can refine later. Commit coming up in the next hours.
Patch applied: http://svn.apache.org/viewvc?view=rev&rev=542237
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed