Skip to content

Fix custom doc link title issue #376

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

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Sep 10, 2022

Bug/issue #, if applicable:

Close #271

Summary

## Overview

Hello

Check out [Hi](https://hi.com)

Check out [this great function](doc:Sloth/eat(_:quantity:))

Check out [this great function ![random image](https://picsum.photos/200)](doc:Sloth/eat(_:quantity:))

- <doc:Sloth/eat(_:quantity:)>

Before the PR (current behavior)
image

After the PR (expected behavior)
image

Dependencies

swiftlang/swift-markdown#82
swiftlang/swift-markdown#110

Testing

See RenderContentCompilerTests.swift

swiftlang/swift#61506

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 Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch 3 times, most recently from 31007e5 to a89cd8f Compare October 8, 2022 13:13
@Kyle-Ye Kyle-Ye marked this pull request as ready for review October 8, 2022 13:20
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 8, 2022

@swift-ci please test

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 9, 2022

Testing passed here with the Markdown change: swiftlang/swift#61506

Copy link
Contributor

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

This is looking really great, thank you @Kyle-Ye!

@franklinsch
Copy link
Contributor

@Kyle-Ye is there remaining work for this PR?

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 15, 2023

@Kyle-Ye is there remaining work for this PR?

The implementation is done. But there is some Code Review discussion to be processed.

Could you help give a look at the following thread👇

@franklinsch

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from bd06ddd to 4a7bf85 Compare February 23, 2023 15:45
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from 4a7bf85 to f7f1d9f Compare March 12, 2023 15:39
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from f7f1d9f to 7b80cf8 Compare March 13, 2023 12:20
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 13, 2023

There is still one thing wrong. The _isAutoLink property will get lost after visiting it and visiting back. This issue was identified in the failed test case, IndexingTests.testTutorial().

To address this problem, we can save and restore the original isAutolink status when changing a link's destination during a visit to a tutorial. However, this information will still be lost when revisiting that same tutorial. The reason for this is that the link will be generated from func makeMarkup(_ data: _MarkupData) -> Markup in Markup.child(at:), and it is not possible to retrieve the original isAutolink information at that point.

mutating func visitLink(_ link: Link) -> Markup? {
    ...
    let isAutolink = link.isAutolink // save the original isAutolink status
    var link = link
    link.destination = resolvedURL.absoluteString
    link.isAutolink = isAutolink // restore the original isAutolink status
    return link
}

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from 7b80cf8 to 37fe8bc Compare March 13, 2023 13:22
Comment on lines 172 to 191
return [
RenderInlineContent.reference(
identifier: .init(resolved.absoluteString),
isActive: true,
overridingTitle: link.isAutolink ? nil : overridingTitle,
overridingTitleInlineContent: link.isAutolink ? nil : overridingTitleInlineContent
)
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A possible solution to fix the failed test case and remove the need of Swift-Markdown's support for Markdown.Link._isAutolink property / Markdown.Link.isAutolink setter

Suggested change
return [
RenderInlineContent.reference(
identifier: .init(resolved.absoluteString),
isActive: true,
overridingTitle: link.isAutolink ? nil : overridingTitle,
overridingTitleInlineContent: link.isAutolink ? nil : overridingTitleInlineContent
)
]
let useOverriding: Bool
if link.isAutolink { // If the link is an auto link, we don't use overriding info
useOverriding = false
} else if let overridingTitle,
overridingTitle.hasPrefix(DocumentationSchemeHandler.scheme + ":"),
destination.hasPrefix(DocumentationSchemeHandler.fullScheme),
destination.hasSuffix(overridingTitle.dropFirst((DocumentationSchemeHandler.scheme + ":").count)) { // If the link is a transformed doc link, we don't use overriding info
useOverriding = false
} else {
useOverriding = true
}
return [
RenderInlineContent.reference(
identifier: .init(resolved.absoluteString),
isActive: true,
overridingTitle: useOverriding ? overridingTitle : nil,
overridingTitleInlineContent: useOverriding ? overridingTitleInlineContent : nil
)
]

@Kyle-Ye Kyle-Ye requested a review from franklinsch March 13, 2023 14:15
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from 5eb2bd7 to c2ad5b4 Compare March 13, 2023 17:04
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

This looks good, thanks!

@QuietMisdreavus
Copy link
Contributor

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor

Looks like the if let destination in Swift-Markdown caused an issue. I'll see about getting a PR up.

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from ce6d3a6 to 025bce5 Compare March 14, 2023 02:17
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 14, 2023

@swift-ci please test

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from 025bce5 to 420271d Compare March 14, 2023 02:40
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 14, 2023

@swift-ci please test

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 14, 2023

Looks like the if let destination in Swift-Markdown caused an issue. I'll see about getting a PR up.

The Swift-Markdown is fixed and the test passed on Swift-DocC. Are we ready to merge this? @QuietMisdreavus

Kyle-Ye and others added 3 commits March 15, 2023 14:13
Refactor link rendering to use overriding titles only when appropriate
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/custom-link-titles branch from 420271d to 9399211 Compare March 15, 2023 06:13
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Mar 15, 2023

@swift-ci please test

@Kyle-Ye Kyle-Ye merged commit 4233786 into swiftlang:main Mar 15, 2023
@Kyle-Ye Kyle-Ye deleted the bugfix/kyle/custom-link-titles branch March 15, 2023 11:59
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.

Custom link titles authored with []() syntax for doc links are silently ignored
4 participants