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

WebHost/Core/Lingo: Render option documentation as reStructuredText in the WebView #3511

Merged
merged 12 commits into from
Jun 14, 2024

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jun 11, 2024

What is this fixing or adding?

This allows Options to use the standard Python documentation format,
while producing much nicer-looking documentation in the WebView with
things like emphasis, lists, and so on.

While this behavior is on by default for new worlds, this also (at
@NewSoupVi's suggestion) adds a list of worlds that are opted into the
old "plain text" behavior. World authors can remove their worlds from
this list and update their Option documentation at their leisure.

HTML Tooltips

This also updates the tooltip infrastructure to support full HTML
tooltips, structured like so:

<div class="tooltip-container">
  (?)
  <div class="tooltip">Tooltip contents</div>
</div>

Plain text tooltips continue to work using the data-tooltip
attribute.

How was this tested?

I updated the Lingo documentation to use the new behavior, and
verified that other worlds' output doesn't change.

If this makes graphical changes, please attach screenshots.

Old look:

image

New look:

image

nex3 added 3 commits June 10, 2024 21:08
This means that options can use the standard Python documentation
format, while producing much nicer-looking documentation in the
WebView with things like emphasis, lists, and so on.
This avoids breaking the rendering of existing option docs which were
written with the old plain text rendering in mind, while also allowing
new options to default to the rich text rendering instead.
@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 11, 2024
@ThePhar ThePhar added the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Jun 11, 2024
@ThePhar ThePhar changed the title Render option documentation as reStructuredText in the WebView WebHost/Core/Lingo: Render option documentation as reStructuredText in the WebView Jun 11, 2024
@ThePhar ThePhar added is: documentation Improvements or additions to documentation. is: enhancement Issues requesting new features or pull requests implementing new features. labels Jun 11, 2024
@LegendaryLinux LegendaryLinux self-assigned this Jun 11, 2024
# These worlds still use the old-style plain text Option docstrings. All other
# worlds' Option docstrings are parsed as reStructuredText (the standard Python
# docstring format) and rendered as HTML in the WebHost.
for world_name in [
Copy link
Member

Choose a reason for hiding this comment

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

This breaks APWorlds. Please don't do this. Let the world opt in.

Copy link
Member

Choose a reason for hiding this comment

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

Imo breaking something purely visual for an apworld during AP's 0.# days, especially when apworld don't usually worry about being displayed on WebHost (cause how would they), needs to be okay

Copy link
Member

@NewSoupVi NewSoupVi Jun 11, 2024

Choose a reason for hiding this comment

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

However, with Phar's alternate suggestion of how to implement this, I'm now on board with this being opt in. Just saying right now that I disagree with your philisophy in general and it will probably come up again in the future :P

Copy link
Member

Choose a reason for hiding this comment

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

Considering the long-term goal is decoupling worlds from core, I'm against adding anything that adds special privileges to worlds currently in the core repo (it also can't be edited easily from outside the apworld, if one of these world authors releases an updated apworld that would get placed in a webhost).

So regardless of what is decided (breaking existing APworlds or not), I am firmly against this file existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To give some background here, this was intended as a combined solution for two problems:

  1. This change shouldn't break the rendering for existing core worlds.
  2. It's infeasible to update existing core worlds in-place because doing so would require a review from every world maintainer.

APWorlds could still easily keep the old behavior by adding plain_text_doc = True to their options (or plain_text_options_doc = True to their World). The idea isn't so much to give special privileges to core worlds as to work around how difficult it would be to treat them the same as APWorlds.

All that said, it sounds like the consensus is that the core maintainers prefer this to be opt-out. I worry that's not the right call in the long-term when plain text option docs provide no benefits other than backwards-compatibility, but I'll bow to your decision and make the change.

Comment on lines 79 to 81
return publish_parts(text, writer_name='html', settings=None, settings_overrides={
'output_encoding': 'unicode'
})['body']
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to avoid raw html for several reasons

text = """.. raw:: html

    <script>alert('pwned')</script>"""
publish_parts(text, writer_name="html", settings=None, settings_overrides={"output_encoding": "unicode"})["body"]

returns "<script>alert('pwned')</script>"

Security may be a concern, since the option export could be "safe" (i.e. jsonworlds), but even if it wasn't, we don't want to have people put raw html there to work around issues instead of fixing them in core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fortunately this is an easy fix 🙂

@ScipioWright ScipioWright added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jun 11, 2024
Options.py Outdated
@@ -126,6 +126,23 @@ class Option(typing.Generic[T], metaclass=AssembleOptions):
# can be weighted between selections
supports_weighting = True

plain_text_doc = False
Copy link
Member

Choose a reason for hiding this comment

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

Rather than attach this to the Option class itself, wouldn't it make more sense to make this an attribute on WebWorld itself that applies to all options?

I don't see a reason why anyone migrating to reST formatting wouldn't just update all their doccomments, and depending on which becomes default (plain text should be default imo), if a dev decides to use the other, they'll have to set it on every option class.

Copy link
Member

@NewSoupVi NewSoupVi Jun 11, 2024

Choose a reason for hiding this comment

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

This is such a better suggestion than what I've been suggesting holy damn

Yeah I think I'm now on board with this over my initial suggestion, and at that point, I think it's much better if this feature is opt-in than opt-out

worlds/lingo/options.py Show resolved Hide resolved
Options.py Outdated Show resolved Hide resolved
@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Jun 11, 2024
Copy link
Collaborator

@hatkirby hatkirby left a comment

Choose a reason for hiding this comment

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

Overall looks great from the Lingo side! I found two minor typos but otherwise I think this is a good change.

worlds/lingo/options.py Outdated Show resolved Hide resolved
worlds/lingo/options.py Outdated Show resolved Hide resolved
@ScipioWright ScipioWright removed the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Jun 11, 2024
@NewSoupVi
Copy link
Member

NewSoupVi commented Jun 11, 2024

I am now more on board with Phar's (honestly way better) suggestion of making this an attribute of WebWorld, and at that point, it's so easy to opt into that I'm in agreement with Black Sliver that it should be opt-in.

@ThePhar
Copy link
Member

ThePhar commented Jun 11, 2024

Oh, and if it wasn't mentioned elsewhere already, whatever the final API is proposed to look like, should also be documented in the world api documentation as well.

@nex3 nex3 force-pushed the fancy-option-docs branch from a5305df to daea045 Compare June 12, 2024 03:43
@nex3 nex3 requested a review from hatkirby June 12, 2024 03:49
docs/options api.md Outdated Show resolved Hide resolved
@LegendaryLinux
Copy link
Member

I'm also on board with the opt-in method via WebWorld. I'll give the code some closer scrutiny tonight.

docs/options api.md Outdated Show resolved Hide resolved
@LegendaryLinux
Copy link
Member

LegendaryLinux commented Jun 13, 2024

I went ahead and looked at the code and it looks good, save for the documentation reference. I'll double check the styles later tonight since I'm not at my desktop computer right now.

@LegendaryLinux
Copy link
Member

LegendaryLinux commented Jun 13, 2024

Okay, this is approved pending the update to the documentation.
I did cause a small merge conflict by merging #3513, but that should be easy to fix.

docs/options api.md Outdated Show resolved Hide resolved
Options.py Outdated Show resolved Hide resolved
LegendaryLinux and others added 2 commits June 14, 2024 18:51
Co-authored-by: Doug Hoskisson <[email protected]>
Co-authored-by: Doug Hoskisson <[email protected]>
@LegendaryLinux LegendaryLinux merged commit c61505b into ArchipelagoMW:main Jun 14, 2024
14 checks passed
@nex3 nex3 deleted the fancy-option-docs branch June 14, 2024 23:02
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
…n the WebView (ArchipelagoMW#3511)

* Render option documentation as reStructuredText in the WebView

This means that options can use the standard Python documentation
format, while producing much nicer-looking documentation in the
WebView with things like emphasis, lists, and so on.

* Opt existing worlds out of rich option docs

This avoids breaking the rendering of existing option docs which were
written with the old plain text rendering in mind, while also allowing
new options to default to the rich text rendering instead.

* Use reStructuredText formatting for Lingo Options docstrings

* Disable raw and file insertion RST directives

* Update doc comments per code review

* Make rich text docs opt-in

* Put rich_text_options_doc on WebWorld

* Document rich text API

* Code review

* Update docs/options api.md

Co-authored-by: Doug Hoskisson <[email protected]>

* Update Options.py

Co-authored-by: Doug Hoskisson <[email protected]>

---------

Co-authored-by: Chris Wilson <[email protected]>
Co-authored-by: Doug Hoskisson <[email protected]>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…n the WebView (ArchipelagoMW#3511)

* Render option documentation as reStructuredText in the WebView

This means that options can use the standard Python documentation
format, while producing much nicer-looking documentation in the
WebView with things like emphasis, lists, and so on.

* Opt existing worlds out of rich option docs

This avoids breaking the rendering of existing option docs which were
written with the old plain text rendering in mind, while also allowing
new options to default to the rich text rendering instead.

* Use reStructuredText formatting for Lingo Options docstrings

* Disable raw and file insertion RST directives

* Update doc comments per code review

* Make rich text docs opt-in

* Put rich_text_options_doc on WebWorld

* Document rich text API

* Code review

* Update docs/options api.md

Co-authored-by: Doug Hoskisson <[email protected]>

* Update Options.py

Co-authored-by: Doug Hoskisson <[email protected]>

---------

Co-authored-by: Chris Wilson <[email protected]>
Co-authored-by: Doug Hoskisson <[email protected]>
GameWyrm pushed a commit to GameWyrm/Archipelago-GW that referenced this pull request Jul 4, 2024
…n the WebView (ArchipelagoMW#3511)

* Render option documentation as reStructuredText in the WebView

This means that options can use the standard Python documentation
format, while producing much nicer-looking documentation in the
WebView with things like emphasis, lists, and so on.

* Opt existing worlds out of rich option docs

This avoids breaking the rendering of existing option docs which were
written with the old plain text rendering in mind, while also allowing
new options to default to the rich text rendering instead.

* Use reStructuredText formatting for Lingo Options docstrings

* Disable raw and file insertion RST directives

* Update doc comments per code review

* Make rich text docs opt-in

* Put rich_text_options_doc on WebWorld

* Document rich text API

* Code review

* Update docs/options api.md

Co-authored-by: Doug Hoskisson <[email protected]>

* Update Options.py

Co-authored-by: Doug Hoskisson <[email protected]>

---------

Co-authored-by: Chris Wilson <[email protected]>
Co-authored-by: Doug Hoskisson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: documentation Improvements or additions to documentation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants