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

Rename Control getters and setters to match properties #47248

Closed

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Mar 22, 2021

Part of #16863, originally identified by @AlexHolly here.

Includes renaming some properties to match the getters and setters where appropriate.

Method renames:

Property Method Before Method After
mouse_default_cursor_shape set_default_cursor_shape set_mouse_default_cursor_shape
" get_default_cursor_shape get_mouse_default_cursor_shape
-- get_cursor_shape get_mouse_cursor_shape
grow_horizontal set_h_grow_direction set_grow_horizontal
" get_h_grow_direction get_grow_horizontal
grow_vertical set_v_grow_direction set_grow_vertical
" get_v_grow_direction get_grow_vertical
rect_clip_contents set_clip_contents set_rect_clip_contents
" is_clipping_contents is_rect_clipping_contents
rect_global_position set_global_position set_rect_global_position
" get_global_position get_rect_global_position
rect_position set_position set_rect_position
" get_position get_rect_position
rect_rotation set_rotation set_rect_rotation
" get_rotation get_rect_rotation
rect_rotation_degrees set_rotation_degrees set_rect_rotation_degrees
" get_rotation_degrees get_rect_rotation_degrees
rect_minimum_size set_custom_minimum_size set_rect_minimum_size
" get_custom_minimum_size get_rect_minimum_size
rect_pivot_offset set_pivot_offset set_rect_pivot_offset
" get_pivot_offset get_rect_pivot_offset
rect_scale set_scale set_rect_scale
" get_scale get_rect_scale
rect_size set_size set_rect_size
" get_size get_rect_size
size_flags_horizontal set_h_size_flags set_size_flags_horizontal
" get_h_size_flags get_size_flags_horizontal
size_flags_stretch_ratio set_stretch_ratio set_size_flags_stretch_ratio
" get_stretch_ratio get_size_flags_stretch_ratio
size_flags_vertical set_v_size_flags set_size_flags_vertical
" get_v_size_flags get_size_flags_vertical

Property renames:

Before After
rect_clip_content rect_clip_contents
rect_min_size rect_minimum_size
hint_tooltip tooltip

@YuriSizov
Copy link
Contributor

Do you mind listing the renames here directly? Otherwise it would be a pain to refactor PRs after this one.

@madmiraal
Copy link
Contributor Author

Do you mind listing the renames here directly?

Done.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 22, 2021

rect_min_size -> rect_minimum_size

Why this needs to be changed? We already use min/max for math functions, so it's not like the property is unclear. Also if we are expanding, rectangle_minimum_size would be more correct/consistent, but it's even longer. While it's ok to make methods more explicit, property names take inspector space, so we shouldn't elongate them unnecessarily.

@AlexHolly
Copy link
Contributor

Such changes where already made in the past. I think this is where it originated.
9bc5348#diff-e31be202f4d69f30dffc794ce6dc2de6cb4d5e77e22fa55552a7cae936ab34ccL327

@YuriSizov
Copy link
Contributor

rect_min_size -> rect_minimum_size

Why this needs to be changed?

It's probably to match the internal methods, which were set/get_custom_minimum_size and became set/get_rect_minimum_size. I guess we can rename methods to set/get_rect_min_size instead.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 22, 2021

I guess we can rename methods to set/get_rect_min_size instead.

Yep, IMO that would be better. The PR is about renaming these methods anyways.

@madmiraal
Copy link
Contributor Author

rect_min_size -> rect_minimum_size

As suggested by @pycbouh, in this instance the property name was changed to better match the methods. Not just the exposed methods, but also Control's internal methods. Using minimum makes things consistent.

rectangle_minimum_size would be more correct/consistent

I agree that rectangle_* would be the better name, and I have changed the display name in the Inspector. However, unless there is some consensus on renaming Rect2 to Rectangle, it's probably better to keep it at rect for now.

property names take inspector space, so we shouldn't elongate them unnecessarily.

I agree, but Minimum Size is not currently Control's longest property name, and it fits in the default Inspector width; so I think it's okay as it is:
Control

@madmiraal madmiraal force-pushed the rename-control-getter-setter branch from 1a871ef to a7e08a4 Compare March 27, 2021 09:35
@madmiraal madmiraal requested a review from a team as a code owner March 27, 2021 09:35
@madmiraal
Copy link
Contributor Author

Rebased following merge of #38054, #45571 and #47166.

@madmiraal madmiraal force-pushed the rename-control-getter-setter branch from a7e08a4 to a9b1a9f Compare April 4, 2021 11:20
@madmiraal
Copy link
Contributor Author

Rebased following merge of #43155, #47230 and #47491.

@madmiraal madmiraal force-pushed the rename-control-getter-setter branch from a9b1a9f to b83de02 Compare April 5, 2021 16:08
@madmiraal
Copy link
Contributor Author

Rebased following merge of #46273 and ran --doctool ..

@madmiraal madmiraal force-pushed the rename-control-getter-setter branch from b83de02 to 1dc206c Compare April 17, 2021 12:04
@madmiraal
Copy link
Contributor Author

Rebased following merge of #37966 and #46340.

@madmiraal madmiraal force-pushed the rename-control-getter-setter branch from 1dc206c to 80d383e Compare May 1, 2021 10:40
@madmiraal
Copy link
Contributor Author

Rebased following merge of #46593 and #47448.

@YuriSizov
Copy link
Contributor

Given that some of these renames are obsolete because we renamed the properties instead, and that this PR has been inactive for over a year now, and we're at the end of the beta cycle, I'm going to close it. Thanks for your contribution nonetheless! Perhaps we'll be able to do more API cleanups in Godot 5 🙂

@YuriSizov YuriSizov closed this Jan 25, 2023
@YuriSizov YuriSizov removed this from the 4.0 milestone Jan 25, 2023
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.

4 participants