opentype.js icon indicating copy to clipboard operation
opentype.js copied to clipboard

implement graceful loading via logging severity levels

Open Connum opened this issue 1 year ago • 9 comments

Description

This is a work in progress, but I wanted to get feedback before adding tests, changing all the instances of console.* and throw and adding to the documentation.

This implements a logger that allows to log messages with different severities (ERROR, WARNING, DEPRECATED). By default, everything gets logged to the console, and only messages of type ERROR get thrown. This can be set via setLogLevel() and setThrowLevel(), using binary flags, e.g. opentype.ErrorTypes.ALL ^ opentype.ErrorTypes.DEPRECATED to log everything but DEPRECATED messages.

When adding a message to the log, this will also trigger a custom opentypejs:message event, so you can catch all the different message types, e.g. for displaying them on our demo pages. Speaking of which, those have also been adapted to handle missing values more gracefully and log multiple messages instead of just the last error thrown.

During initial parsing, many formerly thrown error messages have been changed to messages of type WARNING. This allows incomplete and corrupted fonts to load. To test one of those, I also implemented parsing of standalone CFF fonts, because it was fairly simple. Those are basically standalone CFF tables, so we had everything ready for parsing and just needed to handle some font information that we get from other tables in OpenType CFF files. Might need to extend that a bit further in the future, but it's OK for getting basic glyph and meta data.

Last but not least, the same logging functionality as above is used for Font.validate(). It uses a separate logger instance so the log and throw levels can be set differently from the global logger, and in any case the collected messages will be returned as an array so they can be handled separately.

Motivation and Context

fix #643, fix #458, fix #426, fix #370, fix #337, fix #279 (all related to subset/incomplete font data) fix #607 (Font.validate())

How Has This Been Tested?

Tested all the subset files provided (unfortunately can't simply use those for testing due to unclear licensing).

TODO

  • add some general tests regarding error logging/handling
  • find or produce some usable test files if I can
  • add tests for Font.validate()

Screenshots (if appropriate):

image font.validate() finally being useful

image Showing multiple warning messages in the demo

image image parsing subset fonts

image parsing standalone CFF (subset) fonts

Types of changes

  • [X] Bug fix (non-breaking change which fixes an issue)
  • [X] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [X] I did npm run test and all tests passed green (including code styling checks).
  • [ ] [TODO] I have added tests to cover my changes.
  • [X] My change requires a change to the documentation.
  • [ ] [TODO] I have updated the README accordingly.
  • [X] I have read the CONTRIBUTING document.

Connum avatar Nov 24 '23 20:11 Connum

This is a good idea. We need a more robust logging system and one that's more compatible with other logging systems.

ILOVEPIE avatar Nov 24 '23 20:11 ILOVEPIE

We should make it so that event.preventDefault makes it so that the message does not get printed if it would have been. The reason for that is so that it can be used to integrate with other logging systems.

ILOVEPIE avatar Nov 24 '23 20:11 ILOVEPIE

Can you be more specific about "other logging systems"? Anything particular I should take look at as a reference?

Connum avatar Nov 24 '23 20:11 Connum

there are JavaScript logging and log capturing libraries that are used to manage logs or aggregate them from clients to a central logging server. By providing an event like this, we could then have the warning from our system be suppressed and passed on to the other logging library.

ILOVEPIE avatar Nov 24 '23 20:11 ILOVEPIE

Just pushed a change to make the event cancelable via preventDefault().

Connum avatar Nov 24 '23 20:11 Connum

This feel over engineered. As I understand this try to fix 2 issues:

  • the ability to customize logging
  • the ability to parse more resiliently

So why not allow to set at Font.parse() time:

  • a logger object (that default to the platform agnostic console)
  • a resilientParse boolean ?

yne avatar Nov 24 '23 23:11 yne

@yne we don't have access to the current font object in all the places where we might throw or log an error, in order to access an option like resilientParse. And we would have to check that option every time we want to output an error, so we would need a wrapper for the default console anyway.

Being able to differentiate between recoverable and unrecoverable errors is an integral part of parsing incomplete or corrupt data, so the two issues are intertwined.

Connum avatar Nov 25 '23 00:11 Connum

Is this ready for review? If so, we should mark the PR as no longer being a draft in the description.

ILOVEPIE avatar Jun 12 '24 17:06 ILOVEPIE

Is this ready for review? If so, we should mark the PR as no longer being a draft in the description.

Nope, #704 implements the graceful loading part of this, and there was never a fruitful discussion about the logging aspect of this PR

Connum avatar Jun 12 '24 17:06 Connum