Add external media support in documentation #82

Kyle-Ye · 2022-02-08T16:42:28Z

Bug/issue #, if applicable: SR-15839
Forum discussion: https://forums.swift.org/t/can-we-link-and-render-external-images-that-is-from-urls

Summary

Add a new ExternalMediaResolver to support external media reference.

Dependencies

None

Testing

Test external image and video support in docs and articles.

Checklist

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

TODO

Currently this PR does not support external media in tutorials. Will solve it in a follow-up PR.

Kyle-Ye · 2022-02-08T16:55:09Z

Implementation is almost done. Will update test and documentation later

d-ronnqvist

I'm wondering if there's a way to implement this without new render reference types. Ideally the solution could be easily shared to also add support for external videos and external downloads (if that's something that we want to support)

Sources/SwiftDocC/Model/Rendering/References/ExternalImageReference.swift

Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift

franklinsch · 2022-02-28T13:15:25Z

Hi @Kyle-Ye, are you still working on this? Let @d-ronnqvist or I know we can help!

Kyle-Ye · 2022-03-01T14:52:35Z

Hi @Kyle-Ye, are you still working on this? Let @d-ronnqvist or I know we can help!

Yes. Sorry for the late update (Busy with my full-time iOS RD internship recently)

I have a few question here in order to better finish this PR and your review.

After performing "docc preview" action to the xx.docc file, it will produce a xx.docc/.docc-build/data/documentation/swiftdoccrender.json file

For example if I have this markdown file in my xx.docc folder

# Swift-DocC-Render

@Metadata {
  @TechnologyRoot
}

A web renderer for DocC Archives produced by DocC.

## Overview

Local image

![swift icon](swift.svg)

External image

![swift icon](https://www.swift.org/assets/images/swift.svg)

DocC-Render displays documentation produced by [DocC](https://www.swift.org/documentation/docc) on the web and is included in DocC Archives.

It will produce something like this in reference part

{
    "https://www.swift.org/documentation/docc": {
        "title": "DocC",
        "titleInlineContent": [
            {
                "type": "text",
                "text": "DocC"
            }
        ],
        "type": "link",
        "identifier": "https://www.swift.org/documentation/docc",
        "url": "https://www.swift.org/documentation/docc"
    },
    // MARK: - This is the current behavior. Without the PR, this item will not exist.
    "https://www.swift.org/assets/images/swift.svg": {
        "alt": "swift icon",
        "type": "externalImage",
        "identifier": "https://www.swift.org/assets/images/swift.svg",
        "variants": [
            {
                "url": "https://www.swift.org/assets/images/swift.svg",
                "traits": [
                    "1x",
                    "light"
                ]
            }
        ]
    },
    "swift.svg": {
        "alt": "swift icon",
        "type": "image",
        "identifier": "swift.svg",
        "variants": [
            {
                "url": "/images/swift.svg",
                "traits": [
                    "1x",
                    "light"
                ]
            }
        ]
    }
}

I think "d-ronnqvist" is suggesting me to generate an item like this without introducing a new externalImage type, right?

    "https://www.swift.org/assets/images/swift.svg": {
        "alt": "swift icon",
        "type": "image",
        "identifier": "https://www.swift.org/assets/images/swift.svg",
        "variants": [
            {
                // MARK: Problem here, since in current design, for image type it'll do some magical logic here, how can we identity this is actually an external image? Do we need introduce a new field or just use host == nil?
                "url": "https://www.swift.org/assets/images/swift.svg",
                "traits": [
                    "1x",
                    "light"
                ]
            }
        ]
    },

Kyle-Ye · 2022-03-01T16:28:16Z

Currently, I was stuck by the "display" and "download" context in DataAsset. Should externalImage a "display" context or "download" context? And what is "download" context mean in docc?

The current implementation will throw an error due to "copyAsset" API calling FileManager.copyItem(at:to:) where "at" is a https schema file. Will fix this once I figure what "download" context really means.
error: The file “swift.svg” couldn’t be opened because URL type https isn’t supported.

https://github.com/apple/swift-docc/blob/0d61dc5550c9bdb04fd1579144679e7d3287f640/Sources/SwiftDocCUtilities/Action/Actions/Convert/ConvertFileWritingConsumer.swift#L70-L101

Appreciate for your help and clarification @franklinsch

Kyle-Ye · 2022-03-01T16:34:04Z

The old (working but rejected) implementation is at https://github.com/Kyle-Ye/swift-docc/tree/external-image-support_v1
The new (WIP) implementation is at https://github.com/Kyle-Ye/swift-docc/tree/external-image-support_v2 (Which is currently in sync with external-image-support branch)

d-ronnqvist · 2022-03-02T05:05:38Z

I’ll look at this tomorrow

d-ronnqvist · 2022-03-03T20:53:38Z

Currently, I was stuck by the "display" and "download" context in DataAsset. Should externalImage a "display" context or "download" context? And what is "download" context mean in docc?

From what I recall, this is an indication to Swift-DocC Render that has some impact on how the request for that access is made. @mportiz08 May be able to provide more information about how this information is used.

A "download" context is used for zip archives and similar files that are downloaded but not displayed on the page and a "display" context is used for images and video that are displayed on the page. So in this case I would expect a "display" context.

mportiz08 · 2022-03-03T21:40:36Z

Currently, I was stuck by the "display" and "download" context in DataAsset. Should externalImage a "display" context or "download" context? And what is "download" context mean in docc?

From what I recall, this is an indication to Swift-DocC Render that has some impact on how the request for that access is made. @mportiz08 May be able to provide more information about how this information is used.

A "download" context is used for zip archives and similar files that are downloaded but not displayed on the page and a "display" context is used for images and video that are displayed on the page. So in this case I would expect a "display" context.

"display" sounds right to me. It seems that this enum is used to determine whether a particular asset should be turned into a media or download reference in the Render JSON: https://github.com/apple/swift-docc/blob/0661e969a2cd683ea67b0612dc3075a60fbcf6f8/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift#L1288

This will better indicate the type of reference and how it should be displayed or linked to for any renderers, including DocC-Render. The "download" value might be used for something like a zip file to download sample code, etc. Note that DocC-Render might support rendering content that DocC itself is not yet generating at the moment.

d-ronnqvist · 2022-03-03T23:00:35Z

The current implementation will throw an error due to "copyAsset" API calling FileManager.copyItem(at:to:) where "at" is a https schema file. Will fix this once I figure what "download" context really means.
error: The file “swift.svg” couldn’t be opened because URL type https isn’t supported.

I think this could work by skipping copying assets that aren't local. There are a few places that inspect the URL to determine if it's a local asset or an external asset so maybe that could be used here as well.

Kyle-Ye · 2022-03-05T03:52:22Z

This PR adds external media support in documentation(docs and articles) but not in tutorials.

Should we add support external media support for tutorials? And if we should maybe we can do it in a follow-up PR? cc @franklinsch

franklinsch · 2022-03-11T14:51:20Z

Supporting external images in tutorials in a separate PR sounds good to me. Please make sure to file a ticket for it on bugs.swift.org to make sure we don't lose track of it :)

d-ronnqvist

Thank you. This looks good to me.

I left a few comments and suggestions about some of the details.

Sources/SwiftDocCUtilities/Action/Actions/Convert/ConvertFileWritingConsumer.swift

Tests/SwiftDocCTests/Utility/URL+IsAbsoluteWebURLTests.swift

Sources/SwiftDocC/Utility/FoundationExtensions/URL+IsAbsoluteWebURL.swift

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

Kyle-Ye · 2022-03-19T16:07:36Z

@swift-ci please test

Kyle-Ye · 2022-03-19T16:16:18Z

Sources/SwiftDocC/Model/Rendering/References/ImageReference.swift

-            // If a scheme is present it means it's an absolute URL
-            let isAbsoluteWebURL = !value.isFileURL && value.scheme?.isEmpty == false
-            let url = isAbsoluteWebURL ? value : destinationURL(for: value.lastPathComponent)
+            let url = value.isAbsoluteWebURL ? value : destinationURL(for: value.lastPathComponent)


URL.isAbsoluteWebURL delete the host check, otherwise it will fail on some test due to the case like 'docc-media:///path/a.png'. We can fix it by not using the URL.isAbsoluteWebURL API here. And I think it's better to use one uniformed API, so I delete the host check in URL.isAbsoluteWebURL. Any suggestion is welcomed.

I agree that it's better to use the same API everywhere.

We can revisit the other tests' test data in the future to see if anything should be updated but I don't think that needs to happen now.

Kyle-Ye · 2022-03-19T16:18:44Z

Finally fix all the test cases. Ready for the final review. @d-ronnqvist

Kyle-Ye · 2022-03-19T16:26:30Z

Supporting external images in tutorials in a separate PR sounds good to me. Please make sure to file a ticket for it on bugs.swift.org to make sure we don't lose track of it :)

Update the original SR's title to "Add external media support for DocC documentation" and file up a new ticket to track the issue. https://bugs.swift.org/browse/SR-16026

d-ronnqvist

Thank you! This looks great to me.

d-ronnqvist · 2022-03-22T21:13:22Z

Sources/SwiftDocC/Model/Rendering/References/ImageReference.swift

-            // If a scheme is present it means it's an absolute URL
-            let isAbsoluteWebURL = !value.isFileURL && value.scheme?.isEmpty == false
-            let url = isAbsoluteWebURL ? value : destinationURL(for: value.lastPathComponent)
+            let url = value.isAbsoluteWebURL ? value : destinationURL(for: value.lastPathComponent)


I agree that it's better to use the same API everywhere.

We can revisit the other tests' test data in the future to see if anything should be updated but I don't think that needs to happen now.

Kyle-Ye · 2022-03-23T13:01:32Z

@swift-ci please test

Kyle-Ye marked this pull request as ready for review February 8, 2022 16:55

Kyle-Ye requested a review from franklinsch February 8, 2022 16:55

d-ronnqvist requested changes Feb 8, 2022

View reviewed changes

Sources/SwiftDocC/Model/Rendering/References/ExternalImageReference.swift Outdated Show resolved Hide resolved

Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift Outdated Show resolved Hide resolved

Kyle-Ye force-pushed the external-image-support branch from ef1c258 to 45c5e22 Compare March 1, 2022 16:22

Kyle-Ye marked this pull request as draft March 1, 2022 16:28

Kyle-Ye changed the title ~~Add external image support~~ [WIP] Add external image support Mar 1, 2022

Kyle-Ye marked this pull request as ready for review March 1, 2022 16:29

Kyle-Ye force-pushed the external-image-support branch from 45c5e22 to 6aecffc Compare March 5, 2022 03:36

Kyle-Ye changed the title ~~[WIP] Add external image support~~ Add external media support in documentation Mar 5, 2022

Kyle-Ye requested a review from d-ronnqvist March 5, 2022 03:49

d-ronnqvist reviewed Mar 11, 2022

View reviewed changes

Kyle-Ye force-pushed the external-image-support branch from 6aecffc to 3cceae5 Compare March 12, 2022 15:31

Kyle-Ye force-pushed the external-image-support branch from 3cceae5 to e3e02d4 Compare March 19, 2022 16:04

Kyle-Ye commented Mar 19, 2022

View reviewed changes

Kyle-Ye requested a review from d-ronnqvist March 21, 2022 02:57

d-ronnqvist approved these changes Mar 22, 2022

View reviewed changes

Kyle-Ye added 2 commits March 23, 2022 20:54

Add external image support in documentation

ec11962

Add test case for externalAsset resolve

1d2503a

Kyle-Ye force-pushed the external-image-support branch from e3e02d4 to 1d2503a Compare March 23, 2022 12:54

Kyle-Ye merged commit f0b80f0 into swiftlang:main Mar 23, 2022

Kyle-Ye deleted the external-image-support branch March 23, 2022 13:06

Kyle-Ye mentioned this pull request Feb 8, 2022

[SR-15839] Add external media support for DocC documentation #185

Closed

d-ronnqvist mentioned this pull request May 3, 2022

[SR-16026] Add external media support for DocC tutorial #172

Open

Kyle-Ye mentioned this pull request Aug 14, 2022

Document how to customize appearance of DocC docs #359

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external media support in documentation #82

Add external media support in documentation #82

Kyle-Ye commented Feb 8, 2022 •

edited

Loading

Kyle-Ye commented Feb 8, 2022 •

edited

Loading

d-ronnqvist left a comment

franklinsch commented Feb 28, 2022

Kyle-Ye commented Mar 1, 2022

Kyle-Ye commented Mar 1, 2022

Kyle-Ye commented Mar 1, 2022

d-ronnqvist commented Mar 2, 2022

d-ronnqvist commented Mar 3, 2022

mportiz08 commented Mar 3, 2022

d-ronnqvist commented Mar 3, 2022

Kyle-Ye commented Mar 5, 2022

franklinsch commented Mar 11, 2022

d-ronnqvist left a comment

Kyle-Ye commented Mar 19, 2022

Kyle-Ye Mar 19, 2022

d-ronnqvist Mar 22, 2022

Kyle-Ye commented Mar 19, 2022

Kyle-Ye commented Mar 19, 2022

d-ronnqvist left a comment

d-ronnqvist Mar 22, 2022

Kyle-Ye commented Mar 23, 2022

Add external media support in documentation #82

Add external media support in documentation #82

Conversation

Kyle-Ye commented Feb 8, 2022 • edited Loading

Summary

Dependencies

Testing

Checklist

TODO

Kyle-Ye commented Feb 8, 2022 • edited Loading

d-ronnqvist left a comment

Choose a reason for hiding this comment

franklinsch commented Feb 28, 2022

Kyle-Ye commented Mar 1, 2022

Kyle-Ye commented Mar 1, 2022

Kyle-Ye commented Mar 1, 2022

d-ronnqvist commented Mar 2, 2022

d-ronnqvist commented Mar 3, 2022

mportiz08 commented Mar 3, 2022

d-ronnqvist commented Mar 3, 2022

Kyle-Ye commented Mar 5, 2022

franklinsch commented Mar 11, 2022

d-ronnqvist left a comment

Choose a reason for hiding this comment

Kyle-Ye commented Mar 19, 2022

Kyle-Ye Mar 19, 2022

Choose a reason for hiding this comment

d-ronnqvist Mar 22, 2022

Choose a reason for hiding this comment

Kyle-Ye commented Mar 19, 2022

Kyle-Ye commented Mar 19, 2022

d-ronnqvist left a comment

Choose a reason for hiding this comment

d-ronnqvist Mar 22, 2022

Choose a reason for hiding this comment

Kyle-Ye commented Mar 23, 2022

Kyle-Ye commented Feb 8, 2022 •

edited

Loading

Kyle-Ye commented Feb 8, 2022 •

edited

Loading