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

Reconsider making MarkdownTableOfContents require a Markdown reference #2516

Closed
davep opened this issue May 8, 2023 · 8 comments
Closed
Labels
enhancement New feature or request question Further information is requested Task

Comments

@davep
Copy link
Contributor

davep commented May 8, 2023

a10d2d9 appears to have introduced a requirement for a Markdown reference to be passed to MarkdownTableOfContents. This is potentially problematic for any application that wants to compose the two widgets in very different parts of the DOM, such that neither of them will have knowledge of the other, and any communication between them is bridged by a parent.

Long story short: because Frogmouth decouples the two widgets and neither has knowledge of the other, instead having the owning screen handle the messages and bridge them across the DOM, the change broke Frogmouth and potentially adds unnecessary complexity to fix (at the moment I'm working around it with a throwaway Markdown instance).

If the new parameter is necessary, could it at least be optional?

@davep davep added enhancement New feature or request question Further information is requested Task labels May 8, 2023
davep added a commit to Textualize/frogmouth that referenced this issue May 8, 2023
@rodrigogiraoserrao
Copy link
Contributor

There's something I don't get, though: isn't a TOC inherently dependent on the document whose contents it's capturing?

@davep
Copy link
Contributor Author

davep commented May 9, 2023

It could technically display the TOC of one document with two viewers - an extreme case, I grant, but not impossible.

But also I feel this this makes it harder to compose an application where the ToC isn't DOM-adjacent to the document (see Frogmouth for example, where it is used as one of a number of external sources of input to the viewer).

I feel this would be a case where making the document itself at least entirely optional.

@willmcgugan
Copy link
Collaborator

This might be alleviated if we had some mechanism to pass messages that doesn't go through the DOM. Some kind of pub-sub system. Not sure what form that would take. Just floating the idea.

@davep
Copy link
Contributor Author

davep commented May 9, 2023

Good point.

I also forgot to add here (and the above reminds me) that the relationship between ToC and Markdown isn't hierarchical, as most other message-sending things are. Markdown sends a message on new ToC data but the ToC widget isn't higher in the DOM and never captures it directly. It needs a bridging container (see MarkdownViewer).

@rodrigogiraoserrao
Copy link
Contributor

Well, I do agree that something feels off, here...

relationship between ToC and Markdown isn't hierarchical, as most other message-sending things are.

The docs for compound widgets show that messages are also used like this. A child A posts a message to a parent widget P which then uses it to do something on a child B.
So, at least my adding the parameter to enable adding a control property to the message is wrong.
Maybe the overarching issue is that the message shouldn't float from the TOC to the Markdown via the parent "bridge".
To communicate down, the MarkdownViewer should use methods and reactives, right?
At least, that is what the docs suggest.

@davep
Copy link
Contributor Author

davep commented May 9, 2023

To communicate down, the MarkdownViewer should use methods and reactives, right?

  • Markdown has TableOfContentsUpdated, which lets anyone above know that the table of contents have changed.
  • MarkdownTableOfContents has a table_of_contents reactive that, when set, reconfigures its display.
  • Markdown has TableOfContentsSelected, which is used by MarkdownTableOfContents to let anyone above know that a selection has been made from the table of contents (the provenance of this message is questionable, it likely should be mark of MarkdownTableOfContents and perhaps called Selected, but it works as is).

I think, mostly, this is what we're already looking at. MarkdownViewer uses events to see what Markdown and MarkdownTableOfContents are both up to, and it uses reactives and methods to create an overall working viewer.

Frogmouth does pretty much the same, just with the viewer and the ToC even more separated and with the main screen handling the compound-widget-a-like conversation. The need to explicitly tell the ToC widget what viewer widget it's related to seems like a complication here.

But, like I've suggested above, I can see it potentially being useful as an optional thing to set, for some uses, but having it mandatory would seem to complicate others.

@willmcgugan
Copy link
Collaborator

Possibly worth doing. But low priority unless somebody asks for it. Closing for now.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

talblubClouby96 added a commit to talblubClouby96/frogmouth that referenced this issue Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested Task
Projects
None yet
Development

No branches or pull requests

3 participants