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

Make formatVersion in metainfo.plist a major/minor #78

Closed
typesupply opened this issue Jan 8, 2019 · 28 comments · Fixed by #136
Closed

Make formatVersion in metainfo.plist a major/minor #78

typesupply opened this issue Jan 8, 2019 · 28 comments · Fixed by #136
Assignees
Labels
approved Approved specification change. proposal Proposed specification change. ufo3 UFO 3 issues.

Comments

@typesupply
Copy link
Contributor

We have switched to an incremental approach to updating the spec rather than holding lots of changes until a major version bump. To make it easier for readers to more easily identify these new format UFOs we need to make the formatVersion in metainfo.plist a float instead of an int.

@justvanrossum
Copy link
Contributor

Suggest to only ever to increment by 0.1, and not go over 3.9. After that 4.0 is inevitable and then we should change the format specifier in metainfo.plist.

@anthrotype
Copy link
Member

What about, instead of changing the type of the existing formatVersion element from int to float (which would break existing implementation), we add an additional int element <formatVersionMinor> which defines the minor version, and the existing <formatVersion> continues to define (implicitly) the major version?
This way existing clients will (with all probability) ignore the extra element (unless they are overly strict in their validation), while updated clients will be able to read the new formatVersionMinor element and act accordingly.

We could settle on a versioning policy whereby we only ever change the major version when: 1) we remove support for existing pieces of data; or 2) change their semantics in a breaking way. Whereas we would bump the minor version every time we add extra elements or attributes to existing elements or define new public. lib keys or add whole new files to the UFO package.

@anthrotype
Copy link
Member

(maybe formatMinorVersion reads better in English than formatVersionMinor)

@anthrotype
Copy link
Member

Alternatively, we would be forced to bump the formatVersion directly to 4.0, that is bump the major version because we changed the type of formatVersion element itself from int to float.

@typesupply
Copy link
Contributor Author

I like the formatVersionMinor idea. We have a versionMinor field in fontinfo.plist so I'm fine with the awkward order.

@benkiel
Copy link
Contributor

benkiel commented Nov 1, 2019

+1

@anthrotype
Copy link
Member

OK, I'll work on fontTools.ufoLib support for parsing a new optional formatVersionMinor element in metainfo.plist.
But what about the GLIF format attribute? Do we want to add a new optional formatMinor attribute? Or do we re-purpose the existing format attribute to be a dot-separated "minor.major" string?
XML attributes (unlike plist) do not have an intrinsic type, they're just strings.
I am leaning towards simply doing format="2.1", instead of adding new formatMinor for GLIF.

WDYT?

@benkiel
Copy link
Contributor

benkiel commented Nov 21, 2019

That sounds like the simplest thing

@anthrotype
Copy link
Member

Hm. However the whole point of defining an additional minor version was to avoid breaking existing tools, since the new features are meant to be additive. If we change the meaning of GLIF format attribute from an integer to a dotted version string, then existing tool will break (e.g. you won't be able to read a UFO 3.1 with GLIF 2.1 in an editor that only supports up to UFO3/GLIF2).
Older versions of fontTools.ufoLib, to take one, will refuse to load such GLIF 2.1 with "Unsupported format version" because it casts to int() and check if it's in (1, 2).

@anthrotype
Copy link
Member

cc @justvanrossum

@benkiel
Copy link
Contributor

benkiel commented Nov 21, 2019

@anthrotype You've just argued against your earlier lean; but this raises a really good point. Based on that, I would think the thing to do is to also add formatMinor to GLIF, keeping format for the major format. Nothing will break then.

@justvanrossum
Copy link
Contributor

Should we consider bumping the major version to 4 after all, but keep the amount of spec changes low, to increase the chance that it will be adopted soon?

@anthrotype
Copy link
Member

anthrotype commented Dec 13, 2019

I think adding a extra formatMinorVersion (and formatVersion attribute in GLIF) would give lend us more flexibility in the future for adding non-breaking-changes stuff like the verticalOrigin.
The way I'm implementing them in ufoLib is so that a UFO 3.1 will still be read in by older versions with its additional data (silently) ignored. I think this is better than the font being completely rejected as invalid, especially for things like the verticalOrigin or height which are relatively less common in predominantly horizontal-scripts-centric font develpment workflows.

@justvanrossum
Copy link
Contributor

justvanrossum commented Dec 13, 2019

more flexibility in the future

Oh, that is beyond doubt.

If you think we can pull off a 3.1 without breakage that would be awesome.

(glyph.height returning None instead of 0 would be a breaking change, though, no? #95)

@anthrotype
Copy link
Member

anthrotype commented Dec 13, 2019

(glyph.height returning None instead of 0 would be a breaking change, though, no?)

hm, you got me there.. 🤔
The current behavior of glifLib is to not write a height attribute when the glyph.height value is 0.
Whereas the new glifLib would not write the height attribute when getattr(glyph, "height", None) is None.
So if one reads a new GLIF 2.1 using the old glifLib, then an omitted height attribute would be mistaken for a height of 0. That's not good, but..
Conversely if one reads an old GLIF 2.0 using the new glifLib updated to support formatMinor and new optional-height-default, then we could decide to do two things when faced with an omitted height attribute:

  1. either we interpret this as an implicit height = 0 (but then as we save the UFO with the latest format all the glyphs will end up with an additional explicit height set to 0; maybe not too bad); or
  2. intepreted as (incorrectly) meaning that the default height (asc - desc) is not overridden by this particular glyph and be unset to None.

Ah, another thing. Since these proposed changes to verticalOrigin and glyph height only concern the GLIF file and not the rest of the UFO files or structure, and given GLIF format has a version of its own, I think it would make more sense to only increment the GLIF format version (from 2 to 2.1) and not also the UFO format version which could stay 3 (err 3.0, formatVersion=3 and implicit formatVersionMinor=0 in the metainfo.plist).

@benkiel
Copy link
Contributor

benkiel commented Dec 13, 2019

Regardless of the #95, I think we need to add the additional minor format versions to the spec, it's clearly needed.

@justvanrossum
Copy link
Contributor

I'd be curious about @BlackFoundry's opinion about the height issue, given they use ufo for cjk.

@anthrotype
Copy link
Member

I made the change in fonttools fonttools/fonttools#1786 but I am not sure where exactly we should mention these new formatVersionMinor (in metainfo.plist) and formatMinor attribute in GLIF <glyph> element. Technically they don't belong to the current UFO 3(.0) and GLIF 2(.0) specs, but to the not yet defined UFO 3.1 and GLIF 2.1.
Maybe I can merge that fonttools PR now (given it doesn't change reading/writing current UFO, as the minor digit is omitted/implied when 0), and then when we are ready to push some backward-compatible change to the spec, we'll mention in the new section for the newly defined spec version that it adds these new elements/attributes?

@benkiel
Copy link
Contributor

benkiel commented May 7, 2020

☝️ This makes the most sense, and is a good prompt to get some spec updates for UFO 3.1 and GLIF 2.1 started and done. Otherwise, we can bump to 3.1 and 2.1 just for this, but that seems silly. @typesupply?

@typesupply
Copy link
Contributor Author

Fine with me if everyone else is cool with it.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jun 17, 2020
4.12.1:
- [_n_a_m_e] Fixed error in ``addMultilingualName`` with one-character names.
  Only attempt to recovered malformed UTF-16 data from a ``bytes`` string,
  not from unicode ``str``.

4.12.0:
- [otlLib/varLib] Ensure that the ``AxisNameID`` in the ``STAT`` and ``fvar``
  tables is grater than 255 as per OpenType spec.
- [docs] Document more modules in ``fontTools.misc`` package: ``filenames``,
  ``fixedTools``, ``intTools``, ``loggingTools``, ``macCreatorType``, ``macRes``,
  ``plistlib``.
- [OS/2] Don't calculate whole sets of unicode codepoints, use faster and more memory
  efficient ranges and bisect lookups.
- [voltLib] Support writing back abstract syntax tree as VOLT data.
- [voltLib] Accept DO_NOT_TOUCH_CMAP keyword.
- [subset/merge] Fixed a namespace clash involving a private helper class.

4.11.0:
- [feaLib] Introduced ``includeDir`` parameter on Parser and IncludingLexer to
  explicitly specify the directory to search when ``include()`` statements are
  encountered.
- [ufoLib] Silently delete duplicate glyphs within the same kerning group when reading
  groups.
- [ttLib] Set version of COLR table when decompiling COLRv1 (commit 9d8a7e2).

4.10.2:
- [sfnt] Fixed ``NameError: SimpleNamespace`` while reading TTC header. The regression
  was introduced with 4.10.1 after removing ``py23`` star import.

4.10.1:
- [sfnt] Make ``SFNTReader`` pickleable even when TTFont is loaded with lazy=True
  option and thus keeps a reference to an external file.
- [feaLib.ast] Restore backward compatibility (broken in 4.10 for
  ``ChainContextPosStatement`` and ``ChainContextSubstStatement`` classes.
  Make them accept either list of lookups or list of lists of lookups.
- [docs] Document some modules in ``fontTools.misc`` package: ``arrayTools``,
  ``bezierTools`` ``cliTools`` and ``eexec``.
- [ttLib._n_a_m_e] Fixed ``findMultilingualName()`` when name record's ``string`` is
  encoded as bytes sequence.

4.10.0:
- [varLib] Allow feature variations to be active across the entire space.
- [ufoLib] Added support for ``formatVersionMinor`` in UFO's ``fontinfo.plist`` and for
  ``formatMinor`` attribute in GLIF file as discussed in unified-font-object/ufo-spec#78.
  No changes in reading or writing UFOs until an upcoming (non-0) minor update of the
  UFO specification is published.
- [merge] Fixed merging fonts with different versions of ``OS/2`` table.
- [subset] Fixed ``AttributeError`` while subsetting ``ContextSubst`` and ``ContextPos``
  Format 3 subtable.
- [ttLib.table._m_e_t_a] if data happens to be ascii, emit comment in TTX.
- [feaLib] Support multiple lookups per glyph position.
- [psCharStrings] Use inheritance to avoid repeated code in initializer.
- [Doc] Improved documentation for the following modules: ``afmLib``, ``agl``
 , ``cffLib``, ``cu2qu``, ``encodings``, ``feaLib``, ``merge``.
- [Doc] Split off developer-centric info to new page, making front page of docs more
  user-focused. List all utilities and sub-modules with brief descriptions.
  Make README more concise and focused.
- [otlLib] Add function to build STAT table from high-level description.
- [ttLib._n_a_m_e] Add ``findMultilingualName()`` method.
- [unicodedata] Update ``RTL_SCRIPTS`` for Unicode 13.0.
- [gvar] Sort ``gvar`` XML output by glyph name, not glyph order.
- [Doc] Added help options to ``fonttools`` command line tool.
  Ensure all fonttools CLI tools have help documentation.
- [ufoLib] Only write fontinfo.plist when there actually is content.

4.9.0:
- [subset] Fixed subsetting of FeatureVariations table. The subsetter no longer drops
  FeatureVariationRecords that have empty substitutions as that will keep the search
  going and thus change the logic. It will only drop empty records that occur at the
  end of the FeatureVariationRecords array.
- [subset] Remove FeatureVariations table and downgrade GSUB/GPOS to version 0x10000
  when FeatureVariations contain no FeatureVariationRecords after subsetting.
- [agl] Add support for legacy Adobe Glyph List of glyph names in ``fontTools.agl``
 .
- [feaLib] Ignore superfluous script statements.
- [feaLib] Hide traceback by default on ``fonttools feaLib`` command line.
  Use ``--traceback`` option to show.
- [feaLib] Check lookup index in chaining sub/pos lookups and print better error
  message.
- [feaLib] Fix building chained alt substitutions.
- [Doc] Included all fontTools modules in the sphinx-generated documentation, and
  published it to ReadTheDocs for continuous documentation of the fontTools project
 . Check it out at https://fonttools.readthedocs.io/. Thanks to Chris Simpkins!
- [transform] The ``Transform`` class is now subclass of ``typing.NamedTuple``. No
  change in functionality.
@typesupply
Copy link
Contributor Author

Since this has been rolled out in ufoLib, should it be labelled a UFO 3 or UFO 4 issue? Also, can someone familiar with the change to ufoLib to support this write a description of what it does so that it can be documented?

@typesupply typesupply added approved Approved specification change. proposal Proposed specification change. labels Aug 12, 2020
@alerque
Copy link

alerque commented Aug 12, 2020

Is it too late to revisit this? I don't think float makes any sense for version numbers and is just going to confuse the issue and make other things hard down the road. Floats just aren't good for versioning. Using a major/minor or even major/minor/patch versioning scheme would be awesome (ala Semver) but a float just confuses the issue. It won't be backwards compatible, it will look like a mathematical value but you won't be able to do normal math (x < y) on it, etc. Of course the other option is to just stick with integers. If introducing a breaking change from 3 to 3.1 is somehow okay (i.e. an existing field that needs parsing as a different type) then just calling it 4 and not being afraid to run up the integer count is probably a better plan.

@justvanrossum
Copy link
Contributor

Check ufoLib: it has not been implemented as a float, but as major/minor.

@benkiel
Copy link
Contributor

benkiel commented Aug 12, 2020

I'll write doc for this. Will do this weekend.

@benkiel benkiel self-assigned this Aug 12, 2020
@benkiel benkiel added the ufo3 UFO 3 issues. label Aug 12, 2020
@benkiel
Copy link
Contributor

benkiel commented Aug 12, 2020

And, this should be in 3.1.

@typesupply
Copy link
Contributor Author

I've been working on documenting the specification process and "what version of the spec should this proposal apply to?" is near the top of the page. We have two models for that now:

  1. A change that would trigger code changes in existing tools goes to the next major version.
  2. A change that can be implemented via a lib key goes to the current version.

Where would the minor version fit into this? Is it a hint to tools that "this UFO was last touched by a tool that knows about X, Y, Z new things"? I'm just trying to figure out how to fit this into the process.

@benkiel
Copy link
Contributor

benkiel commented Aug 12, 2020

I think that @anthrotype will have a better nuanced view than I, but i would say that minor versions are for things that won't break existing tools but add information, such as the addition of version minor (yes, this is recursive). Additions to fontinfo would fall under this too, at least in my mind.

@typesupply
Copy link
Contributor Author

i would say that minor versions are for things that won't break existing tools but add information, such as the addition of version minor (yes, this is recursive).

Makes sense. I'll work this into the process.

Additions to fontinfo would fall under this too, at least in my mind.

Cool. That relates to #128.

@benkiel benkiel changed the title Make formatVersion in metainfo.plist a float. Make formatVersion in metainfo.plist a versionMajor.versionMinor Aug 16, 2020
@benkiel benkiel changed the title Make formatVersion in metainfo.plist a versionMajor.versionMinor Make formatVersion in metainfo.plist a major/minor Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved specification change. proposal Proposed specification change. ufo3 UFO 3 issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants