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

Improve links behavior in Markdown files + rendering tweaks #1147

Merged
merged 12 commits into from
Nov 26, 2021

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Nov 13, 2021

This PR makes further improvements to the behavior of links especially in Markdown files, which enable a better user experience:

  • rewrite relative URLs in Markdown files so that URLs in links become absolute to github.com instead of raw.github.com (fabcdc9). This allows links to repository files to be opened directly in OctoDroid.
  • make relative URL rewriting aware of the folder of the current file, so that it works correctly for files that are not in the root of the repo. This makes navigating between MD files linked to each other (such as dotnet/runtime dev docs) a piece of cake.
  • the same URL rewriting logic is now used for both the "Markdown-in-TextView" Readme and for MD files rendered using WebViewerActivity.
  • link click behavior has been made consistent between Markdown-in-TextViews and Markdown-in-WebViews (72f48e4)
  • I've fixed a bug in URL rewriting that caused [Bug] markdown mailto links not working #1090 (274a885) and added explicit handling for mailto: links (the introduction of Improve links behavior #1118 made them unopenable)

Last but not least, I've made some minor improvements to Markdown-in-WebView styling: better paragraph spacing, fix for invisible separators and some more (4bc9f50). Here's a representative comparison (image was broken since the old URL rewriting in showdown.js didn't work with <img> tags in Markdown):

BeforeAfter

Fixes #1090
Closes #764


private static String rewriteUrlsInAttribute(String attribute, String html, String baseUrl) {
final Matcher matcher = Pattern.compile("(" + attribute + ")=\"(\\S+)\"").matcher(html);
final StringBuffer sb = new StringBuffer();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we have two iterations per rewrite now instead of one, I wonder whether it wouldn't make sense to check whether there's any match at all first, and if there isn't, return html right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you would have 3 iterations in case you have a match and still one in any case... I don't think there is a concrete performance penalty that justifies a preliminary check, which would still involve a full text scan.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then lazy init sb in the first 'found match' run and return html instead of sb if it's still null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then lazy init sb in the first 'found match' run and return html instead of sb if it's still null.

I didn't fully understand what you meant with this, but it seemed to me that the goal was to avoid the copy of the html string in the StringBuffer.
I did another thing instead: I've set the initial capacity of the StringBuffer to the length of html, to minimize the number of copies/reallocations when appending to the buffer.
I think that any other change that would make the logic more complex is simply not worth it. In the best case you would only save a few memory allocations.

app/src/main/java/com/gh4a/resolver/LinkClickHandler.java Outdated Show resolved Hide resolved
@Fs00
Copy link
Contributor Author

Fs00 commented Nov 20, 2021

I've made some more adjustments to Markdown-in-WebView rendering:

  • tables have now proper font size and collapsed borders
  • inline code snippets now have appropriate word wrapping so that they won't overflow + cosmetic improvements

Here's two comparisons:

BeforeAfter
BeforeAfter

@Fs00
Copy link
Contributor Author

Fs00 commented Nov 20, 2021

I've also found out that updating Showdown makes Markdown anchor links work in WebView!
May that be considered enough to fix #1027?

@maniac103 maniac103 merged commit 9981bcb into slapperwan:master Nov 26, 2021
@maniac103
Copy link
Collaborator

May that be considered enough to fix #1027?

With the example URL in #1027 this doesn't work for me, clicking on the anchor links in that readme does nothing.

@Fs00 Fs00 deleted the better-md-urls branch November 26, 2021 13:24
@Fs00
Copy link
Contributor Author

Fs00 commented Nov 26, 2021

With the example URL in #1027 this doesn't work for me, clicking on the anchor links in that readme does nothing.

Oh, I see why. The anchor links in that Readme have a user-content- prefix that works only on GitHub, due to how GH generates HTML for headings (which differs from Showdown.js). Here's an example:

<h2 dir="auto">
  <a id="user-content-frequently-requested-features" class="anchor" aria-hidden="true" href="#frequently-requested-features">[... omitted ...]</a>
  <a name="user-content-frequently-requested-features"></a>Frequently requested features
</h2>

The only way to fix this would be not to generate the HTML with Showdown.js, but instead retrieve it directly from GitHub API.

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.

[Bug] markdown mailto links not working GitHub emoji are not rendered in comment preview
2 participants