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

Renames: align to horizontal_alignment, valign to vertical_alignment, *Container/Tabs alignment, related #55299

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Nov 25, 2021

See: #54161 (no comment AFAIK)

Also unifies the repeated enums in different classes such as Label, Button, etc.

@YuriSizov
Copy link
Contributor

So in all those classes that didn't have horizontal fill option, does this introduce an undefined behavior now?

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Nov 25, 2021

So in all those classes that didn't have horizontal fill option, does this introduce an undefined behavior now?

Looks like most classes add FILL as an alias of LEFT, so I did that for the rest of the switch statements.

@nathanfranke
Copy link
Contributor Author

Any ideas on why the static check is failing? Even if I change the type to "Error" it shows "ERROR: Unresolved type 'Error', file: Button"

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 26, 2021

Any ideas on why the static check is failing? Even if I change the type to "Error" it shows "ERROR: Unresolved type 'Error', file: Button"

I think you have a typo, you refer to [HorizontalAlignment] and [enum HorizontalAlignment] there in the docs, both can't be true anyway, but I also think that neither will link properly. It should probably be [enum @GlobalScope.HorizontalAlignment].

@nathanfranke nathanfranke force-pushed the renames branch 2 times, most recently from 33320c9 to b7a2156 Compare November 27, 2021 02:06
@akien-mga
Copy link
Member

We discussed this in a PR meeting and agreed that this makes sense.

There's a couple points that could still be worked on though:

  • InlineAlign should also be renamed to InlineAlignment for consistent (or everything back to Align if we think it's enough)
  • Having those definitions in math_defs.h is weird, but that was already the case before this PR. But this looks like it could be moved to a class enum in Control. The only two classes which wouldn't have access to it right away would be Font and TextParagraph, but they could likely include control.h for that?

@nathanfranke
Copy link
Contributor Author

  • Having those definitions in math_defs.h is weird, but that was already the case before this PR. But this looks like it could be moved to a class enum in Control. The only two classes which wouldn't have access to it right away would be Font and TextParagraph, but they could likely include control.h for that?

Doesn't look like it's easily possible. control.h depends on text_server.h, but now text_server.h also depends on control.h.

@akien-mga
Copy link
Member

  • Having those definitions in math_defs.h is weird, but that was already the case before this PR. But this looks like it could be moved to a class enum in Control. The only two classes which wouldn't have access to it right away would be Font and TextParagraph, but they could likely include control.h for that?

Doesn't look like it's easily possible. control.h depends on text_server.h, but now text_server.h also depends on control.h.

Maybe text_server.h is a good location for this? Not sure. WDYT @bruvzg?

@nathanfranke nathanfranke force-pushed the renames branch 2 times, most recently from 51f0305 to 2e37224 Compare November 30, 2021 21:58
@nathanfranke nathanfranke changed the title Renames: align to horizontal_alignment, valign to vertical_alignment, related Renames: align to horizontal_alignment, valign to vertical_alignment, BoxContainer/Tabs alignment, related Nov 30, 2021
@nathanfranke
Copy link
Contributor Author

FYI InlineAlignment, BoxContainer, TabContainer, TabBar alignments are renamed. However the main enums are still in Global Scope

@bruvzg
Copy link
Member

bruvzg commented Dec 1, 2021

Maybe text_server.h is a good location for this?

I guess it make sense to move text alignment enums to the text server. It should not cause any dependency issues, but I'm not sure if it's better than global space.

@akien-mga
Copy link
Member

Alternatively if we want to keep them in global space, I think some of the enums in math_defs.h should be moved to a new dedicated file as they have nothing to do with math.
But that's maybe out of scope for this PR.

@nathanfranke nathanfranke changed the title Renames: align to horizontal_alignment, valign to vertical_alignment, BoxContainer/Tabs alignment, related Renames: align to horizontal_alignment, valign to vertical_alignment, *Container/Tabs alignment, related Dec 2, 2021
@akien-mga akien-mga merged commit 4129c1d into godotengine:master Dec 9, 2021
@akien-mga
Copy link
Member

Thanks!

@nathanfranke nathanfranke deleted the renames branch December 9, 2021 09:26
@nathanfranke
Copy link
Contributor Author

nathanfranke commented Dec 9, 2021

@akien-mga Uh oh! Something slipped by GH actions somehow: https://github.com/godotengine/godot/blob/master/editor/plugins/material_editor_plugin.cpp#L128

Should I make a follow up, or can you just commit a fix?

Edit: Currently testing if this line is the only error, at least when building for Linux

Edit: Resolved by 3752e8f, crisis averted

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.

5 participants