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 CanvasItem.update() to queue_redraw() #64377

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 13, 2022

Closes godotengine/godot-proposals#5190.

As brought up in #54161 (comment), #54161 (comment) and Should CanvasItem.update() be renamed to request_redraw() or redraw().

Affects a lot of classes. This PR is absolutely, utterly GARGANTUAN.

I very thoroughly checked signal connections and deferred calls to this method, add_do_method/add_undo_method calls, and so on. Just a few comments have also been changed to say "redraw". It's ENTIRELY possible I still missed something, but the engine compiles, at least!

Also renames the internal _update_callback() to _redraw_callback() for consistency.

In CPUParticles2D, there was a private variable with the same name. For now, this has been renamed to do_redraw. It's either that, or something close. I haven't exactly understood what it's meant for.

This PR was previously renaming it to redraw()

@YuriSizov
Copy link
Contributor

We've discussed this in a bikeshedding meeting, and were convinced that a rename would be beneficial here. But we suggest using queue_redraw as a name. It's probably worth renaming the internal function, _update_callback, to make it clear it's only for redrawing and not any other kind of updating.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 26, 2022

There are occasions where update() is actually called externally by a parent. For example:

void GenericTilePolygonEditor::set_polygon(int p_polygon_index, Vector<Point2> p_polygon) {
ERR_FAIL_INDEX(p_polygon_index, (int)polygons.size());
ERR_FAIL_COND(p_polygon.size() < 3);
polygons[p_polygon_index] = p_polygon;
button_edit->set_pressed(true);
base_control->update();
}
Vector<Point2> GenericTilePolygonEditor::get_polygon(int p_polygon_index) {
ERR_FAIL_INDEX_V(p_polygon_index, (int)polygons.size(), Vector<Point2>());
return polygons[p_polygon_index];
}
void GenericTilePolygonEditor::set_polygons_color(Color p_color) {
polygon_color = p_color;
base_control->update();
}

By the naming conventions I've seen, is it still a good idea to rename the internal method _update_callback?

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 26, 2022

There are occasions where update() is actually called externally by a parent.

Yes? That's pretty normal even for user scripts.

By the naming conventions I've seen, is it still a good idea to rename the internal method _update_callback?

What do you mean? We're renaming the public method to make it clearer what it does, so the internal one needs to be renamed as well to avoid any confusion for engine developers. The name would be something _redraw_callback, so I don't see any problem with that.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 26, 2022

Oh I see! I assumed the starting underscore was mostly reserved for virtual or private methods. I am a little confused on _update_callback

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 26, 2022

Oh I see! I assumed the starting underscore was mostly reserved for virtual or private methods.

_update_callback is and _redraw_callback should still remain a private method.

Again, public method is changed from update to queue_redraw. Its internal counterpart that is actually called to perform the redrawing is changed from _update_callback to _redraw_callback. Two methods are getting renamed.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 26, 2022

Oooh I see, I understand it all now. Thank you very much

@Mickeon Mickeon force-pushed the rename-canvas-redraw branch 2 times, most recently from dd0a9e5 to 6478db9 Compare August 26, 2022 13:18
@Mickeon Mickeon requested review from a team as code owners August 26, 2022 15:03
@Mickeon Mickeon requested review from a team as code owners August 26, 2022 15:03
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 26, 2022

image

Updated the PR to use queue_redraw. To be honest, I do still prefer redraw over it, because for being so commonly utilised in the codebase it may be bit annoying to see, and the documentation of this method I updated goes into greater detail on how it works, but I've seen a few methods around with similar names and they do actually redraw on the spot, so... I suppose it's not worth splitting hair for.

@Mickeon Mickeon force-pushed the rename-canvas-redraw branch 2 times, most recently from ce438a8 to 7c7216f Compare August 26, 2022 15:45
@Mickeon Mickeon changed the title Rename CanvasItem.update() to redraw() Rename CanvasItem.update() to queue_redraw() Aug 26, 2022
@Mickeon Mickeon force-pushed the rename-canvas-redraw branch 2 times, most recently from e23a360 to c666574 Compare August 28, 2022 10:23
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.

I am tentatively ready to approve this, but it needs another rebase and quick look in case you accidentally renamed stuff you shouldn't have or missed stuff that you should've renamed (though unlikely if you've only did the bulk of changes in various cpp files).

Edit: Also, you didn't rename _update_callback to _redraw_callback?

@YuriSizov
Copy link
Contributor

Updated the PR to use queue_redraw. To be honest, I do still prefer redraw over it, because for being so commonly utilised in the codebase it may be bit annoying to see, and the documentation of this method I updated goes into greater detail on how it works, but I've seen a few methods around with similar names and they do actually redraw on the spot, so... I suppose it's not worth splitting hair for.

I think it's important to indicate that the method has no immediate effect. We use queue_* for other method names as well, including widely utilized queue_free(), so it's not a foreign concept.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 29, 2022

Edit: Also, you didn't rename _update_callback to _redraw_callback?

I first forgot, then I thought of doing it in another PR but it doesn't hurt to just do it now at this point. Won't take too much.

@Mickeon Mickeon force-pushed the rename-canvas-redraw branch from c666574 to c027035 Compare August 29, 2022 12:53
Affects a lot of classes. Very thoroughly checked signal connections and deferred calls to this method, add_do_method/add_undo_method calls, and so on.

Also renames the internal `_update_callback()` to `_redraw_callback()` for consistency.

Just a few comments have also been changed to say "redraw".

In CPUParticles2D, there was a private variable with the same name. It has been renamed to `do_redraw`.
@Mickeon Mickeon force-pushed the rename-canvas-redraw branch from c027035 to e31bb5f Compare August 29, 2022 13:01
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 29, 2022

All done, I had checked one more time.

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.

Changeset seems good now. Didn't check every single line, but in principle it should be correct if the CI passes

@akien-mga akien-mga merged commit ae349d8 into godotengine:master Aug 30, 2022
@YuriSizov
Copy link
Contributor

image

akien-mga added a commit that referenced this pull request Aug 30, 2022
@Mickeon Mickeon deleted the rename-canvas-redraw branch September 6, 2022 16:52
evanwalsh added a commit to evanwalsh/godot-ply that referenced this pull request Sep 18, 2022
jarneson added a commit to jarneson/godot-ply that referenced this pull request Oct 22, 2022
* Godot 4.0 beta 1 import file updates

* API rename: `update` is now `queue_redraw`

Per godotengine/godot#64377

* API rename: `x2y` functions are now `x_to_y`

Per godotengine/godot#64367

* Update types: `UndoRedo` is now an internal of `EditorUndoRedoManager`

Per godotengine/godot#59564

* beta 3

Co-authored-by: Jeff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Rename CanvasItem's update() to redraw() for Godot 4
4 participants