Skip to content

Add custom Sphinx roles for editor UI #10251

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tetrapod00
Copy link
Contributor

This PR is a proof of concept, and is subject to change. It only implements the custom roles and a short example usage section. I have a separate branch for actual usage changes, with several hundred already changed. Decisions of which roles to add are being driven by actual usage.

What this is

This PR adds custom Sphinx roles, like :btn:, :menu:, and :ui:, for various editor UI elements. It renders most of them with bold text and a light blue-gray background.

It looks like this:

msedge_YpWdiI9fOd

Or this:

chrome_dmv9HGaD83

Currently implemented are:

  • :btn: A button, toggle, or most other clickable UI elements. If the reader is meant to click on it, and it's not a menu, use this.
  • :menu: A series of menus to click through, or a single menu to click on. When listing a series of menus, separate them with >.
  • :uiproperty: A property in the inspector. Note that you can also link to a property, or use code style to refer to a property when used in code rather than the inspector. It's called "uiproperty" and not "property" for a reason, since inspector properties are treated differently than properties accessed through code. "property" or "prop" should be reserved for future use for the other case. This name is definitely too long, it will be changed.
  • :ui A catch-all role for any editor UI elements. Use this if you would have otherwise just used bold style. It can be changed later, if necessary.
  • :field: An input field in the editor; anything that you type into. Excludes properties, though.
  • :path: A UI navigation path in the editor. Use :uisection: instead for nested sections in the inspector. Use :uiproperty: instead for inspector properties that include a section like Process > Mode. Use :projsection: for sections in the project settings. Probably will be merged with menu, but I'm keeping both around to see if there is some nuance to their usage to preserve.
  • :wndw: An editor window, popup dialog, or modal. Anything that can be separately dragged and has a title.
  • :uisection: A section in the inspector. Likely will be renamed to be shorter.
  • :projsection: A section in the project settings. Likely will be renamed to be shorter.
  • :tab: A tab.
  • :dock: A dock.
  • :panel: A panel.

As you can see, some are very specific, and some are probably redundant. I'm tagging very specifically first, then I'll come back and merge them together based on actual usage.

Why

This solves two problems, style inconsistency and maintenance.

Style inconsistency: Style for UI elements in the editor is inconsistent. For buttons, we currently use bold, italics, code, or "quotes". We've mostly standardized on bold, but there are a lot of existing usages that are inconsistent.

Maintenance: Using semantically meaningful roles for each separate kind of editor UI allows us to change the style of each one independently, without breaking translations, just by changing the CSS. It also makes searching for all instances of one kind of editor UI much easier, since the tag can be searched.

Technical details

The custom roles are defined inline in rst_prolog. I experimented with more complex custom roles as a Sphinx extension, but they are not needed. All this can be accomplished in Sphinx userspace with CSS and some simple definitions:

.. role:: btn
   :class: uibutton uistyle

However, in the future we can change these roles to have more complex behavior, without changing the RST content files. Since all the usages will already be tagged, we can switch out the implementation of the role seamlessly.

Downsides

  • Source files are less readable.
  • Source files are a bit harder to type.
  • Markdown previews become less useful. Some of the new roles simply render as bold, but since they no longer use **, previews don't know that.
  • Contributors have to know about the specific tags, rather than just using bold as they see fit. I think this can be mitigated by using btn, menu, and ui as general-purpose tags. If the tag needs to be made more specific, it can be done in review by a contributors who are more familiar. Or we can limit the overall number of tags.
  • Huge translation churn for the initial set of changes.

Q & A

Why not just use :guilabel:?

Sphinx's guilabel and menuselection roles are the inspiration for this PR. I tried them in #10238. They look like this:

msedge_RZYRAwMuEr

For one reason, the name is too long, very annoying to type. For another, it's not granular enough in my opinion. It solves the style inconsistency problem, but does not solve the maintenance problem, since many things would be tagged together under one label.

Why so many specific roles?

All existing usages must be changed from bold, italics, etc to use a role. It's easier to tag them very specifically on the first pass, then replace specific roles with generic ones. We might decide to only use a few roles (I would pick :ui:. :btn:, and :menu:, and one for inspector properties).

Why :btn: and not :button:, :wndw: not :window:, etc?

I wasn't sure about this myself but after replacing a lot of usages, the extra letters do add up! I think 3-5 is the sweet spot.

Why not enforce a style guideline by hand?

We currently do this, and the manual is currently not consistent. Why not let the computer handle the formatting?

How many existing usages to change?

Just for existing bold usage, I found hundreds of instances to change. But after I started actually looking in files, there are lots more than this. Probably on the order of 2000+ usages to change. I think I've changed several hundred in my other branch already.

  • 48 instances of **Something** button
  • 9 instances of **Something** window
  • 33 instances of **Something** menu
  • 18 instances of **Something** setting
  • 7 instances of **Something** dropdown
  • 7 instances of **Something** field
  • 268 instances of **UI > Navigation > Path**
  • 95 instances of **Inspector Property**
  • 14 instances of click on **Something**
  • 52 instances of click **Something**
  • 35 instances of select **Something**
  • 24 instances of enable **Something**
  • 46 instances of **Something** tab

Won't this cause a lot of translation churn?

Yep! That's why I want to do it once, using fairly granular tagging, and then in the future we can change style with CSS instead of editing the RST content directly.

Future plans

At some point I'd like to take a shot at implementing a :property: or :method: role that autolinks to the class reference (and ideally is smart enough to link on the first usage in a page, then use code style. See #8890. This PR is a first step towards that.

@tetrapod00 tetrapod00 added area:about Issues and PRs related to the About section of the documentation and other general articles area:getting started Issues and PRs related to the Getting Started section of the documentation area:manual Issues and PRs related to the Manual/Tutorials section of the documentation area:contributing Issues and PRs related to the Contributing/Development section of the documentation enhancement discussion python Pull requests that update Python code labels Nov 14, 2024
@mhilbrunner
Copy link
Member

From RC: I am in favor of doing this (now and once) over having the permanent churn of some style changes here and there for how we do Paths, "Menu -> Submenu -> Submenu", buttons/keymappings etc. Implementation wise, I think my only nitpick is that I'd prefer the more-readable and imo clear "button", "window" and "label" over "btn", "wndw" and "lbl", even at the cost of some brevity.

Once we split off 4.4 soon, it'd be a good point to introduce this, if we go for it.

@mhilbrunner mhilbrunner modified the milestone: 4.4 Jan 22, 2025
@tetrapod00
Copy link
Contributor Author

tetrapod00 commented Jan 22, 2025

I've been thinking about this some and the various arguments for fewer and less-specific roles, at least initially, makes sense to me. I think my initial, minimum-viable set would be ui, button, path (or menu, either way it's the one with > that is based on the Sphinx menuselection role). I'd like something for properties but I think that may be a separate concern.

Copy link
Contributor

@skyace65 skyace65 left a comment

Choose a reason for hiding this comment

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

I can't speak for the code but I do want this feature.

@skyace65
Copy link
Contributor

skyace65 commented Apr 7, 2025

@mhilbrunner I'm gonna build this branch tomorrow and test it. Any objections to me merging if everything is good?

@tetrapod00
Copy link
Contributor Author

As is this shouldn't be merged. We settled on a much smaller set of roles to start with after rocketchat discussion. This needs a rebase and an update to use the smaller set of roles. I'll try to get this ready in time so we can merge this just after the last set of 4.4 changes.

@skyace65
Copy link
Contributor

skyace65 commented Apr 7, 2025

As is this shouldn't be merged. We settled on a much smaller set of roles to start with after rocketchat discussion. This needs a rebase and an update to use the smaller set of roles. I'll try to get this ready in time so we can merge this just after the last set of 4.4 changes.

If you need an extra week or something I can push back merging that first 4.5 PR. This needs to get merged before we start merging 4.5 PRs because otherwise it would make cherrypicking PRs significantly harder.

@tetrapod00
Copy link
Contributor Author

Should be ready later today or tomorrow, I'm just double checking a few things. I didn't realize we wanted to get this into the 4.4 version as well, but it makes sense.

I'm assuming that we want the role definitions available in 4.4 but we won't be cherrypicking back most of the actual usage changes?

@tetrapod00 tetrapod00 force-pushed the custom-role branch 3 times, most recently from c196cab to b645901 Compare April 9, 2025 04:05
@tetrapod00
Copy link
Contributor Author

tetrapod00 commented Apr 9, 2025

Ready for review. I settled on four custom roles, ui, uiproperrty, menu, and button, which are documented within this PR itself.

Implementation notes/points of discussion:

  • Still using rst_prolog and a simple RST custom role; nothing that involves a Sphinx extension. But it is possible to convert these to more complex Sphinx extension roles in the future, without changing the usages across many pages.
  • ui and uiproperty use just bold, while button and menu use bold with a background. Paragraphs look too busy if the bold with background is used too often. If a UI element is meant to be clicked on, it uses the more attention-grabbing style. The line is a little fuzzy with things like inspector sections or project settings sections. I leaned towards using just the less distracting bold wherever it was a choice. We can fine-tune this in the future.
  • The dark theme role-button-background-color is #22252D, the same as dark mode code-background-color. For the light theme, the same code background color looked too bright, so I just picked a nice looking color (#D3D7E1). Feel free to suggest improvements to the visual theming. We can also iterate on this later.
  • In addition to the examples in the Writing Guidelines page, I also converted the Getting Started > Step by step > Nodes and Scenes page to use the new roles, so you can see how this would look in practice.
  • The All styles in context section might be redundant and removable. I found it useful for testing how the CSS looks in admonitions though.
  • I'd like second opinions on uiproperty. Currently, it is meant for properties in the inspector, and not in code, which is why it is named as it is. I'm happy with any of these four options:
    • 1 Current state of this PR. Called uiproperty, to disambiguate and future-proof, but the name is awkward. It leaves the possibility to add a property role later for code properties, which are styled differently than inspector properties.
    • 2 Rename it to property. Don't worry about future-proofing, and instead use the best name we have available now. If we did add a different property role in the future, we would likely reconsider all existing usages of uiproperty anyway.
    • 3 Rename it to inspector (or some other name, if you have suggestions). These properties exist in the inspector. This would be a one-word role name, and saves the property role for later.
    • 4 Postpone adding a property role for the future. For now, use ui for all inspector properties, which renders the same (it's just bold style). If we add a property role of any kind later, this will entail another set of large changes as we change ui to property.
  • If anything, I'm leaning towards option 2 after writing these up.

@skyace65
Copy link
Contributor

Haven't had a chance to review this yet. Just wanted to note that upon reflection we should be fine to merge this in to the 4.4 branch later as a cherrypick if we go that route.

@mhilbrunner
Copy link
Member

I'd prefer :inspector: over :uiproperty: to be more specific and guard against misuse. Similarly, I wonder if :editor: would be more clear than :ui:, so people don't start using that to denote UI classes or their properties or whatever. We could still catch those in reviews, of course.

@tetrapod00
Copy link
Contributor Author

I think :ui: is the right choice for the catch-all role. ui and button will also be used to describe the UI of other programs, such as using GitHub or IDEs, so :editor: is not quite right. Also, all else being equal I would strongly prefer shorter tags here, and I think :ui: is still reasonably clear, unlike something like :btn:. Happy to be overruled here though.

:inspector: seems okay with me, if no one else has any opinions one way or the other.

@tetrapod00
Copy link
Contributor Author

Changed to use :inspector: over :uiproperty:.

@timothyqiu
Copy link
Member

For one reason, the name is too long, very annoying to type.

Replacing the standard :menuselection: with a custom :menu: for this reason alone isn't worth it and can be confusing for users who are familiar with Sphinx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:about Issues and PRs related to the About section of the documentation and other general articles area:contributing Issues and PRs related to the Contributing/Development section of the documentation area:getting started Issues and PRs related to the Getting Started section of the documentation area:manual Issues and PRs related to the Manual/Tutorials section of the documentation discussion enhancement python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants