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

[WIP] [designspaceLib] designspace docs edits #1750

Merged
merged 11 commits into from
Nov 7, 2019

Conversation

justvanrossum
Copy link
Collaborator

@justvanrossum justvanrossum commented Oct 18, 2019

- added docs for DesignSpaceDocument object, methods and attributes
- removed comments on validation, localisation and generating UFO instances.
- added note that axis minimum, default and maximum are in userspace coordinates.
- added clarification to map input (userpace!) / output (designspace!) values.
- added note that sourceDescriptor location is in designspace coordinates.
- moved comment on rule subs to rule descriptor object.
- added proposed "processing" flag to rules element
- move note on sub element
Add comment
@justvanrossum
Copy link
Collaborator Author

Would the new processing attribute in the rules element need a format version update? Say 4.1?

(See also #1749)

justvanrossum added a commit to justvanrossum/fonttools that referenced this pull request Oct 18, 2019
Copy link
Collaborator Author

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

Apart from the import statement in the example, this looks good to me.

Doc/source/designspaceLib/readme.rst Outdated Show resolved Hide resolved
Comment on lines -60 to -71
Default font
============

- The source with the ``copyInfo`` flag indicates this is the default
font.
- In mutatorMath the default font is selected automatically. A warning
is printed if the mutatorMath default selection differs from the one
set by ``copyInfo``. But the ``copyInfo`` source will be used.
- If no source has a ``copyInfo`` flag, mutatorMath will be used to
select one. This source gets its ``copyInfo`` flag set. If you save
the document this flag will be set.
- Use ``doc.checkDefault()`` to set the default font.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this bit of information going to be moved to another part of the document? Or is it removed completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for other deleted bits, to me they were interesting information

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe these need to moved to a paragraph marked deprecated, rather than be removed?

Copy link
Collaborator

@LettError LettError Oct 26, 2019

Choose a reason for hiding this comment

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

I'm sorry for removing it without proper discussion, I guess I was trying to clean the document up. The motives for removing this text were:

  • Finding or assigning the default font in a designspace used to be more informal and automatic, so there was code to support that. But the role of the default has become much more prominent and it should be assigned explicitly by setting its location and the axes to specific values.
  • This was formalised a while ago when the default is understood to be the font located on the (mapped!) default value on each axis. The author of the designspace has to make sure this is the case, there are no reliable methods of guessing what the default should be.
  • Assigning the default by setting the copyInfo flag then only creates ambiguity.

I could add a paragraph explaining that there may be older designspaces in which the copyInfo flag could mean that the source was intended to be the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could add a paragraph explaining that there may be older designspaces in which the copyInfo flag could mean that the source was intended to be the default.

That sounds perfect, thanks.

Copy link
Collaborator

@LettError LettError Oct 28, 2019

Choose a reason for hiding this comment

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

Fine, we'll keep the copyInfo flag, but remove the note about that it assigns the default.

The default master in the axis is defined in variable font coordinates but that masters can have a different range. So I need to first go through all axis, collect all default coordinates, transform them to the master coordinate space and hope that I find a master with that coordinates. This is to complicated and introduces to many possibilities for failure.

I agree this is not great, the decision was made a while ago by @behdad. It is outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, using the axes default makes .designspace potentially more useful. We can potentially have software that uses .designspace files plus a compiled VF to produce “slices” (subset) etc. I know it’s a bit annoying at first but I think the axes default method is conceptually cleaner.

Copy link
Collaborator

@LettError LettError Oct 28, 2019

Choose a reason for hiding this comment

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

The default master on the default values of the axes is very clear. It can be a problem that these values are in userspace coordinates. As long as there is no axis map this is fine because user space == design space. But adding a map can easily move the expected location of the default. That just means editors need to keep track of that. I updated mine.

Copy link
Contributor

Choose a reason for hiding this comment

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

To come back to the second part of Jany's question, after the decision to keep copy* in:

The copy* flags:

  • if absent, copy * from the default font.
  • if present, copy * from the source that has the flag if you feel like it, or don't, but don't change the source for the default outlines (which is always determined by the axis defaults).

Is that correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct.

----------

- ``processing``: flag, optional. Valid values are [``first``, ``last``]. This flag indicates whether the substitution rules should be applied before or after other glyph substitution features.
- If no ``processing`` attribute is given, interpret as ``first``.
Copy link
Member

Choose a reason for hiding this comment

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

in the other thread @twardoch raised a valid point about the undefined meaning of processing=post when generating instance UFOs from a designspace. Should the spec say something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "UFO instances" section about rules currently doesn't say anything about other glyph substitutions, so I think a general clarification of how to deal with that would be good.

Copy link
Collaborator Author

@justvanrossum justvanrossum Oct 22, 2019

Choose a reason for hiding this comment

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

An important detail is (under "5.0 rules element") "The rules are evaluated in this order". This is currently not true with varLib.featureVars, and I'm not sure it should be. The order is only relevant if rules react on each other (eg. rule N+1 acts on the output of rule N).

While this may be fixable (generate substitution lookups in the order of the original rules), I'm not sure what the use case is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Perhaps this discussion deserves its own issue.)

 - implementation differences
 - varlib vs. mutatormath
 - older versions
 - rules and generating static ufo instances

Updated the description of the `copyInfo` flag of the sourceDescriptor.
@justvanrossum justvanrossum merged commit b8fafdd into master Nov 7, 2019
@justvanrossum justvanrossum deleted the WIP-designspace-docs-edit branch November 7, 2019 18:30
simoncozens pushed a commit to simoncozens/fonttools that referenced this pull request Mar 10, 2020
simoncozens pushed a commit to simoncozens/fonttools that referenced this pull request Mar 10, 2020
Various changes in the designspaceLib readme.rst
- added docs for DesignSpaceDocument object, methods and attributes
- removed comments on validation, localisation and generating UFO instances.
- added note that axis minimum, default and maximum are in userspace coordinates.
- added clarification to map input (userpace!) / output (designspace!) values.
- added note that sourceDescriptor location is in designspace coordinates.
- moved comment on rule subs to rule descriptor object.
- added proposed "processing" flag to rules element
- move note on sub element
- implementation differences
- varlib vs. mutatormath
- older versions
- rules and generating static ufo instances
- Updated the description of the `copyInfo` flag of the sourceDescriptor.
- Change the example import.
- Remove additional mention of copyInfo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants