From ac57633146edd97ab0ab67bb8f7572340fba46bc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 5 Sep 2023 15:33:50 +0100 Subject: [PATCH 01/14] Add tests for loading a markdown file with an anchor included Tests the problem reported in #3094 --- tests/test_markdownviewer.md | 15 +++++++++++++++ tests/test_markdownviewer.py | 23 +++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 tests/test_markdownviewer.md create mode 100644 tests/test_markdownviewer.py diff --git a/tests/test_markdownviewer.md b/tests/test_markdownviewer.md new file mode 100644 index 000000000..781603faf --- /dev/null +++ b/tests/test_markdownviewer.md @@ -0,0 +1,15 @@ +* [First](test_markdownviewer.md#first) +* [Second](test_markdownviewer.md#second) +* [Third](test_markdownviewer.md#third) + +# First + +The first. + +# Second + +The second. + +# Third + +The third. diff --git a/tests/test_markdownviewer.py b/tests/test_markdownviewer.py new file mode 100644 index 000000000..687aab2dd --- /dev/null +++ b/tests/test_markdownviewer.py @@ -0,0 +1,23 @@ +from pathlib import Path + +from textual.app import App, ComposeResult +from textual.geometry import Offset +from textual.widgets import Markdown, MarkdownViewer + + +class MarkdownViewerApp(App[None]): + def compose(self) -> ComposeResult: + yield MarkdownViewer() + + async def on_mount(self) -> None: + self.query_one(MarkdownViewer).show_table_of_contents = False + await self.query_one(MarkdownViewer).go(Path(__file__).with_suffix(".md")) + + +async def test_markdown_viewer_anchor_link() -> None: + """Test https://github.com/Textualize/textual/issues/3094""" + async with MarkdownViewerApp().run_test() as pilot: + # There's not really anything to test *for* here, but the lack of an + # exception is the win (before the fix this is testing it would have + # been FileNotFoundError). + await pilot.click(Markdown, Offset(2, 1)) From fa7f6b3066a2f15ff06a387a19d2a014c1d5045e Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 5 Sep 2023 15:35:24 +0100 Subject: [PATCH 02/14] Remove any anchor that's included in a filename to load from a Markdown file Fixes the issue reported in #3094. There's more to come on this, as rather than just fix that error, we'd also like to go to the header that the anchor relates to. See #3094 for an initial approach to this. This PR builds on the idea in a different way. But before doing that wider part, this simply starts out by fixing the reported bug. --- src/textual/widgets/_markdown.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index dafd64ee1..02c613822 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -628,6 +628,20 @@ class Markdown(Widget): if self._markdown is not None: self.update(self._markdown) + @staticmethod + def _sanitise_location(location: str) -> tuple[Path, str]: + """Given a location, extract and remove any anchor. + + Args: + location: The location to sanitise. + + Returns: + A tuple of the path to the location cleaned of any anchor, plus + the anchor (or an empty string if none was found). + """ + location, _, anchor = location.partition("#") + return Path(location), anchor + async def load(self, path: Path) -> None: """Load a new Markdown document. @@ -641,6 +655,7 @@ class Markdown(Widget): The exceptions that can be raised by this method are all of those that can be raised by calling [`Path.read_text`][pathlib.Path.read_text]. """ + path, _ = self._sanitise_location(str(path)) await self.update(path.read_text(encoding="utf-8")) def unhandled_token(self, token: Token) -> MarkdownBlock | None: From 7f40287691587cb63bfc74489bf86f1861286dc3 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 5 Sep 2023 16:20:05 +0100 Subject: [PATCH 03/14] Persist the table of contents in the Markdown widget --- src/textual/widgets/_markdown.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index 02c613822..8c4142bde 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -564,6 +564,7 @@ class Markdown(Widget): super().__init__(name=name, id=id, classes=classes) self._markdown = markdown self._parser_factory = parser_factory + self._table_of_contents: TableOfContentsType | None = None class TableOfContentsUpdated(Message): """The table of contents was updated.""" @@ -687,7 +688,7 @@ class Markdown(Widget): ) block_id: int = 0 - table_of_contents: TableOfContentsType = [] + self._table_of_contents = [] for token in parser.parse(markdown): if token.type == "heading_open": @@ -736,7 +737,7 @@ class Markdown(Widget): if token.type == "heading_close": heading = block._text.plain level = int(token.tag[1:]) - table_of_contents.append((level, heading, block.id)) + self._table_of_contents.append((level, heading, block.id)) if stack: stack[-1]._blocks.append(block) else: @@ -816,7 +817,9 @@ class Markdown(Widget): if external is not None: (stack[-1]._blocks if stack else output).append(external) - self.post_message(Markdown.TableOfContentsUpdated(self, table_of_contents)) + self.post_message( + Markdown.TableOfContentsUpdated(self, self._table_of_contents) + ) with self.app.batch_update(): self.query("MarkdownBlock").remove() return self.mount_all(output) From 798f67f01d501bdf760f6e97dea9f8ae3da9c396 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 09:27:22 +0100 Subject: [PATCH 04/14] Allow loading a different file into the markdown example Not getting carried away with this -- frogmouth exists after all -- but allowing passing a different file on the command line does make it easier to quickly test the Markdown widget. --- examples/markdown.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples/markdown.py b/examples/markdown.py index 0ade6718a..a6d26cb19 100644 --- a/examples/markdown.py +++ b/examples/markdown.py @@ -1,4 +1,5 @@ from pathlib import Path +from sys import argv from textual.app import App, ComposeResult from textual.reactive import var @@ -44,4 +45,6 @@ class MarkdownApp(App): if __name__ == "__main__": app = MarkdownApp() + if len(argv) > 1 and Path(argv[1]).exists(): + app.path = Path(argv[1]) app.run() From 738eb7b9b784365fe4e7734009fb2de85bec6dfc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 10:13:37 +0100 Subject: [PATCH 05/14] After loading a Markdown document, jump to any matching anchor Co-authored-by: Chakib Benziane --- src/textual/widgets/_markdown.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index 8c4142bde..56dd2c92e 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -12,6 +12,7 @@ from rich.table import Table from rich.text import Text from typing_extensions import TypeAlias +from .._slug import TrackedSlugs from ..app import ComposeResult from ..containers import Horizontal, Vertical, VerticalScroll from ..events import Mount @@ -643,6 +644,20 @@ class Markdown(Widget): location, _, anchor = location.partition("#") return Path(location), anchor + def _goto_anchor(self, anchor: str) -> None: + """Try and find the given anchor in the current document. + + Args: + anchor: The anchor to try and find. + """ + if not self._table_of_contents or not isinstance(self.parent, Widget): + return + unique = TrackedSlugs() + for _, title, header_id in self._table_of_contents: + if unique.slug(title) == anchor: + self.parent.scroll_to_widget(self.query_one(f"#{header_id}"), top=True) + return + async def load(self, path: Path) -> None: """Load a new Markdown document. @@ -656,8 +671,10 @@ class Markdown(Widget): The exceptions that can be raised by this method are all of those that can be raised by calling [`Path.read_text`][pathlib.Path.read_text]. """ - path, _ = self._sanitise_location(str(path)) + path, anchor = self._sanitise_location(str(path)) await self.update(path.read_text(encoding="utf-8")) + if anchor: + self._goto_anchor(anchor) def unhandled_token(self, token: Token) -> MarkdownBlock | None: """Process an unhandled token. From a70235e42d8d4579c7287e5b3515bde1ad31d45b Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 10:18:47 +0100 Subject: [PATCH 06/14] Update the CHANGELOG Co-authored-by: Chakib Benziane --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aeaecf207..4ff108323 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## Unreleased + +### Changed + +- `Markdown.load` will now attempt to scroll to a related heading if an anchor is provided [PR here] + +### Fixed + +- Fixed a crash in `MarkdownViewer` when clicking on a link that contains an anchor https://github.com/Textualize/textual/issues/3094 + ## [0.36.0] - 2023-09-05 ### Added From 284a5b973fb69adfdeb604e66e15b405f6aa2d53 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 13:19:56 +0100 Subject: [PATCH 07/14] Make goto_anchor public --- src/textual/widgets/_markdown.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index 56dd2c92e..a6cab09c3 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -644,11 +644,19 @@ class Markdown(Widget): location, _, anchor = location.partition("#") return Path(location), anchor - def _goto_anchor(self, anchor: str) -> None: + def goto_anchor(self, anchor: str) -> None: """Try and find the given anchor in the current document. Args: anchor: The anchor to try and find. + + Note: + The anchor is found by looking at all of the headings in the + document and finding the first one whose slug matches the + anchor. + + Note that the slugging method used is similar to that found on + GitHub. """ if not self._table_of_contents or not isinstance(self.parent, Widget): return @@ -674,7 +682,7 @@ class Markdown(Widget): path, anchor = self._sanitise_location(str(path)) await self.update(path.read_text(encoding="utf-8")) if anchor: - self._goto_anchor(anchor) + self.goto_anchor(anchor) def unhandled_token(self, token: Token) -> MarkdownBlock | None: """Process an unhandled token. From db2a5853d8beb6a08bdc6e2afc513146ed7d5233 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 13:29:52 +0100 Subject: [PATCH 08/14] Make sanitize_location a public API method --- src/textual/widgets/_markdown.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index a6cab09c3..bb46492c3 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -631,11 +631,11 @@ class Markdown(Widget): self.update(self._markdown) @staticmethod - def _sanitise_location(location: str) -> tuple[Path, str]: - """Given a location, extract and remove any anchor. + def sanitize_location(location: str) -> tuple[Path, str]: + """Given a location, break out the path and any anchor. Args: - location: The location to sanitise. + location: The location to sanitize. Returns: A tuple of the path to the location cleaned of any anchor, plus @@ -679,7 +679,7 @@ class Markdown(Widget): The exceptions that can be raised by this method are all of those that can be raised by calling [`Path.read_text`][pathlib.Path.read_text]. """ - path, anchor = self._sanitise_location(str(path)) + path, anchor = self.sanitize_location(str(path)) await self.update(path.read_text(encoding="utf-8")) if anchor: self.goto_anchor(anchor) From 1c1836e7f4397f1e5823e803bc61215c76d643a0 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 6 Sep 2023 14:06:13 +0100 Subject: [PATCH 09/14] Handle locations that are *just* the anchor If just an anchor is given, it is assumed that we'll be finding it within the current document. --- src/textual/widgets/_markdown.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index bb46492c3..01d29f7a8 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -51,6 +51,10 @@ class Navigator: Returns: New location. """ + location, anchor = Markdown.sanitize_location(str(path)) + if location == Path(".") and anchor: + current_file, _ = Markdown.sanitize_location(str(self.location)) + path = f"{current_file}#{anchor}" new_path = self.location.parent / Path(path) self.stack = self.stack[: self.index + 1] new_path = new_path.absolute() From 3b0e86bfef5018512a2dcd1ca7f36d1cf088dbde Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 7 Sep 2023 16:39:47 +0100 Subject: [PATCH 10/14] Add more testing of clicking on links --- tests/test_markdownviewer.md | 4 ++-- tests/test_markdownviewer.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_markdownviewer.md b/tests/test_markdownviewer.md index 781603faf..a611ad24e 100644 --- a/tests/test_markdownviewer.md +++ b/tests/test_markdownviewer.md @@ -1,6 +1,6 @@ * [First](test_markdownviewer.md#first) -* [Second](test_markdownviewer.md#second) -* [Third](test_markdownviewer.md#third) +* [Second](#second) +* [Third](./test_markdownviewer.md#third) # First diff --git a/tests/test_markdownviewer.py b/tests/test_markdownviewer.py index 687aab2dd..a002fbd5f 100644 --- a/tests/test_markdownviewer.py +++ b/tests/test_markdownviewer.py @@ -1,5 +1,7 @@ from pathlib import Path +import pytest + from textual.app import App, ComposeResult from textual.geometry import Offset from textual.widgets import Markdown, MarkdownViewer @@ -14,10 +16,11 @@ class MarkdownViewerApp(App[None]): await self.query_one(MarkdownViewer).go(Path(__file__).with_suffix(".md")) -async def test_markdown_viewer_anchor_link() -> None: +@pytest.mark.parametrize("link", [0, 1, 2]) +async def test_markdown_viewer_anchor_link(link: int) -> None: """Test https://github.com/Textualize/textual/issues/3094""" async with MarkdownViewerApp().run_test() as pilot: # There's not really anything to test *for* here, but the lack of an # exception is the win (before the fix this is testing it would have # been FileNotFoundError). - await pilot.click(Markdown, Offset(2, 1)) + await pilot.click(Markdown, Offset(2, link)) From ea832b8a9ae8935268b1d04fdeaac8669bb8512f Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 7 Sep 2023 16:43:09 +0100 Subject: [PATCH 11/14] Add tests for clicking on a link when markdown is from a string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timothée Mazzucotelli --- tests/test_markdownviewer.py | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/tests/test_markdownviewer.py b/tests/test_markdownviewer.py index a002fbd5f..19d6e47f0 100644 --- a/tests/test_markdownviewer.py +++ b/tests/test_markdownviewer.py @@ -7,7 +7,7 @@ from textual.geometry import Offset from textual.widgets import Markdown, MarkdownViewer -class MarkdownViewerApp(App[None]): +class MarkdownFileViewerApp(App[None]): def compose(self) -> ComposeResult: yield MarkdownViewer() @@ -17,9 +17,29 @@ class MarkdownViewerApp(App[None]): @pytest.mark.parametrize("link", [0, 1, 2]) -async def test_markdown_viewer_anchor_link(link: int) -> None: +async def test_markdown_file_viewer_anchor_link(link: int) -> None: """Test https://github.com/Textualize/textual/issues/3094""" - async with MarkdownViewerApp().run_test() as pilot: + async with MarkdownFileViewerApp().run_test() as pilot: + # There's not really anything to test *for* here, but the lack of an + # exception is the win (before the fix this is testing it would have + # been FileNotFoundError). + await pilot.click(Markdown, Offset(2, link)) + + +class MarkdownStringViewerApp(App[None]): + def compose(self) -> ComposeResult: + yield MarkdownViewer(Path(__file__).with_suffix(".md").read_text()) + + async def on_mount(self) -> None: + self.query_one(MarkdownViewer).show_table_of_contents = False + + +@pytest.mark.parametrize("link", [0, 1, 2]) +async def test_markdown_string_viewer_anchor_link(link: int) -> None: + """Test https://github.com/Textualize/textual/issues/3094 + + Also https://github.com/Textualize/textual/pull/3244#issuecomment-1710278718.""" + async with MarkdownStringViewerApp().run_test() as pilot: # There's not really anything to test *for* here, but the lack of an # exception is the win (before the fix this is testing it would have # been FileNotFoundError). From 4ac8df257402229e1547f6e71cd0a086ac36b0e5 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 7 Sep 2023 17:24:53 +0100 Subject: [PATCH 12/14] Improve the test for file and string markdown viewing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timothée Mazzucotelli --- tests/test_markdownviewer.md | 15 --------------- tests/test_markdownviewer.py | 30 ++++++++++++++++++++++++------ 2 files changed, 24 insertions(+), 21 deletions(-) delete mode 100644 tests/test_markdownviewer.md diff --git a/tests/test_markdownviewer.md b/tests/test_markdownviewer.md deleted file mode 100644 index a611ad24e..000000000 --- a/tests/test_markdownviewer.md +++ /dev/null @@ -1,15 +0,0 @@ -* [First](test_markdownviewer.md#first) -* [Second](#second) -* [Third](./test_markdownviewer.md#third) - -# First - -The first. - -# Second - -The second. - -# Third - -The third. diff --git a/tests/test_markdownviewer.py b/tests/test_markdownviewer.py index 19d6e47f0..27ccf0da9 100644 --- a/tests/test_markdownviewer.py +++ b/tests/test_markdownviewer.py @@ -6,20 +6,38 @@ from textual.app import App, ComposeResult from textual.geometry import Offset from textual.widgets import Markdown, MarkdownViewer +TEST_MARKDOWN = """\ +* [First]({{file}}#first) +* [Second](#second) + +# First + +The first. + +# Second + +The second. +""" + class MarkdownFileViewerApp(App[None]): + def __init__(self, markdown_file: Path) -> None: + super().__init__() + self.markdown_file = markdown_file + markdown_file.write_text(TEST_MARKDOWN.replace("{{file}}", markdown_file.name)) + def compose(self) -> ComposeResult: yield MarkdownViewer() async def on_mount(self) -> None: self.query_one(MarkdownViewer).show_table_of_contents = False - await self.query_one(MarkdownViewer).go(Path(__file__).with_suffix(".md")) + await self.query_one(MarkdownViewer).go(self.markdown_file) -@pytest.mark.parametrize("link", [0, 1, 2]) -async def test_markdown_file_viewer_anchor_link(link: int) -> None: +@pytest.mark.parametrize("link", [0, 1]) +async def test_markdown_file_viewer_anchor_link(tmp_path, link: int) -> None: """Test https://github.com/Textualize/textual/issues/3094""" - async with MarkdownFileViewerApp().run_test() as pilot: + async with MarkdownFileViewerApp(Path(tmp_path) / "test.md").run_test() as pilot: # There's not really anything to test *for* here, but the lack of an # exception is the win (before the fix this is testing it would have # been FileNotFoundError). @@ -28,13 +46,13 @@ async def test_markdown_file_viewer_anchor_link(link: int) -> None: class MarkdownStringViewerApp(App[None]): def compose(self) -> ComposeResult: - yield MarkdownViewer(Path(__file__).with_suffix(".md").read_text()) + yield MarkdownViewer(TEST_MARKDOWN.replace("{{file}}", "")) async def on_mount(self) -> None: self.query_one(MarkdownViewer).show_table_of_contents = False -@pytest.mark.parametrize("link", [0, 1, 2]) +@pytest.mark.parametrize("link", [0, 1]) async def test_markdown_string_viewer_anchor_link(link: int) -> None: """Test https://github.com/Textualize/textual/issues/3094 From 1d120d91fc272fe8c3af722029912d524e1d0885 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Fri, 8 Sep 2023 07:57:20 +0100 Subject: [PATCH 13/14] Fix being asked to go to an anchor with no filename given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timothée Mazzucotelli --- src/textual/widgets/_markdown.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/textual/widgets/_markdown.py b/src/textual/widgets/_markdown.py index 01d29f7a8..7c15be653 100644 --- a/src/textual/widgets/_markdown.py +++ b/src/textual/widgets/_markdown.py @@ -999,7 +999,13 @@ class MarkdownViewer(VerticalScroll, can_focus=True, can_focus_children=True): async def go(self, location: str | PurePath) -> None: """Navigate to a new document path.""" - await self.document.load(self.navigator.go(location)) + path, anchor = self.document.sanitize_location(str(location)) + if path == Path(".") and anchor: + # We've been asked to go to an anchor but with no file specified. + self.document.goto_anchor(anchor) + else: + # We've been asked to go to a file, optionally with an anchor. + await self.document.load(self.navigator.go(location)) async def back(self) -> None: """Go back one level in the history.""" From a83954e122ae6650eab3bb53296fcad929a6e888 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 12 Sep 2023 11:08:13 +0100 Subject: [PATCH 14/14] Actually link to the relevant PR --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0740ecc3..7070387e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Widget.notify and App.notify are now thread-safe https://github.com/Textualize/textual/pull/3275 - Breaking change: Widget.notify and App.notify now return None https://github.com/Textualize/textual/pull/3275 - App.unnotify is now private (renamed to App._unnotify) https://github.com/Textualize/textual/pull/3275 -- `Markdown.load` will now attempt to scroll to a related heading if an anchor is provided [PR here] +- `Markdown.load` will now attempt to scroll to a related heading if an anchor is provided https://github.com/Textualize/textual/pull/3244 ## [0.36.0] - 2023-09-05