Encoding source-specific data in Octopus #47

Open scoiatael opened 1 year ago

In GitLab by @janroztocil on Jun 17, 2021, 17:13

Octopus is a generic format which unifies all the possible sources.

Ideally, this statement should hold in all of the case, we should always try to find the universal representation for whichever phenomena. However, sometimes the essence of a particular feature is such that this is just not possible, or at best would bring confusion and unnecessary complexity into the format. Sometimes the fundamentals are too different and the feature requires specific implementations for particular sources anyway.

We originally thought that we'd be solving the issue in the next major Octopus release (ver. 3 and forth), however, recent requirements for Octopus have shown a need to address it sooner. The concrete incoming changes are "auto layout configuration" for other sources than Sketch (namely Figma) and Figma image adjustments.

Auto layouts configuration (backend/octopus#45)

Currently there's only smartLayout for Sketch - it can be on document root (symbol master) or a group layer (inside a symbol master).

I propose to

1) introduce new layout object which would be used in layers or on documents, and only inside the object there would be one of the concrete configurations

2) explicitly distinguish the sources in the keys, i.e. name them smartLayout_sketch, autoLayout_figma, contentAwareLayout_xd... or may be even just simply: layout_sketch, layout_figma, layout_xd - we don't need to put drag their marketing into our open format

"layout": {
    "smartLayout_sketch": "HORIZONTAL_CENTER",      // <-----

    "autoLayout_figma": {                           // <-----
        "layoutAlign": stretch,
        "layoutMode": vertical,
        "primaryAxisSizingMode": fixed,
        "counterSizingMode": auto,

        "primaryAxisAlignItems: "space_between",
        "counterAxisAlignItems: "min",

        "paddingLeft": 12,
        "paddingRight": 54,

        // padding(Left,Right,Top,Bottom,Horizontal,Vertical)

        "itemSpacing": 34.56,
    }

    "contentAwareLayout_xd": {                      // <-----
        "isContentAware": true,
        "contentStackType": horizontal,
        "contentPaddingLeft: 12.34,

        // ... contentPadding{left,right,top,bottom}

        "contentStackMarginAfter": 12.34,

        "canonicalPaddingLeft": 12.34,
        // ... "canonicalPadding{Left,Right,Bottom,Top}
    }
}

Image adjustments backend/octopus#45

Currently there's only a colorAdjust effect coming from Sketch. We need to introduce adjustments from Figma as well. Some of the properties overlap, however, the meaning and the implementation are different. Therefore, putting the source name into the key only seems reasonable.

I propose this:

"effects: {

    "colorAdjust_sketch": {
        "hue: 2.107207218659309,
        "brightness": -0.2503439465408805,
        "contrast": 2.52623820754717,
        "saturation": 1.701306996855346
    }

    "colorAdjust_figma": {
        "exposure": 0,
        "contrast": 0,
        "saturation": 0,
        "temperature": 0,
        "tint": 0,
        "highlights": 0,
        "shadows": 0
    }

}

Summary/questions

Do you agree with adding the source name into the specific keys?
Would you prefer other format? E.g. hiding it in an object: "layout.sketch.config", "layout.xd.config"? ... or using a colon sign: "layout.config:xd", "layout.config:sketch"?
Or would you propose different approach? Like having a dedicated object directly on layer where all the source-specific data would lie?

I personally stand behind what I proposed in the examples above (w/o the brand names) but it's definitely open for discussion.

In GitLab by @nikitamironov on Jun 17, 2021, 17:50

Regarding layouts: do we need 2 levels ("layout" + "layoutXd" inside) instead of just having "layout" as discriminated union? Example of what I mean:

type Layout = LayoutXd | LayoutSketch | LayoutFigma
type LayoutXd = {
  type: "xd",
  someXdSpecificProp: 'SOME_VALUE'
}
type LayoutSketch = {
  type: "sketch",
  someSketchSpecificProp: 'SOME_VALUE'
}
type LayoutFigma = {
  type: "figma",
  someFigmaSpecificProp: 'SOME_VALUE'
}
type OctopusLayer = {
  // ...
  layout: Layout,
  // ...
}

I think source format shouldn't be part of a key and should rather be a value of some generic property (in this case its type prop). It will help to avoid checking if any of those keys exist on a structure and just handle processing by the value in such type prop. Of course it's only valid when only one possible type can exist for given purpose (but I dont think we can meet situation when sketch and figma conflicting in same design).

In GitLab by @janroztocil on Jun 18, 2021, 12:59

Sure, it can be like you propose. The advantage is that's possible to formally encode in the specification the fact that at most one of those fields is allowed there. Also a mapping of which type is determined by which value is possible.

My idea was to be explicit and kind of self-explanatory in the Octopus. If we take the "color adjust" as an example then let's say colorAdjust.contrast means something completely different for octopuses from Sketch and Figma... and yet Octopus gives the impression that it's something that can be viewed the same. So that was my reasoning - to optically and syntactically separate them.

Of course, this could be solved if users always used parsers generated from the specification and always useed some sort of type checking, then the colorAdjust.contrast would be of different type and there wouldn't be a problem.

And that's probably a correct approach, we should encourage users to use the formal tools over just guessing from the data. My thinking was too much in the Octopus2 world, because there it's basically everywhere - self explicatory aspect of the format is preferred over correctness and compactness.

And we should start changing that, and as we can't wait for Octopus3 we should start incorporating it into what we have now.

Unless other suggestions occur during upcoming days, I'll prepare an update of Octopus specs for both of the incoming features.

In GitLab by @rychlis on Jun 18, 2021, 16:46

Definetely go ahead and prepare the update, I'm OK with having a format-specific data in Octopus for such cases where unification would be impossible/confusing.

Would you prefer other format? E.g. hiding it in an object: "layout.sketch.config", "layout.xd.config"? ... or using a colon sign: "layout.config:xd", "layout.config:sketch"?

Nikitas approach looks best to me, but i dont have a supserstrong opinion about that.

In GitLab by @janroztocil on Jun 24, 2021, 15:57

OpenAPI specs for the mentioned features are in backend/octopus-schema in

https://gitlab.avcd.cz/backend/octopus-schema/-/merge_requests/28 (auto layouts)

and

https://gitlab.avcd.cz/backend/octopus-schema/-/merge_requests/29 (color adjust)

opendesigndev / octopus-specs