Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jun 14, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a181959
Render option documentation as reStructuredText in the WebView
nex3 Jun 11, 2024
6dbb90a
Opt existing worlds out of rich option docs
nex3 Jun 11, 2024
7f0dbc3
Use reStructuredText formatting for Lingo Options docstrings
nex3 Jun 11, 2024
657fb26
Disable raw and file insertion RST directives
nex3 Jun 12, 2024
9b3d739
Update doc comments per code review
nex3 Jun 12, 2024
181aaec
Make rich text docs opt-in
nex3 Jun 12, 2024
daea045
Put rich_text_options_doc on WebWorld
nex3 Jun 12, 2024
11f80af
Document rich text API
nex3 Jun 12, 2024
9f8fbfe
Code review
nex3 Jun 14, 2024
218524f
Merge remote-tracking branch 'origin/main' into fancy-option-docs
nex3 Jun 14, 2024
e07d1fc
Update docs/options api.md
LegendaryLinux Jun 14, 2024
90b97bf
Update Options.py
LegendaryLinux Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions Options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

"""Whether the WebHost should interpret the option's docstring as plain.

By default, a docstring is interpreted as reStructuredText_, the standard
Python markup format. In the WebHost, it's rendered to HTML so that lists,
emphasis, and other rich text features are displayed properly.

However, before reStructuredText support was added, the WebHost rendered all
Option documentation as plain text with preserved whitespace. Most worlds'
Options were documented in a way to make this look good, which doesn't
necessarily look good when rendered as reStructuredText. This flag is set to
False for those Options so they can update their documentation at their
leisure.

.. _reStructuredText: https://docutils.sourceforge.io/rst.html
"""

# filled by AssembleOptions:
name_lookup: typing.ClassVar[typing.Dict[T, str]] # type: ignore
# https://github.com/python/typing/discussions/1460 the reason for this type: ignore
Expand Down Expand Up @@ -1121,9 +1138,11 @@ def __len__(self) -> int:

class Accessibility(Choice):
"""Set rules for reachability of your items/locations.
Locations: ensure everything can be reached and acquired.
Items: ensure all logically relevant items can be acquired.
Minimal: ensure what is needed to reach your goal can be acquired."""

- **Locations:** ensure everything can be reached and acquired.
- **Items:** ensure all logically relevant items can be acquired.
- **Minimal:** ensure what is needed to reach your goal can be acquired.
"""
display_name = "Accessibility"
option_locations = 0
option_items = 1
Expand All @@ -1133,9 +1152,12 @@ class Accessibility(Choice):


class ProgressionBalancing(NamedRange):
"""
A system that can move progression earlier, to try and prevent the player from getting stuck and bored early.
A lower setting means more getting stuck. A higher setting means less getting stuck.
"""Move progression items earlier.

This helps prevent the player from getting stuck and bored early. A lower
setting means items are more random and the player is more likely to get
stuck, a higher setting means more progression items are available early and
the player is less likely to get stuck.
"""
default = 50
range_start = 0
Expand Down Expand Up @@ -1220,13 +1242,15 @@ class StartInventory(ItemDict):

class StartInventoryPool(StartInventory):
"""Start with these items and don't place them in the world.
The game decides what the replacement items will be."""

The game decides what the replacement items will be.
"""
verify_item_name = True
display_name = "Start Inventory from Pool"


class StartHints(ItemSet):
"""Start with these item's locations prefilled into the !hint command."""
"""Start with these item's locations prefilled into the ``!hint`` command."""
display_name = "Start Hints"


Expand All @@ -1236,17 +1260,17 @@ class LocationSet(OptionSet):


class StartLocationHints(LocationSet):
"""Start with these locations and their item prefilled into the !hint command"""
"""Start with these locations and their item prefilled into the ``!hint`` command."""
display_name = "Start Location Hints"


class ExcludeLocations(LocationSet):
"""Prevent these locations from having an important item"""
"""Prevent these locations from having an important item."""
display_name = "Excluded Locations"


class PriorityLocations(LocationSet):
"""Prevent these locations from having an unimportant item"""
"""Prevent these locations from having an unimportant item."""
display_name = "Priority Locations"


Expand Down
15 changes: 15 additions & 0 deletions WebHostLib/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import os
from textwrap import dedent
from typing import Dict, Union
from docutils.core import publish_parts

import yaml
from flask import redirect, render_template, request, Response
Expand Down Expand Up @@ -66,6 +67,20 @@ def filter_dedent(text: str) -> str:
return dedent(text).strip("\n ")


@app.template_filter("rst_to_html")
def filter_rst_to_html(text: str) -> str:
"""Converts reStructuredText (such as a Python docstring) to HTML."""
if text.startswith(" ") or text.startswith("\t"):
text = dedent(text)
elif "\n" in text:
lines = text.splitlines()
text = lines[0] + "\n" + dedent("\n".join(lines[1:]))

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 🙂



@app.template_test("ordered")
def test_ordered(obj):
return isinstance(obj, collections.abc.Sequence)
Expand Down
54 changes: 34 additions & 20 deletions WebHostLib/static/styles/tooltip.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
*/

/* Base styles for the element that has a tooltip */
[data-tooltip], .tooltip {
[data-tooltip], .tooltip-container {
position: relative;
}

/* Base styles for the entire tooltip */
[data-tooltip]:before, [data-tooltip]:after, .tooltip:before, .tooltip:after {
[data-tooltip]:before, [data-tooltip]:after, .tooltip-container:before, .tooltip {
position: absolute;
visibility: hidden;
opacity: 0;
Expand All @@ -39,22 +39,23 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
pointer-events: none;
}

[data-tooltip]:hover:before, [data-tooltip]:hover:after, .tooltip:hover:before, .tooltip:hover:after{
[data-tooltip]:hover:before, [data-tooltip]:hover:after, .tooltip-container:hover:before,
.tooltip-container:hover .tooltip {
visibility: visible;
opacity: 1;
word-break: break-word;
}

/** Directional arrow styles */
.tooltip:before, [data-tooltip]:before {
[data-tooltip]:before, .tooltip-container:before {
z-index: 10000;
border: 6px solid transparent;
background: transparent;
content: "";
}

/** Content styles */
.tooltip:after, [data-tooltip]:after {
[data-tooltip]:after, .tooltip {
width: 260px;
z-index: 10000;
padding: 8px;
Expand All @@ -63,44 +64,46 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
background-color: hsla(0, 0%, 20%, 0.9);
color: #fff;
content: attr(data-tooltip);
white-space: pre-wrap;
font-size: 14px;
line-height: 1.2;
}

[data-tooltip]:before, [data-tooltip]:after{
[data-tooltip]:after {
white-space: pre-wrap;
}

[data-tooltip]:before, [data-tooltip]:after, .tooltip-container:before, .tooltip {
visibility: hidden;
opacity: 0;
pointer-events: none;
}

[data-tooltip]:before, [data-tooltip]:after, .tooltip:before, .tooltip:after,
.tooltip-top:before, .tooltip-top:after {
[data-tooltip]:before, [data-tooltip]:after, .tooltip-container:before, .tooltip {
bottom: 100%;
left: 50%;
}

[data-tooltip]:before, .tooltip:before, .tooltip-top:before {
[data-tooltip]:before, .tooltip-container:before {
margin-left: -6px;
margin-bottom: -12px;
border-top-color: #000;
border-top-color: hsla(0, 0%, 20%, 0.9);
}

/** Horizontally align tooltips on the top and bottom */
[data-tooltip]:after, .tooltip:after, .tooltip-top:after {
[data-tooltip]:after, .tooltip {
margin-left: -80px;
}

[data-tooltip]:hover:before, [data-tooltip]:hover:after, .tooltip:hover:before, .tooltip:hover:after,
.tooltip-top:hover:before, .tooltip-top:hover:after {
[data-tooltip]:hover:before, [data-tooltip]:hover:after, .tooltip-container:hover:before,
.tooltip-container:hover .tooltip {
-webkit-transform: translateY(-12px);
-moz-transform: translateY(-12px);
transform: translateY(-12px);
}

/** Tooltips on the left */
.tooltip-left:before, .tooltip-left:after {
.tooltip-left:before, [data-tooltip].tooltip-left:after, .tooltip-left .tooltip {
right: 100%;
bottom: 50%;
left: auto;
Expand All @@ -115,14 +118,14 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
border-left-color: hsla(0, 0%, 20%, 0.9);
}

.tooltip-left:hover:before, .tooltip-left:hover:after {
.tooltip-left:hover:before, [data-tooltip].tooltip-left:hover:after, .tooltip-left:hover .tooltip {
-webkit-transform: translateX(-12px);
-moz-transform: translateX(-12px);
transform: translateX(-12px);
}

/** Tooltips on the bottom */
.tooltip-bottom:before, .tooltip-bottom:after {
.tooltip-bottom:before, [data-tooltip].tooltip-bottom:after, .tooltip-bottom .tooltip {
top: 100%;
bottom: auto;
left: 50%;
Expand All @@ -136,14 +139,15 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
border-bottom-color: hsla(0, 0%, 20%, 0.9);
}

.tooltip-bottom:hover:before, .tooltip-bottom:hover:after {
.tooltip-bottom:hover:before, [data-tooltip].tooltip-bottom:hover:after,
.tooltip-bottom:hover .tooltip {
-webkit-transform: translateY(12px);
-moz-transform: translateY(12px);
transform: translateY(12px);
}

/** Tooltips on the right */
.tooltip-right:before, .tooltip-right:after {
.tooltip-right:before, [data-tooltip].tooltip-right:after, .tooltip-right .tooltip {
bottom: 50%;
left: 100%;
}
Expand All @@ -156,7 +160,8 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
border-right-color: hsla(0, 0%, 20%, 0.9);
}

.tooltip-right:hover:before, .tooltip-right:hover:after {
.tooltip-right:hover:before, [data-tooltip].tooltip-right:hover:after,
.tooltip-right:hover .tooltip {
-webkit-transform: translateX(12px);
-moz-transform: translateX(12px);
transform: translateX(12px);
Expand All @@ -168,7 +173,16 @@ give it one of the following classes: tooltip-left, tooltip-right, tooltip-top,
}

/** Center content vertically for tooltips ont he left and right */
.tooltip-left:after, .tooltip-right:after {
[data-tooltip].tooltip-left:after, [data-tooltip].tooltip-right:after,
.tooltip-left .tooltip, .tooltip-right .tooltip {
margin-left: 0;
margin-bottom: -16px;
}

.tooltip ul, .tooltip ol {
padding-left: 1rem;
}

.tooltip :last-child {
margin-bottom: 0;
}
13 changes: 12 additions & 1 deletion WebHostLib/templates/playerOptions/macros.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,18 @@
{% macro OptionTitle(option_name, option) %}
<label for="{{ option_name }}">
{{ option.display_name|default(option_name) }}:
<span class="interactive" data-tooltip="{% filter dedent %}{{(option.__doc__ | default("Please document me!"))|escape }}{% endfilter %}">(?)</span>
<span
class="interactive tooltip-container"
{% if option.plain_text_doc %}
data-tooltip="{{option.__doc__ | default("**Please document me!**") | dedent}}"
{% endif %}>
(?)
{% if not option.plain_text_doc %}
<div class="tooltip">
{{ option.__doc__ | default("**Please document me!**") | rst_to_html | safe }}
</div>
{% endif %}
</span>
</label>
{% endmacro %}

Expand Down
4 changes: 4 additions & 0 deletions worlds/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ def load(self) -> bool:
# Build the data package for each game.
from .AutoWorld import AutoWorldRegister

# Set legacy features
import worlds.legacy_world_features

network_data_package: DataPackage = {
"games": {world_name: world.get_data_package_data() for world_name, world in AutoWorldRegister.world_types.items()},
}

34 changes: 34 additions & 0 deletions worlds/legacy_world_features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# This file triggers various "legacy modes" for worlds, so that breaking changes
# can be made without needing to simultaneously update or break every exisitng
# world. Authors of worlds listed here should remove their worlds when they have
# time to update them to the new behavior.

from Options import PerGameCommonOptions
from worlds.AutoWorld import AutoWorldRegister

# # Option docstring format
#
# 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 •
edited
Loading

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.

"Adventure", "A Hat in Time", "A Link to the Past", "Aquaria", "ArchipIDLE", "A Short Hike",
"Blasphemous", "Bomb Rush Cyberfunk", "Bumper Stickers", "Castlevania 64", "Celeste 64",
"ChecksFinder", "Clique", "Dark Souls III", "DLCQuest", "Donkey Kong Country 3", "DOOM 1993",
"DOOM II", "Factorio", "Final Fantasy", "Final Fantasy Mystic Quest", "Heretic",
"Hollow Knight", "Hylics 2", "Kingdom Hearts 2", "Kirby's Dream Land 3",
"Landstalker - The Treasures of King Nole", "Lufia II Ancient Cave",
"Mario & Luigi Superstar Saga", "MegaMan Battle Network 3", "Meritous", "Minecraft",
"Muse Dash", "Noita", "Ocarina of Time", "Overcooked! 2", "Pokemon Emerald",
"Pokemon Red and Blue", "Raft", "Risk of Rain 2", "Rogue Legacy", "Secret of Evermore",
"Shivers", "Slay the Spire", "SMZ3", "Sonic Adventure 2 Battle", "Starcraft 2",
"Stardew Valley", "Subnautica", "Sudoku", "Super Mario 64", "Super Mario World",
"Super Metroid", "Terraria", "The Legend of Zelda", "The Messenger", "The Witness",
"Timespinner", "TUNIC", "Undertale", "VVVVVV", "Wargroove", "Yoshi's Island", "Yu-Gi-Oh! 2006",
"Zillion"
]:
common_options = set(option for option in PerGameCommonOptions.type_hints.values())
world = AutoWorldRegister.world_types[world_name]
for option in world.options_dataclass.type_hints.values():
if option not in common_options:
option.plain_text_doc = True
Loading
Loading