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.

Sign up for GitHub

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

Stories projects interoperability: strip Android specifics #478

Open
wants to merge 3 commits into
base: issue/discuss-interop-format
Choose a base branch
from
Open

Stories projects interoperability: strip Android specifics #478

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28b78d2
removed SaveResultReason from interop json definition
mzorz Aug 4, 2020
7188bcd
normalized fontSize, textColor, textAlignment
mzorz Aug 4, 2020
186679d
added metadata object
mzorz Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 35 additions & 33 deletions stories-interop-example.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
{
"metadata": {
"revision": "1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would be the revision number to correctly attempt deserialization.

"device": {
"screen": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the device object for now contains a screen width/height measures as informed by the device, given all other positioning measures will be relative to this canvas size.

"width": 1080,
"height": 1280
},
"platform": "android|ios",
"model": "iphone10"
}
},
"frames": [
{
"source": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

regarding this:
"type": "com.wordpress.stories.compose.story.StoryFrameItem.BackgroundSource.FileBackgroundSource",
I'm not sure we can change this on Android - we could by writing our own serializer for this class though, just commenting here. Maybe we can keep the format, even if this verbose?

Expand All @@ -18,17 +29,14 @@
"scale": 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

             "translationX": -180,
            "translationY": -861.4966,

these would remain the same, and are always calculated from the center of the screen where the object is initially placed. Added the screen size for this as part of the metadata object. In this particular example, the user would have moved the object 861 dp up (that's why it has a negative Y translation) and 180 dp to the left.

"addedViewTextInfo": {
"text": "this is some text",
"fontSizePx": 140,
"textColor": -1,
"textAlignment": 2
"fontSize": 14,
Copy link
Contributor Author

@mzorz mzorz Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

suggest we change this from fontSizePx to fontSize where the size is expressed in pt (which would be absolute) or em? deferring to @aforcier here, don't know what the best unit would be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, what looks like a good "universal" unit on iOS @bjtitus ? wonder if they save this information in Kanvas already, maybe we can borrow any ideas from there?

Copy link

Choose a reason for hiding this comment

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

We use point sizes, typically. I believe this is the same as the "density-independent pixels" size on Android:
https://en.wikipedia.org/wiki/Point_(typography)#Apple_point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah cool! yes they even seem to have the same correlation, TIL @bjtitus 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like sp would be the way to go then 👍 A few more text size-related things are added with #494, most of them (shadow radius, shadow dx, shadow dy) can be in sp as well.

One exception would be letterspacing, which is stored in ems (here's the documentation), which should be an appropriate unit to keep it in.

@mzorz probably once the text feature work is done we can update this with all the new units and expand the discussion a bit - I'll follow up with you on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good!

"textColor": "#00000000",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this hex code format comes in handy for cross-platform text representation so, maybe a no-brainer, let me know if there's any other better options.

"textAlignment": "left|center|right"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably we can do some app-level defined values like these three?

Copy link

Choose a reason for hiding this comment

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

iOS has 4 built in text alignments:

  • left
  • right
  • center
  • justified
  • natural

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! is natural something to do with RTL, or is it dependent on the font?
[searches for it]
ah I see yes it goes with the default alignment for RTL https://developer.apple.com/documentation/uikit/nstextalignment/natural
I think if we use this we'd need to also add information about RTL to treat it the same way on every device / platform. We may want to just use maybe the first four and make sure all is contained for interoperability, or we can make this a "loose" field value and let the app figure out whatever it wants / can do with the given value here.

}
},
"uri": null
}
],
"saveResultReason": {
"type": "com.wordpress.stories.compose.frame.StorySaveEvents.SaveResultReason.SaveSuccess"
}
]
},
{
"source": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding this:
- "contentUri": "content://media/external/video/media/639"
We should probably get rid of this one altogether, it made sense for Android at the beginning since the source of a background image could be a third party app for example, that implements a Content Provider. I think given other reasons we've been discussing, we're going to have a local copy of the background image, and as such, we'll only need a file source here.
In the context of WordPress, such background image will also need to be uploaded to the WordPress site, and as such this will end up always being an https:// URL pointing to the background file (we could do a mediaID but the stories library should be agnostic of WordPress entities).

Expand All @@ -49,9 +57,9 @@
"scale": 1,
"addedViewTextInfo": {
"text": "this is some text, on a video background",
"fontSizePx": 140,
"textColor": -1,
"textAlignment": 4
"fontSize": 14,
"textColor": "#ffffffff",
"textAlignment": "left|center|right"
}
},
"uri": null
Expand All @@ -65,17 +73,14 @@
"scale": 1,
"addedViewTextInfo": {
"text": "😁",
"fontSizePx": 284,
"textColor": -16777216,
"textAlignment": 4
"fontSize": 28,
"textColor": "#ffffffff",
"textAlignment": "left|center|right"
}
},
"uri": null
}
],
"saveResultReason": {
"type": "com.wordpress.stories.compose.frame.StorySaveEvents.SaveResultReason.SaveSuccess"
}
]
},
{
"source": {
Expand All @@ -95,9 +100,9 @@
"scale": 1,
"addedViewTextInfo": {
"text": "one text view",
"fontSizePx": 140,
"textColor": -1,
"textAlignment": 2
"fontSize": 14,
"textColor": "#ffffffff",
"textAlignment": "center"
}
},
"uri": null
Expand All @@ -111,9 +116,9 @@
"scale": 1,
"addedViewTextInfo": {
"text": "another text view",
"fontSizePx": 140,
"textColor": -1,
"textAlignment": 2
"fontSize": 14,
"textColor": "#ffffffff",
"textAlignment": "center"
}
},
"uri": null
Expand All @@ -127,9 +132,9 @@
"scale": 1,
"addedViewTextInfo": {
"text": "third text view",
"fontSizePx": 140,
"textColor": -1,
"textAlignment": 2
"fontSize": 14,
"textColor": "#ffffffff",
"textAlignment": "center"
}
},
"uri": null
Expand All @@ -143,17 +148,14 @@
"scale": 1,
"addedViewTextInfo": {
"text": "😬",
"fontSizePx": 284,
"textColor": -16777216,
"textAlignment": 4
"fontSize": 28,
"textColor": "#ff55ff",
"textAlignment": "right"
}
},
"uri": null
}
],
"saveResultReason": {
"type": "com.wordpress.stories.compose.frame.StorySaveEvents.SaveResultReason.SaveSuccess"
}
]
}
],
"title": null
Expand Down