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

Add copy codeblock button to built-in documentation #87363

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 19, 2024

Would combine really well with #87301

This PR adds a Copy button to the codeblock examples in the built-in class reference.

Unfortunately, with the icon, the underline still shows up and cuts through the icon. This is a bug with RichTextLabel itself.
image

If the underline could be removed outright this would be less of an issue, but alas. So in the current PR it looks like this:
image

@Mickeon Mickeon changed the title Update editor_help.cpp Add copy codeblock button to built-in documentation Jan 19, 2024
@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from 1485634 to b64bbc4 Compare January 19, 2024 09:10
@AThousandShips AThousandShips added this to the 4.x milestone Jan 19, 2024
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
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

It doesn't quite work at the moment. Left-brackets get eaten somewhere

godot.windows.editor.dev.x86_64_2024-01-29_18-32-50.mp4

It would also be nice to add some visual indication to the click. We might need to modify RTL to achieve that, but without any feedback it feels very poor.

On the subject of modifying RTL, we should be able to add a new argument to the meta tag which allows disabling the underlining on the case by case to solve your issue with the icon.

@Calinou
Copy link
Member

Calinou commented Jan 29, 2024

It would also be nice to add some visual indication to the click. We might need to modify RTL to achieve that, but without any feedback it feels very poor.

I've definitely been missing hover/pressed feedback in RTL (also in projects), so we'll probably need to modify RTL to support this using additional theme items.

@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from b64bbc4 to 6a18dc4 Compare January 29, 2024 19:01
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 29, 2024

The PR has been updated to fix the above quirks. Still, I suppose it cannot be merged as is because the above things are missing in RichTextLabel.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 29, 2024

Sidenote, all of these desired features may give more "credence" to implement godotengine/godot-proposals#7617, but that would be a huge undertaking.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Seems to work correctly now. However, without #87301, or something similar, this isn't really practical. Copied code snippets will immediate err and throw the user into confusion.

So approving for functionality, but as is, I wouldn't merge it. Usability improvements discussed above would also be nice (can be implemented separately as a pre-requisite, then this could be updated on top).

@Mickeon Mickeon requested review from a team February 11, 2024 15:24
@KoBeWi
Copy link
Member

KoBeWi commented Feb 27, 2024

Needs rebase.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 27, 2024

I will gladly rebase if this is desired, even without the above omissions due to RichTextLabel's limitations

@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch 2 times, most recently from f05bb59 to 266bc29 Compare February 28, 2024 00:40
editor/editor_help.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine. The Copy button doesn't overlap anything, so as long as it's usable, it can be improved later.

The codeblock_text optimization I mentioned should be looked into, as appending that many strings might actually make the help slower to load (though it doesn't seem to have significant impact).

@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from 266bc29 to 17eda55 Compare February 28, 2024 17:05
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 28, 2024

The entire built-in class ref generation would benefit from StringBuilder t.b.h.. I don't know what that is, never used it, but maybe it'll be the occasion to...

For now I updated the PR to remove the default "" value but maybe I can figure this one out before a merge.

@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from 17eda55 to adfa6e4 Compare February 28, 2024 18:02
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 28, 2024

Hey, that's not too bad. Updated the PR to use StringBuilder. This "thing" could be used in so many other places...

editor/editor_help.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

@Mickeon Do you want to go back to an icon now that #89024 is merged?

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 4, 2024

I can do that. There is still a problem of "lack of feedback" when hovering and clicking over the button that the "Copy" text would mitigate (because the underline could appear on hover).

@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from 29470b3 to e1265b0 Compare March 4, 2024 17:19
@Mickeon Mickeon force-pushed the documentation-copy-codeblock-button branch from e1265b0 to f739f78 Compare March 4, 2024 19:07
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 4, 2024

I updated the PR to use an image. If it is settled to use raw text, we can go back on it just fine.

At first I went with this but I thought it was way too highlighted.
image

So I settled with something like this:

You may notice that the underline is a bit bugged out when hovering on the side, but that's RichTextLabel's fault (again). It once again makes me question the utility of DISABLED but it's got to be useful somewhere.

The gif itself is outdated as I made the icon ever so slightly smaller.
image

@akien-mga
Copy link
Member

The icon looks a bit blurry to me, is there a scaling issue?

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 4, 2024

Yeah. It's 24x24 (excluding editor scale), I can tone the scale down to 16x16 (as per the previews in the description of the PR) if you wish.

@bruvzg
Copy link
Member

bruvzg commented Mar 4, 2024

You may notice that the underline is a bit bugged out when hovering on the side, but that's RichTextLabel's fault (again).

Seems like meta hover callbacks are using wrong argument (for selection, instead of meta), will fix it in a moment.

@akien-mga
Copy link
Member

Yeah. It's 24x24 (excluding editor scale), I can tone the scale down to 16x16 (as per the previews in the description of the PR) if you wish.

I think it should be rendered from SVG at the size we want to use it with (including editor scale), so it's crisp. Linearly scaling up will always look bad.

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 4, 2024

Definitely out of scope here. The "blurriness" has been an issue with other icons in the built-in documentation, as well. It's just more noticeable here because it is larger than usual.
image

So, should I reduce the size until the issue with blurry icons is resolved?

@akien-mga
Copy link
Member

I guess it's fine as is for now, but we really need to solve these blurry icons :)

@akien-mga akien-mga merged commit a07dd0d into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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