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

Stories projects interoperability: strip Android specifics #478

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

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 4, 2020

See #477 first

Discussion

Here I'm proposing some of the changes to the current serialization format, to get to a format that works for both Android and iOS.

  • 28b78d2 removed SaveResultReason which was a very local state representation
  • 7188bcd normalized fontSize, textColor, textAlignment
  • 186679d added metadata object which tells more about the context in which this Story has been created:
  "metadata": {
    "revision": "1.0",
    "device": {
      "screen": {
        "width": 1080,
        "height": 1280
      },
      "platform": "android|ios",
      "model": "iphone10"
    }
  },
  • the revision is the interop JSON definition revision number to correctly attempt deserialization.
  • 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.
  • platform and model are self-explanative. We can add brand or vendor too which makes sense for Android.

Adding more comments on the code itself down here in the PR 👍

"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?

@@ -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.

"fontSizePx": 140,
"textColor": -1,
"textAlignment": 2
"fontSize": 14,
Copy link
Contributor Author

@mzorz mzorz Aug 4, 2020

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": -1,
"textAlignment": 2
"fontSize": 14,
"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": 2
"fontSize": 14,
"textColor": "#00000000",
"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.

"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).

@@ -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.

"metadata": {
"revision": "1.0",
"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.

@mzorz mzorz requested review from bjtitus and aforcier August 4, 2020 16:02
@aforcier aforcier mentioned this pull request Aug 15, 2020
17 tasks
@bjtitus bjtitus removed their request for review July 10, 2024 20:45
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.

3 participants