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

Replace built-in docs codeblock's leading spaces with tabs #87301

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 17, 2024

This PR replaces the spaces used in the codeblocks of the class references with actual tabs.
If your Editor Setting's preference is using spaces, nothing changes.
With one GIF, this means the following (notice the tabs):

Before After

However, hm... It comes at a cost. For the tab to display in a way that actually looks good, the RichTextLabel's tab_size has to be set to 8, otherwise the tab's indent would be way, way too small, for some reason. Because of this, there's the side effect of increasing the indentation across the whole built-in documentation:

Take a closer look:

Before After
image image
image image
image image
image image

Some may say this is a happy accident, some may absolutely not. So keep that in mind.

@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from 9404070 to 737fbaa Compare January 17, 2024 16:50
@AThousandShips AThousandShips added this to the 4.x milestone Jan 17, 2024
@YuriSizov
Copy link
Contributor

Could you please rebase on the current master? We had an issue with macOS builds which failed your CI.

@Mickeon Mickeon requested review from a team as code owners January 17, 2024 18:24
@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from a458548 to 3f9c0e4 Compare January 17, 2024 18:25
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

Done just that.

@MewPurPur
Copy link
Contributor

MewPurPur commented Jan 18, 2024

I don't like the increased indent but I think it's worth it. I'm just confused why we'd need an 8 tab size for what's supposed to be monospace. Also, I would expect the mono tabs to be as wide as my code editor theme, which isn't 4.

This is still better than before, I don't think these nitpicks are more important than getting it merged.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 18, 2024

I'm just confused why we'd need an 8 tab size for what's supposed to be monospace.

Honestly, I am not sure why RichTextLabel acts like this. I tried a value of 4 which you would think would be appropriate, but it resulted in the padding being way too short for some reason.

It originally was also based on the "indent size" Editor Setting, but because it would've affected the entire RichTextLabel I was not sure about it.

@Mickeon Mickeon requested review from a team and removed request for a team February 27, 2024 12:32
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

editor/editor_help.cpp Outdated Show resolved Hide resolved
editor/editor_help.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from 3f9c0e4 to 9d3d805 Compare February 27, 2024 17:35
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, there are issues with code samples that contain multiple tabs on the same line.

For instance, the HTTPRequest class description has a code sample where the push_error() line has two leading tabs, but none are displayed and copied here:

image

If you select the block and paste it in a script, you get this:

image

@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from 9d3d805 to a30f7f1 Compare February 28, 2024 18:58
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 28, 2024

You will never guess who the idiot that completely fucked up the formula is.

Should work fine now:

@Mickeon Mickeon requested a review from Calinou February 29, 2024 00:44
editor/editor_help.cpp Outdated Show resolved Hide resolved
editor/editor_help.cpp Outdated Show resolved Hide resolved
editor/editor_help.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from a30f7f1 to 4c3e5c0 Compare February 29, 2024 16:16
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 29, 2024

Updated the PR to use StringBuilder. I tested and nothing really changed on the outside.

Replace built-in docs codeblock's leading spaces with tabs
@Mickeon Mickeon force-pushed the documentation-leading-spaces-to-tabulation branch from 4c3e5c0 to 9d3768d Compare February 29, 2024 16:18
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 29, 2024
@akien-mga akien-mga merged commit 06d4023 into godotengine:master Mar 1, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 1, 2024

And from here on out begins the built-in reference revolution

@Mickeon Mickeon deleted the documentation-leading-spaces-to-tabulation branch March 1, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants