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

markdown_viewer: jump to anchor position when clicking on an anchor link #2941

Closed
wants to merge 7 commits into from

Conversation

blob42
Copy link
Contributor

@blob42 blob42 commented Jul 14, 2023

A little patch to handle links with anchors in MarkdownViewer and jumps to the anchor position after loading the new md file.

I need help for adding tests if they are necessary.

Rationale

I am trying to build an offline sphinx documentation that is generated into markdown output. With the current MarkdownViewer any link containing an anchor does not open the target file. This patch loads the target file and jumps to the anchor position.

@blob42 blob42 changed the title markdown_viewer: jump to anchor position when clicking on a link with anchor markdown_viewer: jump to anchor position when clicking on an anchor link Jul 14, 2023
@blob42 blob42 force-pushed the markdown_link_anchor branch 3 times, most recently from 45cc853 to 779bba4 Compare July 14, 2023 11:03
@blob42
Copy link
Contributor Author

blob42 commented Jul 18, 2023

Hi @davep if you have time to make a quick review for this PR that would be great.

@davep
Copy link
Contributor

davep commented Jul 18, 2023

Sure thing, when one of us gets the time to review we will. :-)

@davep
Copy link
Contributor

davep commented Aug 21, 2023

Just to let you know: haven't forgotten about this; just been deep in some longer-running stuff, but should look at sweeping this up with the other issue soon.

@blob42
Copy link
Contributor Author

blob42 commented Aug 21, 2023

Thanks for the update @davep it's not urgent anyway. Let me know if there is something I can do.

@davep davep linked an issue Aug 23, 2023 that may be closed by this pull request
@@ -125,3 +125,18 @@ async def test_load_non_existing_file() -> None:
await pilot.app.query_one(Markdown).load(
Path("---this-does-not-exist---.it.is.not.a.md")
)
async def test_anchor_links() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is looking good (I'm looking to pull this in, in part as a fix for #3094), but I don't think I understand what the test is trying to do here; especially given that it fails.

What is it aiming to test exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I committed by error I didn't know the right way to test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, I just wanted to check first. I'm happy to look at adding any appropriate testing. 🙂

@davep
Copy link
Contributor

davep commented Aug 24, 2023

@blob42 If you wish to make another change to this, could I ask a favour? Don't force push changes. I'd merged in main and tweaked a couple of things in anticipation of moving to merging this but the force push removed that work.

@davep
Copy link
Contributor

davep commented Aug 24, 2023

I'm going to do a little more work on this; it looks like it runs into a problem that I ran into with Frogmouth too, in that loading a document needs to take into account the original location of the document.

@blob42
Copy link
Contributor Author

blob42 commented Aug 24, 2023

@blob42 If you wish to make another change to this, could I ask a favour? Don't force push changes. I'd merged in main and tweaked a couple of things in anticipation of moving to merging this but the force push removed that work.

My bad sorry I didn't know. I hope you still had a local copy of the changes !

No changes to add on my side and I will never use force push again.

@davep
Copy link
Contributor

davep commented Aug 24, 2023

I'm going to do a little more work on this; it looks like it runs into a problem that I ran into with Frogmouth too, in that loading a document needs to take into account the original location of the document.

Turns out it's fine, I'm just bad at writing tests.

@davep davep self-assigned this Aug 24, 2023
@davep davep added bug Something isn't working Task labels Aug 24, 2023
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me.
I thought we could have hyperlinks in markdown documents and they'd work well, i.e., I could click them and open web pages.

I just tested it on the Textual README.md and it doesn't open URLs.

Since we're handling clicks in the PR, is this something that makes sense fixing here?

CHANGELOG.md Outdated Show resolved Hide resolved
tests/test_markdownviewer.py Show resolved Hide resolved
@davep
Copy link
Contributor

davep commented Aug 28, 2023

I thought we could have hyperlinks in markdown documents and they'd work well, i.e., I could click them and open web pages.

I just tested it on the Textual README.md and it doesn't open URLs.

Since we're handling clicks in the PR, is this something that makes sense fixing here?

I don't recall that ever being part of the Markdown viewer (it is part of Frogmouth though). I sense it's somewhat out of scope for this PR as this is mostly about handling a genuine crash. Might be worth adding an issue to the effect of the above though.

@blob42
Copy link
Contributor Author

blob42 commented Aug 29, 2023

The changes look reasonable to me. I thought we could have hyperlinks in markdown documents and they'd work well, i.e., I could click them and open web pages.

Since we're handling clicks in the PR, is this something that makes sense fixing here?

Keep in mind that most modern terminal emulators handle url clicks usually ctrl+click. They require the full link to be rendered to the terminal.

I don't know if Textual is planning to implement some modern terminal protocol like Kitty, which would solve all these issues.

Using Markdown from rich I can disable link rendering which allows me to use my native emulator url handler.

Maybe the same option could be added to the widget to disable rendering urls but would ignore relative links used for jumping around. It seems out of scope for this PR though.

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

A good first pass, but needs work...

What happens if the very first URL we visit has a fragment?

toc = self.table_of_contents.table_of_contents
assert toc is not None
for _, name, block_id in toc:
if name.lower().replace(" ", "-") == anchor:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is making a somewhat naive assumption about how fragments are generated from headers. It wont always be as simple as converting spaces to hyphens and lower casing. Consider characters with accents for example.

I think it makes sense to match what GitHub markdown does, and replicate that.

if message.href.find("#") == -1:
await self.go(message.href)
else:
link, anchor = message.href.split("#", 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest using str.partition here, so we can avoid the additional branch.

self.scroll_to_widget(block, top=True)
break

self.call_later(scroll_to_anchor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

call_later probably isn't the right approach here. It will execute after all other messages have been processed. This is normally very quick, but it is possible that an entirely new document has been loaded in the meantime.

I think this functionality should be built in to go, so we can be sure that it applies to the correct document.

link, anchor = message.href.split("#", 1)
await self.go(link)

def scroll_to_anchor() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a docstring. I know it seems self explanatory, but it's how we roll.

@davep
Copy link
Contributor

davep commented Aug 30, 2023

@blob42 Do you want to make the changes based on Will's feedback?

@blob42
Copy link
Contributor Author

blob42 commented Aug 30, 2023

@davep

I am a bit busy this week but will have time to properly work on it next week.

Feel free to do the changes if you would like.

@davep
Copy link
Contributor

davep commented Aug 30, 2023

@blob42 That's fine; not sure if I'll get back to this before next week either so let's say whoever gets back here first. :-)

@davep
Copy link
Contributor

davep commented Sep 4, 2023

Picking this up again.

@davep
Copy link
Contributor

davep commented Sep 5, 2023

#3236 is now merged into main, which should give a good basis for handling Markdown header slugs.

@davep
Copy link
Contributor

davep commented Sep 6, 2023

I've taken the core idea here and created #3244.

@davep
Copy link
Contributor

davep commented Sep 18, 2023

I think we hit this and some other stuff with #3244 so I'll close this one. Thanks again!

@davep davep closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants