Added alignment options to icons on buttons. They can now be centered and right-aligned.

[jitspoe]

Icons now have the same alignment options as text on buttons. Icons can be aligned on the left, right, or centered. Left is the same as the current behavior. Right is basically the same, but on the other side. Centered centers the icon and draws the text (if any) on top. Very useful if you want to have menu buttons that can sometimes have text OR an icon and you want them justified in the same way.

Fixes #11380

[YeldhamDev]

A bit of documentation for the new methods would be nice.

[jitspoe]

A bit of documentation for the new methods would be nice.

Ah, good call. I forgot I added new functions for this. Should that be a separate pull request, or should it be squashed into this one? Never touched the documentation before.

[akien-mga]

Squashed in this commit would be best. You can generate the template for the new functions with godot --doctool . (see https://docs.godotengine.org/en/latest/community/contributing/updating_the_class_reference.html). You might have a few unrelated changes to undo, especially removal of some Mono things if you're generating docs with a non-Mono build.

[jitspoe]

Ok, I think I have it all squared away now. For some reason, the doc generation did not work proprely using just "." as the output directory. Nothing changed. I then tried using a test directory, and it generate a bunch of stub xml files. Finally, I ended up just doing "testdir.." and that ended up working. Strange. This was on a window build.

Rebasing also got interesting with some merge conflicts. Always something new whenever I try to do a pull request.

[Calinou]

Rebasing also got interesting with some merge conflicts. Always something new whenever I try to do a pull request.

The master branch is currently ongoing heavy refactoring (DisplayServer, node names, ...), so this is expected. Sorry for the inconvenience.

[jitspoe]

Merge conflicts resolved. Just waiting on a review, now.

[KoBeWi]

Looks ok feature and code-wise (aside from that one nitpick about order).

We could rename align to text_align, so these properties are less confusing, but this could be done in another PR (and allow this one to be cherry-picked easier).

[sairam4123]

Having an option for drawing text on the bottom will be good too while the icon is centered.

[akien-mga]

The implementation seems fine, but looking at the screenshot on #11380 (comment), I'm not sure how usable the CENTER align is when using text which will overlap, unless we also implement vertical positioning options for the text as suggested by @sairam4123.

[jitspoe]

I don't imagine people would really use the centered text AND centered icon. It's just there because that's a possible combination. I primarily wanted to just be able to center icons by themselves. Icons could maybe be used for backgrounds on buttons as well. We could always expand this later if there's a desire for it.

[RandomShaper]

LGTM.

[akien-mga]

@jitspoe Could you rebase to solve merge conflicts?

[jitspoe]

@jitspoe Could you rebase to solve merge conflicts?

Working on merging this. It's a pretty significant conflict with all the rtl changes.

[jitspoe]

I'm really confused by some of these changes and how to test them. For example,

					if (rtl) {
						if (_internal_margin[SIDE_RIGHT] > 0) {
							icon_ofs_region = _internal_margin[SIDE_RIGHT] + get_theme_constant("hseparation");
						}
					} else {
						if (_internal_margin[SIDE_LEFT] > 0) {
							icon_ofs_region = _internal_margin[SIDE_LEFT] + get_theme_constant("hseparation");
						}
					}

Why, if the text is RTL, would we be using the right side margin for the icon offset? The icon is still on the left, right? Wouldn't we always want to be using the left margin? Also, when does "rtl" get set? There's a "Text Direction" property on the button that can specify "RTL", but it appears to not change any behavior with the text. I even tried putting in some Arabic text to test. Nothing appears to change within the editor. I'm confused as to what all these checks are doing, and if they're even necessary. Seems to be making the code rather complex.

[akien-mga]

CC @bruvzg

You might find some explanations in https://godotengine.org/article/complex-text-layouts-progress-report-2 in the part about UI mirroring.

If RTL UI mirroring is enabled, then it would be expected that a Left icon would be shown to the Right. E.g. if your usage is "user avatar on the left, then name on the right", in Arabic this should be "user avatar on the right, then name on the left". Godot's Complex Text Layouts would handle this automagically based on the locale's preference.

[bruvzg]

Why, if the text is RTL, would we be using the right side margin for the icon offset? The icon is still on the left, right?

No, both text and icon are aligned to the right, as well as everything else. It's done to automatically flip layout for right-to-left languages without redesigning UI.

For is_rtl branches just swap left and right alignment for both text and icon (ALIGN_LEFT aligns to the right and vise versa).

[jitspoe]

Ok, that's what the code looked like it was doing, but when I tried a nightly build and attempted to enable RTL on a button, it didn't seem to make a difference. How do I test this?

[bruvzg]

Ok, that's what the code looked like it was doing, but when I tried a nightly build and attempted to enable RTL on a button, it didn't seem to make a difference. How do I test this?

Layout direction is controlled by layout_direction property (under Control section), there's also text_direction property which controls BiDi reordering base direction (it won't have any effect unless you have text with mixing writing directions in the control).

[jitspoe]

I rewrote some of the rtl checking logic a little bit because the way it was previously implemented would involve lots of copy/pasting code and be difficult to maintain. I believe everything is working properly. I'm unsure how to test the internal margins, though. What sets things like _internal_margin[SIDE_RIGHT]?

[akien-mga]

It would be good if you can rebase on top of upstream master to remove the "Merge branch 'master'" commit (we use merge commits when merging PRs to master, but don't want them from in-development PR syncs).

[bruvzg]

With exception to min. size not updated on icon alignment change, everything seems to be working fine.

[akien-mga]

Force pushed an update to rebase on master, add the missing minimum size update, and fix a few style issues.

[akien-mga]

Thanks!

[jitspoe]

Thanks for taking care of the last couple changes. Should I make a pull request for 3.x as well? Will the RTL stuff be making its way to 3.x?

[Calinou]

Should I make a pull request for 3.x as well?

Feel free to open a pull request for 3.x as well 🙂

Will the RTL stuff be making its way to 3.x?

No, as it's backwards-incompatible with existing projects.

[raulsntos]

@jitspoe Did you ever get around to backporting this to 3.x? It's totally fine if you are busy/unavailable and I'd be willing to backport it myself if you don't mind.

[jitspoe]

It's on my todo list but it's kinda far down. I actually wrote it for 3.x then rewrote it for 4.0, so I have some iteration of it here: https://github.com/jitspoe/godot/tree/3.x.jitspoe

I think I may have changed some variable names or something and wanted to make sure it was consistent with what was in 4.0. Got kind of a big milestone coming up, so I probably won't have time to look at it this month.

[raulsntos]

I've opened PR #57771 to backport this to 3.x.