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

[Feature] Add svg and gif support for DocC image(SR-15311) #4

Merged
merged 5 commits into from
Oct 27, 2021
Merged

[Feature] Add svg and gif support for DocC image(SR-15311) #4

merged 5 commits into from
Oct 27, 2021

Conversation

Kyle-Ye
Copy link
Collaborator

@Kyle-Ye Kyle-Ye commented Oct 15, 2021

Feature/SR-15311

Summary

Swift-DocC currently ignores SVG and GIF files.

This PR add support for recognizing them as images in the same we do for JPEG, JPG, and PNG files.

Testing

Steps:

  1. Download sample project developer.apple.com/documentation/xcode/slothcreator_building_docc_documentation_in_xcode
  2. Download a svg file and a gif file and drag them to the Package's Sources/SlothCreator/SlothCreator.docc/Resources folder
  3. Replace the content in SlothCreator.tutorial
@Image(source: slothcreator-intro.png, alt: "An illustration of ... tutorials.")

to

@Image(source: xx.svg, alt: "An illustration of ... tutorials.")
@Image(source: xx.gif, alt: "An illustration of ... tutorials.")
  1. Build documentation to verify.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

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

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 15, 2021

Which one should we use for gif's MIME type? "image/gif" or "application/octet-stream"

"image/gif" in https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types

"application/octet-stream" in FileRequestHandlerTests.swift

// default font type
try verifyAsset(root: tempDir.url, path: "/fonts/test.ttf", body: "ttf", type: "application/octet-stream")
try verifyAsset(root: tempDir.url, path: "/images/image.png", body: "png", type: "image/png")
try verifyAsset(root: tempDir.url, path: "/images/image.gif", body: "gif", type: "application/octet-stream")
// default image type
try verifyAsset(root: tempDir.url, path: "/images/image.jpg", body: "jpg", type: "image/jpeg")
try verifyAsset(root: tempDir.url, path: "/images/logo.svg", body: "svg", type: "image/svg+xml")
try verifyAsset(root: tempDir.url, path: "/img/image.png", body: "png", type: "image/png")
try verifyAsset(root: tempDir.url, path: "/img/image.gif", body: "gif", type: "application/octet-stream")

@d-ronnqvist d-ronnqvist self-requested a review October 16, 2021 00:52
@d-ronnqvist
Copy link
Contributor

Which one should we use for gif's MIME type? "image/gif" or "application/octet-stream"

"image/gif" in https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types

"application/octet-stream" in FileRequestHandlerTests.swift

Thanks for opening this PR.

This test should verify that the MIME type is “image/gif”. The “application/octet-stream” value is a general binary data file or an unknown file type but “image/gif” is more precise here.

The rest of these changes look good to me.

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 16, 2021

Which one should we use for gif's MIME type? "image/gif" or "application/octet-stream"
"image/gif" in developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types
"application/octet-stream" in FileRequestHandlerTests.swift

Thanks for opening this PR.

This test should verify that the MIME type is “image/gif”. The “application/octet-stream” value is a general binary data file or an unknown file type but “image/gif” is more precise here.

The rest of these changes look good to me.

Change “application/octet-stream” to "image/gif" and pass the test. Any other suggestion?

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 16, 2021

@swift-ci Please smoke test

@d-ronnqvist
Copy link
Contributor

@swift-ci test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. I will merge this after the CI passes.

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 21, 2021

The Linux test fail seem to a Swift bug on the development branch.
The test machine is running on ubuntu1604 with 2021-10-18 snapshot
And I can reproduce it on my server on ubuntu2004 with 2021-10-18 snapshot
Also I rebased the branch to make the history more readable. Can you please test it again after the bug is fixed.

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 21, 2021

It seems to be a bug on Swift Complier on Linux. And the CI Test on swift-markdown is missing, so we did not find it early.

Also the 2021-10-05 snapshot of swift is working both on Linux and macOS.(For the success of building swift-markdown)

Bug created at https://bugs.swift.org/browse/SR-15362

@d-ronnqvist
Copy link
Contributor

@swift-ci test

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 22, 2021

The bug has been fixed on the main branch of Swift, but the latest snapshot has not updated yet.

Can we merge it as it pass the macOS Platform test or do we need wait until there is a new snapshot released and pass the Linux Platform test?

@franklinsch
Copy link
Member

A successful build on macOS and Linux are required to merge, so we'll wait for the fix for https://bugs.swift.org/browse/SR-15362 to land in a toolchain.

@d-ronnqvist d-ronnqvist self-assigned this Oct 22, 2021
@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 27, 2021

A successful build on macOS and Linux are required to merge, so we'll wait for the fix for bugs.swift.org/browse/SR-15362 to land in a toolchain.

The new toolchain is landed, could you please make the ci-bots to test again?

@franklinsch
Copy link
Member

@swift-ci please test

@franklinsch
Copy link
Member

@swift-ci please test

@d-ronnqvist
Copy link
Contributor

@swift-ci please test

1 similar comment
@d-ronnqvist
Copy link
Contributor

@swift-ci please test

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.

None yet

3 participants