Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement graceful loading via logging severity levels #648

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Nov 24, 2023

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

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

Checklist:

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

@ILOVEPIE
Copy link
Contributor

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

@ILOVEPIE
Copy link
Contributor

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.

@Connum
Copy link
Contributor Author

Connum commented Nov 24, 2023

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

@ILOVEPIE
Copy link
Contributor

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.

@Connum
Copy link
Contributor Author

Connum commented Nov 24, 2023

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

@Connum Connum force-pushed the WIP/graceful-loading branch from 517f2f0 to d4ac90b Compare November 24, 2023 20:33
} else {
el.style.display = 'block';
el.innerHTML = `<p class="message-type-1">${message}</p>`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use innerText and set the class to el directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd loose the wrapping paragraph element and set the class to the outer message area. But you're right, we definitely don't want the message to be interpreted as HTML. so I'll have to create a new paragraph element and set the class and text on that.

@yne
Copy link
Contributor

yne commented Nov 24, 2023

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
    ?

@Connum
Copy link
Contributor Author

Connum commented Nov 25, 2023

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

@ILOVEPIE
Copy link
Contributor

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

@Connum
Copy link
Contributor Author

Connum commented Jun 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants