From a89ea78a7a54caa26d2882f12cdb7e086e4f937f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 22 Oct 2019 00:34:33 +0200 Subject: [PATCH 1/6] Add extended testing for Markdown links While reviewing #6542 it became apparent that there was another Markdown link format variant, used in comments that came from the web API, called the "inline link" style. It allows to specify the tooltip title within the actual URL portion, as such: [link text](https://osu.ppy.sh "tooltip text") Add tests with a couple of easy and trickier examples of such a format. Moreover, add a new edge case of a Markdown link with a link inside the display text, which during tests was detected to be problematic. --- osu.Game.Tests/Chat/MessageFormatterTests.cs | 48 ++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/osu.Game.Tests/Chat/MessageFormatterTests.cs b/osu.Game.Tests/Chat/MessageFormatterTests.cs index 9b4a90e9a9..1cbd083f0d 100644 --- a/osu.Game.Tests/Chat/MessageFormatterTests.cs +++ b/osu.Game.Tests/Chat/MessageFormatterTests.cs @@ -273,6 +273,54 @@ namespace osu.Game.Tests.Chat Assert.AreEqual(21, result.Links[0].Length); } + [Test] + public void TestMarkdownFormatLinkWithInlineTitle() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "I haven't seen [this link format](https://osu.ppy.sh \"osu!\") before..." }); + + Assert.AreEqual("I haven't seen this link format before...", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://osu.ppy.sh", result.Links[0].Url); + Assert.AreEqual(15, result.Links[0].Index); + Assert.AreEqual(16, result.Links[0].Length); + } + + [Test] + public void TestMarkdownFormatLinkWithInlineTitleAndEscapedQuotes() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "I haven't seen [this link format](https://osu.ppy.sh \"inner quote \\\" just to confuse \") before..." }); + + Assert.AreEqual("I haven't seen this link format before...", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://osu.ppy.sh", result.Links[0].Url); + Assert.AreEqual(15, result.Links[0].Index); + Assert.AreEqual(16, result.Links[0].Length); + } + + [Test] + public void TestMarkdownFormatLinkWithUrlInTextAndInlineTitle() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "I haven't seen [https://osu.ppy.sh](https://osu.ppy.sh \"https://osu.ppy.sh\") before..." }); + + Assert.AreEqual("I haven't seen https://osu.ppy.sh before...", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://osu.ppy.sh", result.Links[0].Url); + Assert.AreEqual(15, result.Links[0].Index); + Assert.AreEqual(18, result.Links[0].Length); + } + + [Test] + public void TestMarkdownFormatLinkWithMisleadingUrlInText() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "I haven't seen [https://google.com](https://osu.ppy.sh) before..." }); + + Assert.AreEqual("I haven't seen https://google.com before...", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://osu.ppy.sh", result.Links[0].Url); + Assert.AreEqual(15, result.Links[0].Index); + Assert.AreEqual(18, result.Links[0].Length); + } + [Test] public void TestChannelLink() { From 24b71605227c247a37937dacafeaf2d600007760 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 22 Oct 2019 00:40:58 +0200 Subject: [PATCH 2/6] Add support for parsing Markdown inline links Extend the Markdown parsing regex to allow parsing so-called inline links. Within the parenthesis () part of the Markdown URL syntax, introduce a new capturing group: ( \s+ // whitespace between actual URL and inline title (? // start of "title" named group "" // opening double quote (doubled inside @ string) ( [^""] // any character but a double quote | // or (?<=\\) // the next character should be preceded by a \ "" // a double quote )* // zero or more times "" // closing double quote ) )? // the whole group is optional This allows for parsing the inline links as-provided by web. Correctness is displayed by the passing tests. --- osu.Game/Online/Chat/MessageFormatter.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/Chat/MessageFormatter.cs b/osu.Game/Online/Chat/MessageFormatter.cs index 24d17612ee..749d24ee75 100644 --- a/osu.Game/Online/Chat/MessageFormatter.cs +++ b/osu.Game/Online/Chat/MessageFormatter.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -20,7 +20,7 @@ namespace osu.Game.Online.Chat private static readonly Regex new_link_regex = new Regex(@"\[(?<url>[a-z]+://[^ ]+) (?<text>(((?<=\\)[\[\]])|[^\[\]])*(((?<open>\[)(((?<=\\)[\[\]])|[^\[\]])*)+((?<close-open>\])(((?<=\\)[\[\]])|[^\[\]])*)+)*(?(open)(?!)))\]"); // [test](https://osu.ppy.sh/b/1234) -> test (https://osu.ppy.sh/b/1234) aka correct markdown format - private static readonly Regex markdown_link_regex = new Regex(@"\[(?<text>(((?<=\\)[\[\]])|[^\[\]])*(((?<open>\[)(((?<=\\)[\[\]])|[^\[\]])*)+((?<close-open>\])(((?<=\\)[\[\]])|[^\[\]])*)+)*(?(open)(?!)))\]\((?<url>[a-z]+://[^ ]+)\)"); + private static readonly Regex markdown_link_regex = new Regex(@"\[(?<text>(((?<=\\)[\[\]])|[^\[\]])*(((?<open>\[)(((?<=\\)[\[\]])|[^\[\]])*)+((?<close-open>\])(((?<=\\)[\[\]])|[^\[\]])*)+)*(?(open)(?!)))\]\((?<url>[a-z]+://[^ ]+)(\s+(?<title>""([^""]|(?<=\\)"")*""))?\)"); // advanced, RFC-compatible regular expression that matches any possible URL, *but* allows certain invalid characters that are widely used // This is in the format (<required>, [optional]): From cbd99cc767085bf79f17682e2ffe2845330d39f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= <dach.bartlomiej@gmail.com> Date: Tue, 22 Oct 2019 01:01:37 +0200 Subject: [PATCH 3/6] Resolve link-in-link edge case Testing with #6542 surfaced a crash scenario, caused by formatted links that had URLs in the display text, for example [mean example - https://osu.ppy.sh](https://osu.ppy.sh) In that case the outer Markdown link would get picked up once, and then reduced to the link text when looking for other links, leading to it being picked up again the second time when the raw link is found. Add a check in the raw link parsing path that ensures that the found URL is not a part of a bigger, pre-existing link. --- osu.Game.Tests/Chat/MessageFormatterTests.cs | 12 ++++++++++++ osu.Game/Online/Chat/MessageFormatter.cs | 18 +++++++++++++----- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Chat/MessageFormatterTests.cs b/osu.Game.Tests/Chat/MessageFormatterTests.cs index 1cbd083f0d..7988c6b96d 100644 --- a/osu.Game.Tests/Chat/MessageFormatterTests.cs +++ b/osu.Game.Tests/Chat/MessageFormatterTests.cs @@ -309,6 +309,18 @@ namespace osu.Game.Tests.Chat Assert.AreEqual(18, result.Links[0].Length); } + [Test] + public void TestMarkdownFormatLinkWithUrlAndTextInTitle() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "I haven't seen [oh no, text here! https://osu.ppy.sh](https://osu.ppy.sh) before..." }); + + Assert.AreEqual("I haven't seen oh no, text here! https://osu.ppy.sh before...", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://osu.ppy.sh", result.Links[0].Url); + Assert.AreEqual(15, result.Links[0].Index); + Assert.AreEqual(36, result.Links[0].Length); + } + [Test] public void TestMarkdownFormatLinkWithMisleadingUrlInText() { diff --git a/osu.Game/Online/Chat/MessageFormatter.cs b/osu.Game/Online/Chat/MessageFormatter.cs index 749d24ee75..d77920c97d 100644 --- a/osu.Game/Online/Chat/MessageFormatter.cs +++ b/osu.Game/Online/Chat/MessageFormatter.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -95,11 +95,17 @@ namespace osu.Game.Online.Chat foreach (Match m in regex.Matches(result.Text, startIndex)) { var index = m.Index; - var link = m.Groups["link"].Value; - var indexLength = link.Length; + var linkText = m.Groups["link"].Value; + var indexLength = linkText.Length; - var details = getLinkDetails(link); - result.Links.Add(new Link(link, index, indexLength, details.Action, details.Argument)); + var details = getLinkDetails(linkText); + var link = new Link(linkText, index, indexLength, details.Action, details.Argument); + + // sometimes an already-processed formatted link can reduce to a simple URL, too + // (example: [mean example - https://osu.ppy.sh](https://osu.ppy.sh)) + // therefore we need to check if any of the pre-existing links contains the raw one we found + if (result.Links.All(existingLink => !existingLink.Contains(link))) + result.Links.Add(link); } } @@ -292,6 +298,8 @@ namespace osu.Game.Online.Chat Argument = argument; } + public bool Contains(Link otherLink) => otherLink.Index >= Index && otherLink.Index + otherLink.Length <= Index + Length; + public int CompareTo(Link otherLink) => Index > otherLink.Index ? 1 : -1; } } From 661dfbefaf55e0c9b238d86274e792ed06b3df61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= <dach.bartlomiej@gmail.com> Date: Fri, 25 Oct 2019 00:20:44 +0200 Subject: [PATCH 4/6] Change containment check to overlap Due to scenarios wherein a formatted link ended up as part of a larger raw link after parsing, change the containment check to an overlap check and add appropriate tests for these edge cases. --- osu.Game.Tests/Chat/MessageFormatterTests.cs | 30 ++++++++++++++++++++ osu.Game/Online/Chat/MessageFormatter.cs | 4 +-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Chat/MessageFormatterTests.cs b/osu.Game.Tests/Chat/MessageFormatterTests.cs index 7988c6b96d..fbb0416c45 100644 --- a/osu.Game.Tests/Chat/MessageFormatterTests.cs +++ b/osu.Game.Tests/Chat/MessageFormatterTests.cs @@ -333,6 +333,36 @@ namespace osu.Game.Tests.Chat Assert.AreEqual(18, result.Links[0].Length); } + [Test] + public void TestMarkdownFormatLinkThatContractsIntoLargerLink() + { + Message result = MessageFormatter.FormatMessage(new Message { Content = "super broken https://[osu.ppy](https://reddit.com).sh/" }); + + Assert.AreEqual("super broken https://osu.ppy.sh/", result.DisplayContent); + Assert.AreEqual(1, result.Links.Count); + Assert.AreEqual("https://reddit.com", result.Links[0].Url); + Assert.AreEqual(21, result.Links[0].Index); + Assert.AreEqual(7, result.Links[0].Length); + } + + [Test] + public void TestMarkdownFormatLinkDirectlyNextToRawLink() + { + // the raw link has a port at the end of it, so that the raw link regex terminates at the port and doesn't consume display text from the formatted one + Message result = MessageFormatter.FormatMessage(new Message { Content = "https://localhost:8080[https://osu.ppy.sh](https://osu.ppy.sh) should be two links" }); + + Assert.AreEqual("https://localhost:8080https://osu.ppy.sh should be two links", result.DisplayContent); + Assert.AreEqual(2, result.Links.Count); + + Assert.AreEqual("https://localhost:8080", result.Links[0].Url); + Assert.AreEqual(0, result.Links[0].Index); + Assert.AreEqual(22, result.Links[0].Length); + + Assert.AreEqual("https://osu.ppy.sh", result.Links[1].Url); + Assert.AreEqual(22, result.Links[1].Index); + Assert.AreEqual(18, result.Links[1].Length); + } + [Test] public void TestChannelLink() { diff --git a/osu.Game/Online/Chat/MessageFormatter.cs b/osu.Game/Online/Chat/MessageFormatter.cs index d77920c97d..3ffff281f8 100644 --- a/osu.Game/Online/Chat/MessageFormatter.cs +++ b/osu.Game/Online/Chat/MessageFormatter.cs @@ -104,7 +104,7 @@ namespace osu.Game.Online.Chat // sometimes an already-processed formatted link can reduce to a simple URL, too // (example: [mean example - https://osu.ppy.sh](https://osu.ppy.sh)) // therefore we need to check if any of the pre-existing links contains the raw one we found - if (result.Links.All(existingLink => !existingLink.Contains(link))) + if (result.Links.All(existingLink => !existingLink.Overlaps(link))) result.Links.Add(link); } } @@ -298,7 +298,7 @@ namespace osu.Game.Online.Chat Argument = argument; } - public bool Contains(Link otherLink) => otherLink.Index >= Index && otherLink.Index + otherLink.Length <= Index + Length; + public bool Overlaps(Link otherLink) => Index < otherLink.Index + otherLink.Length && otherLink.Index < Index + Length; public int CompareTo(Link otherLink) => Index > otherLink.Index ? 1 : -1; } From 07f7944fc61f2fb907385cf4e9de4b2b7e575482 Mon Sep 17 00:00:00 2001 From: Dean Herbert <pe@ppy.sh> Date: Fri, 25 Oct 2019 12:22:19 +0900 Subject: [PATCH 5/6] Fix DateTime display sizing on results screen --- .../Screens/Ranking/Pages/ScoreResultsPage.cs | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs b/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs index 7c35742ff6..af47003682 100644 --- a/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs +++ b/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs @@ -253,9 +253,7 @@ namespace osu.Game.Screens.Ranking.Pages { this.date = date; - AutoSizeAxes = Axes.Y; - - Width = 140; + AutoSizeAxes = Axes.Both; Masking = true; CornerRadius = 5; @@ -271,22 +269,26 @@ namespace osu.Game.Screens.Ranking.Pages RelativeSizeAxes = Axes.Both, Colour = colours.Gray6, }, - new OsuSpriteText + new FillFlowContainer { - Origin = Anchor.CentreLeft, - Anchor = Anchor.CentreLeft, - Text = date.ToShortDateString(), + AutoSizeAxes = Axes.Both, + Direction = FillDirection.Horizontal, Padding = new MarginPadding { Horizontal = 10, Vertical = 5 }, - Colour = Color4.White, + Spacing = new Vector2(5), + Children = new[] + { + new OsuSpriteText + { + Text = date.ToShortDateString(), + Colour = Color4.White, + }, + new OsuSpriteText + { + Text = date.ToShortTimeString(), + Colour = Color4.White, + } + } }, - new OsuSpriteText - { - Origin = Anchor.CentreRight, - Anchor = Anchor.CentreRight, - Text = date.ToShortTimeString(), - Padding = new MarginPadding { Horizontal = 10, Vertical = 5 }, - Colour = Color4.White, - } }; } } From e5b5d286fd4aa21cc077bbc99a9892fe0d2deec1 Mon Sep 17 00:00:00 2001 From: Dean Herbert <pe@ppy.sh> Date: Fri, 25 Oct 2019 12:48:34 +0900 Subject: [PATCH 6/6] Increase spacing to closer match the design --- osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs b/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs index af47003682..56ae069a26 100644 --- a/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs +++ b/osu.Game/Screens/Ranking/Pages/ScoreResultsPage.cs @@ -274,7 +274,7 @@ namespace osu.Game.Screens.Ranking.Pages AutoSizeAxes = Axes.Both, Direction = FillDirection.Horizontal, Padding = new MarginPadding { Horizontal = 10, Vertical = 5 }, - Spacing = new Vector2(5), + Spacing = new Vector2(10), Children = new[] { new OsuSpriteText