Fop
  1. Fop
  2. FOP-1326

[PATCH] Refactored configuration, font detection and caching, url resolution

    Details

    • Type: Bug Bug
    • Status: Closed
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: unqualified
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other
    • External issue ID:
      41831

      Description

      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 .

      1. out.txt
        6 kB
        Adrian Cumiskey
      2. out.txt
        6 kB
        Adrian Cumiskey
      3. filelist.txt
        6 kB
        Adrian Cumiskey
      4. bug_41831.patch
        309 kB
        Adrian Cumiskey
      5. bug_41831.patch
        315 kB
        Adrian Cumiskey
      6. bug_41831.patch
        315 kB
        Adrian Cumiskey
      7. bug_41831.patch
        370 kB
        Adrian Cumiskey
      8. autodetect.zip
        10 kB
        Adrian Cumiskey
      9. autodetect.zip
        10 kB
        Adrian Cumiskey

        Issue Links

          Activity

          Hide
          Adrian Cumiskey added a comment -

          Attachment out.txt has been added with description: List of updated/new files

          Show
          Adrian Cumiskey added a comment - Attachment out.txt has been added with description: List of updated/new files
          Hide
          Adrian Cumiskey added a comment -

          Attachment bug_41831.patch has been added with description: Patch file

          Show
          Adrian Cumiskey added a comment - Attachment bug_41831.patch has been added with description: Patch file
          Hide
          Vincent Hennebert added a comment -

          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

          Show
          Vincent Hennebert added a comment - 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
          Hide
          Adrian Cumiskey added a comment -

          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

          Show
          Adrian Cumiskey added a comment - 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
          Hide
          Andreas L. Delmelle added a comment -

          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

          Show
          Andreas L. Delmelle added a comment - 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
          Hide
          Jeremias Maerki added a comment -

          (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/>

          Show
          Jeremias Maerki added a comment - (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/>
          Hide
          Andreas L. Delmelle added a comment -

          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

          Show
          Andreas L. Delmelle added a comment - 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
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Adrian Cumiskey added a comment -

          Attachment out.txt has been added with description: List of updated/new files

          Show
          Adrian Cumiskey added a comment - Attachment out.txt has been added with description: List of updated/new files
          Hide
          Adrian Cumiskey added a comment -

          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 .

          Show
          Adrian Cumiskey added a comment - 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 .
          Hide
          Adrian Cumiskey added a comment -

          Attachment bug_41831.patch has been added with description: Patch file

          Show
          Adrian Cumiskey added a comment - Attachment bug_41831.patch has been added with description: Patch file
          Hide
          Andreas L. Delmelle added a comment -

          (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?

          Show
          Andreas L. Delmelle added a comment - (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?
          Hide
          Andreas L. Delmelle added a comment -

          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...

          Show
          Andreas L. Delmelle added a comment - 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...
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Adrian Cumiskey added a comment -

          Attachment bug_41831.patch has been added with description: Patch file

          Show
          Adrian Cumiskey added a comment - Attachment bug_41831.patch has been added with description: Patch file
          Hide
          Andreas L. Delmelle added a comment -

          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

          Show
          Andreas L. Delmelle added a comment - 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
          Hide
          Andreas L. Delmelle added a comment -

          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

          Show
          Andreas L. Delmelle added a comment - 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
          Hide
          Adrian Cumiskey added a comment -

          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

          Show
          Adrian Cumiskey added a comment - 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
          Hide
          Keith Stribley added a comment -

          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

          Show
          Keith Stribley added a comment - 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
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Adrian Cumiskey added a comment -

          Following feedback from Andreas and Keith I have updated the autodetect
          package. I hope this does not cause you too many merge problems Andreas.

          Show
          Adrian Cumiskey added a comment - Following feedback from Andreas and Keith I have updated the autodetect package. I hope this does not cause you too many merge problems Andreas.
          Hide
          Adrian Cumiskey added a comment -

          Attachment autodetect.zip has been added with description: updated files from org.apache.fop.fonts.autodetect package

          Show
          Adrian Cumiskey added a comment - Attachment autodetect.zip has been added with description: updated files from org.apache.fop.fonts.autodetect package
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Adrian Cumiskey added a comment -

          Attachment autodetect.zip has been added with description: updated files from org.apache.fop.fonts.autodetect package

          Show
          Adrian Cumiskey added a comment - Attachment autodetect.zip has been added with description: updated files from org.apache.fop.fonts.autodetect package
          Hide
          Keith Stribley added a comment -

          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

          Show
          Keith Stribley added a comment - 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
          Hide
          Andreas L. Delmelle added a comment -

          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

          Show
          Andreas L. Delmelle added a comment - 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
          Hide
          Andreas L. Delmelle added a comment -

          (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

          Show
          Andreas L. Delmelle added a comment - (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
          Hide
          Andreas L. Delmelle added a comment -

          (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

          Show
          Andreas L. Delmelle added a comment - (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
          Hide
          Adrian Cumiskey added a comment -

          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

          Show
          Adrian Cumiskey added a comment - 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
          Hide
          Adrian Cumiskey added a comment -
              • FOP-1304 has been marked as a duplicate of this bug. ***
          Show
          Adrian Cumiskey added a comment - FOP-1304 has been marked as a duplicate of this bug. ***
          Hide
          Jeremias Maerki added a comment -

          Andreas, are you still working on this patch?

          Show
          Jeremias Maerki added a comment - Andreas, are you still working on this patch?
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Adrian Cumiskey added a comment -

          Attachment bug_41831.patch has been added with description: Patch file

          Show
          Adrian Cumiskey added a comment - Attachment bug_41831.patch has been added with description: Patch file
          Hide
          Adrian Cumiskey added a comment -

          Attachment filelist.txt has been added with description: List of updated/new files

          Show
          Adrian Cumiskey added a comment - Attachment filelist.txt has been added with description: List of updated/new files
          Hide
          Adrian Cumiskey added a comment -

          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.

          Show
          Adrian Cumiskey added a comment - 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.
          Hide
          Jeremias Maerki added a comment -

          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.

          Show
          Jeremias Maerki added a comment - 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.
          Hide
          Jeremias Maerki added a comment -

          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.

          Show
          Jeremias Maerki added a comment - 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.
          Hide
          Jeremias Maerki added a comment -
          Show
          Jeremias Maerki added a comment - Patch applied: http://svn.apache.org/viewvc?view=rev&rev=542237
          Hide
          Glenn Adams added a comment -

          batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

          Show
          Glenn Adams added a comment - batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

            People

            • Assignee:
              fop-dev
              Reporter:
              Adrian Cumiskey
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development