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

Code Review: Fill in all schema description fields and tidy up TODOs (#6) #7

Merged
merged 8 commits into from
Oct 9, 2019

Conversation

jedrichards
Copy link
Member

@jedrichards jedrichards commented Sep 26, 2019

Closes #6

This PR will enhance the schemas by filling in human readable descriptions for the top level objects, and tidies up some loose ends indicated by TODO comments.

I don't consider this PR ready for final review yet, so it's still in draft status. It needs dedicated input from someone in the Mac team close to the file format. The description values I've added need going over carefully and determining whether they make sense in the historical/technical context of the file format - corrections, additions and improvements welcome. Any suggestions for extra detail in descriptions welcome too, I realise some of mine may be a bit terse.

At this stage we're just adding descriptions to the top level objects, but a future effort could be to add descriptions to more properties, especially where some discussion would aid those implementing this spec in other products.

Questions

Below are the outstanding questions I have. Once I've had answers to these in this thread I'll incorporate into this PR and we can move towards a final review and merge.

  • What is the purpose of the enum that appears in the lineSpacingBehaviour field of a text layer? How is it edited via the UI? What are the descriptions for each of its numerical values?
  • What are the purpose of the startMarkerType and endMarkerType fields on a style object? How are they edited via the UI?
  • I've got -1, 0, 1 and 2 values for the pointRadiusBehaviour enum on shape layers, what could be considered human readable descriptions for these?
  • What could be considered a human readable description for the enum that appears on the resizingType field of a layer?
  • What are the full range of possible values (and human descriptions) for the enum that appears on the resizingConstraint field of a layer?
  • What is the purpose of the visibleScaleType field on an exportFormat object? How is it changed from the UI?
  • What is the purpose of the namingScheme field on an exportFormat object? Is it an enum? If so what are its possible values and associated human readable descriptions?
  • What is the purpose of the MSImmutableFreeformGroupLayout object? It always seems to be empty
  • Is the imageCollection object in use? It always seems to be empty
  • How is the symbolContainerobject that appears on the document used? It always seems to be empty
  • What is the strikethroughStyle field on a text style? Is it an enum? How is it edited via the UI?
  • What is textStyleVerticalAlignmentKey on a text style?

@mikeabdullah
Copy link

Some answers/pointers as best I can:

What is the purpose of the enum that appears in the lineSpacingBehaviour field of a text layer? How is it edited via the UI? What are the descriptions for each of its numerical values?

My memory says we used to use the text system's default line spacing approach, but now have something that mimics what web browsers do. The enum is for compatibility to know which one should be in use. @randomsequence is best to know I think?

I've got -1, 0, 1 and 2 values for the pointRadiusBehaviour enum on shape layers, what could be considered human readable descriptions for these?

Also @randomsequence is best to answer I believe.

What are the full range of possible values (and human descriptions) for the enum that appears on the resizingConstraint field of a layer?

This one corresponds to BCConstraint in the Mac code which I think is fairly well documented. It's roughly an equivalent of NSAutoresizingMaskOptions.

What is the purpose of the MSImmutableFreeformGroupLayout object? It always seems to be empty

We've got this idea that a group can have a layout object associated with it to control what happens when resized etc. Normal groups have this freeform layout which doesn't really do anything special. But then Smart Layout can kick in to have an effect and other things like that.

@randomsequence
Copy link

(a) What is the purpose of the enum that appears in the lineSpacingBehaviour field of a text layer? (b) How is it edited via the UI? (c) What are the descriptions for each of its numerical values?

(a) This was new in Sketch 3.6. If you have fixed line height on a layer we use a custom layout manager to make sure the baselines are a fixed distance apart, even with mixed fonts, like in a browser. This is not how the system layout manager behaves.

(b) Layers created before 3.6 have the old behaviour. Changing the fixed line height on any layer in 3.6 or later switches it to the new behaviour. There is no UI for setting it back to the old.

(c) We haven't yet expose this to users, but if you really want one I'd go with "legacy" and "consistent baseline"

@randomsequence
Copy link

randomsequence commented Sep 27, 2019

I've got -1, 0, 1 and 2 values for the pointRadiusBehaviour enum on shape layers, what could be considered human readable descriptions for these?

See MSPointRadiusBehaviour.h

typedef NS_ENUM(NSInteger, MSPointRadiusBehaviour) {
  MSPointRadiusBehaviourV0 = 0, // Legacy mode, from the beginning of time to #9488
  MSPointRadiusBehaviourV1 = 1, // From #9488
  MSPointRadiusBehaviourV1Smooth = 2, // Use smooth curvature corner radius
  
  /// Builds the path without any corner radii. e.g. for https://github.com/BohemianCoding/Sketch/issues/13002
  MSPointRadiusBehaviourDisabled = -1,
  
  MSPointRadiusBehaviourLegacy = MSPointRadiusBehaviourV0,
  MSPointRadiusBehaviourCurrent = MSPointRadiusBehaviourV1,
};

For user-facing strings, I'd go with:

0: Legacy
1: Rounded
2: Smooth
3: Disabled

See also: https://blog.sketchapp.com/introducing-libraries-and-smooth-corners-in-sketch-47-2abc5dfc1fb3

@jedrichards
Copy link
Member Author

@mikeabdullah @randomsequence Thanks for your help so far dudes! I'm off on holiday next week, but please keep any info/input coming in the meantime if you're able, and I'll incorporate when I'm back.

@opsGavin
Copy link
Contributor

A few more gaps Jed:

What are the purpose of the startMarkerType and endMarkerType fields on a style object? How are they edited via the UI?

They denote the type of line ending:

image

What is the purpose of the visibleScaleType field on an exportFormat object? How is it changed from the UI?

The posible values are:

  MSUserVisibleScaleTypeScale = 0,
  MSUserVisibleScaleTypeWidth = 1,
  MSUserVisibleScaleTypeHeight = 2,

They determine how the exported layer should be scaled. E.g. in the export options you can enter a size like 200w which means that the image size will be 200 wide and the height will be determined by resizing such that the aspect ratio is preserved.

What is the purpose of the namingScheme field on an exportFormat object? Is it an enum? If so what are its possible values and associated human readable descriptions?

  MSExportFormatNamingSchemeSuffix = 0,
  MSExportFormatNamingSchemePrefix = 1

The export file name will be the layer name with an optional suffix or prefix (e.g. @2x). This just lets you choose which

Is the imageCollection object in use? It always seems to be empty

I believe this is legacy and only kept for migrating old documents.

How is the symbolContainerobject that appears on the document used? It always seems to be empty

It's not used. We have shared layer and text style containers. The original idea was that symbols would similarly be in a symbol container, but that didn't happen.

What is the strikethroughStyle field on a text style? Is it an enum? How is it edited via the UI?

I think this is the strike through that can be set on text via:

image

As far as I can tell, the valid values are defined by Apple:

typedef NS_OPTIONS(NSInteger, NSUnderlineStyle) {
    NSUnderlineStyleNone                                    = 0x00,
    NSUnderlineStyleSingle                                  = 0x01,
    NSUnderlineStyleThick NS_ENUM_AVAILABLE(10_0, 7_0)      = 0x02,
    NSUnderlineStyleDouble NS_ENUM_AVAILABLE(10_0, 7_0)     = 0x09,

    NSUnderlineStylePatternSolid NS_ENUM_AVAILABLE(10_0, 7_0)      = 0x0000,
    NSUnderlineStylePatternDot NS_ENUM_AVAILABLE(10_0, 7_0)        = 0x0100,
    NSUnderlineStylePatternDash NS_ENUM_AVAILABLE(10_0, 7_0)       = 0x0200,
    NSUnderlineStylePatternDashDot NS_ENUM_AVAILABLE(10_0, 7_0)    = 0x0300,
    NSUnderlineStylePatternDashDotDot NS_ENUM_AVAILABLE(10_0, 7_0) = 0x0400,

    NSUnderlineStyleByWord NS_ENUM_AVAILABLE(10_0, 7_0)            = 0x8000
} NS_ENUM_AVAILABLE(10_0, 6_0);

(Yep this says underline, but I think it's used for strikethrough as well 😕)

What is textStyleVerticalAlignmentKey on a text style?

Just how text can be vertically alined to it's container:

  MSTextStyleVerticalAlignmentTop = 0,
  MSTextStyleVerticalAlignmentMiddle = 1,
  MSTextStyleVerticalAlignmentBottom = 2,

image

@jedrichards
Copy link
Member Author

@opsGavin 🙇

@jedrichards jedrichards marked this pull request as ready for review October 8, 2019 16:29
@jedrichards
Copy link
Member Author

jedrichards commented Oct 8, 2019

@mikeabdullah

This one corresponds to BCConstraint in the Mac code which I think is fairly well documented. It's roughly an equivalent of NSAutoresizingMaskOptions.

I'm seeing this in the BCConstraint file:

BCConstraintNone           = 0,
BCConstraintMaxXSizeable   = 1U << 0,
BCConstraintWidthSizeable  = 1U << 1,
BCConstraintMinXSizeable   = 1U << 2,
BCConstraintMaxYSizeable   = 1U << 3,
BCConstraintHeightSizeable = 1U << 4,
BCConstraintMinYSizeable   = 1U << 5,
BCConstraintAllFixed       = 1U << 6,

How do these play out into real values in the document JSON? I'm confused because I thought for example 1U << 6 is equivalent to 64, but I'm seeing a value of 63 in the wild in document JSON resizingConstraint fields ...

@christianklotz
Copy link
Contributor

@jedrichards where did you come across the 63 value?

@randomsequence
Copy link

BCConstraintMaxXSizeable | BCConstraintWidthSizeable | BCConstraintMinXSizeable | BCConstraintMaxYSizeable | BCConstraintHeightSizeable | BCConstraintMinYSizeable
== 00000001 | 00000010 | 00000100 | 00001000 | 00010000
== 00011111
== 63

@jedrichards
Copy link
Member Author

jedrichards commented Oct 9, 2019

Ok gotcha, thanks - so in terms of what 63 actually means when it appears in JSON ... it's basically the same as 64? All constraints active, i.e. all fixed? Or is that wrong? Here's the JSON Schema I've got going so far for it:

title: Resize Constraint
description: Enumeration of the possible types of resizing constraints
type: integer
enum:
  - 0
  - 1
  - 2
  - 4
  - 8
  - 16
  - 32
  - 64
enumDescriptions:
  - All flexible
  - Right margin flexible
  - Width flexible
  - Left margin flexible
  - Top margin flexible
  - Height flexible
  - Bottom margin flexible
  - All fixed

@randomsequence
Copy link

randomsequence commented Oct 9, 2019

It's not an enumeration, it's a bit field (or set of flags) - the values can be combined together. I've no idea how you do that in js 😳

@jedrichards
Copy link
Member Author

jedrichards commented Oct 9, 2019

Ah yeah I was beginning to have a hunch something like that might be the case ... it's not so much doing it in JS, it's just trying to formalise it into a schema that describes the possible range of values to be found in the JSON.

Maybe the resizingConstraints field just needs to be a positive integer for now? Or can you think of any way to tighten that up, or add a bit more information there? Another way to think about it is that the schema validates the JSON and let's us know whether its correct, i.e. parsable by Sketch.

@randomsequence
Copy link

By combining the values you can make any integer from 0 to 127, so I guess a positive integer should work.

I think you're going to encounter quite a few of these bitfields though, so it might be something you need to come up with a proper solution for.

@jedrichards
Copy link
Member Author

Defining it as an integer between 0 and 127 sounds good for now. I guess that is fundamentally what the value is, and if people need to reproduce a meaningful value they'll need to do some bit-shifting themselves, which is out of scope of this schema.

I didn't come across anything else so far that looked like a bitfield, but I may have been mistaking some for enums, not sure.

jedrichards and others added 3 commits October 9, 2019 17:21
Co-Authored-By: Christian Klotz <hello@christianklotz.co.uk>
Co-Authored-By: Christian Klotz <hello@christianklotz.co.uk>
Co-Authored-By: Christian Klotz <hello@christianklotz.co.uk>
@jedrichards jedrichards merged commit 3765ff1 into master Oct 9, 2019
@jedrichards jedrichards deleted the feature/6-descriptions-todos branch October 9, 2019 17:44
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.

Fill in all schema description fields and tidy up TODOs
5 participants