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.

Sign up for GitHub

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

Jump to bottom

Implemented (F)Rect.move_to #2165

Merged
merged 2 commits into from
Oct 7, 2023
Merged

Implemented (F)Rect.move_to #2165

merged 2 commits into from
Oct 7, 2023

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented May 16, 2023

Fixes #2156

@Matiiss Matiiss requested a review from a team as a code owner May 16, 2023 18:16
@Matiiss
Copy link
Member Author

Matiiss commented May 17, 2023

Should this also support

.move_to(x)
.move_to(x, y)

?

itzpr3d4t0r
itzpr3d4t0r reviewed May 17, 2023
View reviewed changes
src_c/rect_impl.h Outdated Show resolved Hide resolved
@itzpr3d4t0r itzpr3d4t0r added New API This pull request may need extra debate as it adds a new class or function to pygame rect pygame.rect labels May 17, 2023
@yunline
Copy link
Contributor

yunline commented May 18, 2023

Does this function duplicate with Rect.move()? Is it just a keyword version move()?

@Matiiss
Copy link
Member Author

Matiiss commented May 18, 2023

@yunline

Does this function duplicate with Rect.move()? Is it just a keyword version move()?

No, move returns a rectangle moved by the given values, this returns a rectangle moved to the given position while also supporting keywords for various rectangle attributes. Similar to Surface.get_rect in some regard.

@yunline
Copy link
Contributor

yunline commented May 19, 2023 •
edited
Loading

Should this also support

.move_to(x)
.move_to(x, y)

?

I think that would be yes, to be consistent with move() and move_ip()

>>> r=Rect(0,0,1,1)  
>>> r.move(1,1)
Rect(1, 1, 1, 1)
>>> r.move((1,1)) 
Rect(1, 1, 1, 1)

@Matiiss
Copy link
Member Author

Matiiss commented May 19, 2023

@yunline
So, the single positional argument if you pass in a tuple is implemented already, the question was about
move_to(1) (single float/int and will return a new rect whose x coord is this value)
and move_to(1, 1) (which you seem to support)

yunline
yunline requested changes May 19, 2023
View reviewed changes
buildconfig/stubs/pygame/rect.pyi Outdated Show resolved Hide resolved
buildconfig/stubs/pygame/rect.pyi Outdated Show resolved Hide resolved
yunline
yunline approved these changes May 19, 2023
View reviewed changes
Copy link
Contributor

@yunline yunline left a comment

Choose a reason for hiding this comment

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

LGTM

@Matiiss
Copy link
Member Author

Matiiss commented May 25, 2023

After thinking a bit more about it

Should this even have topleft and x, y as the default positional arguments, I mean, it could currently be kwarg only the same way get_rect is, positional args could be added later anyways if necessary without breaking backwards compat

@Matiiss Matiiss changed the title Implemented (F)Rect.move_to Implemented (F)Rect.move_to May 29, 2023
@Matiiss Matiiss marked this pull request as draft May 29, 2023 20:49
@Matiiss
Copy link
Member Author

Matiiss commented May 29, 2023

Draft while removing topleft and x, y positional args for simplicity

@Matiiss Matiiss marked this pull request as ready for review May 31, 2023 17:07
itzpr3d4t0r
itzpr3d4t0r approved these changes Jul 1, 2023
View reviewed changes
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

Aside from these nits code looks fine to me!

src_c/rect_impl.h Outdated Show resolved Hide resolved
src_c/rect_impl.h Outdated Show resolved Hide resolved
itzpr3d4t0r
itzpr3d4t0r reviewed Jul 7, 2023
View reviewed changes
src_c/rect_impl.h Outdated Show resolved Hide resolved
@Matiiss Matiiss force-pushed the matiiss-add-rect-move_to branch from 9355e49 to 5b688b0 Compare July 7, 2023 10:01
itzpr3d4t0r
itzpr3d4t0r reviewed Jul 10, 2023
View reviewed changes
src_c/rect_impl.h Outdated Show resolved Hide resolved
@Matiiss Matiiss force-pushed the matiiss-add-rect-move_to branch from 4422c98 to 25f582c Compare July 17, 2023 23:15
@Matiiss Matiiss force-pushed the matiiss-add-rect-move_to branch 3 times, most recently from 7a595fe to cd03d1b Compare July 29, 2023 08:44
@Matiiss
Added rect.move_to
004e89e 
Change size getter

Reduced indentation, updated versionadded

formatting...

Removed unused variable

Fixed memory leaks (and added a semicolon)

Changed stuff to only accept keyword arguments

regen docs and formatting (for the billionth time already...)

removed redundant call signatures

should've done this from the beginning

Should be fixed now, more test cases

formatting...

Type casting

Implementation changes

Doc changes

Improved performance using METH_FASTCALL | METH_KEYWORDS

Call PyTuple_GET_SIZE only once

Updated docs with versionadded

Changed function types

Update rect.pyi

updated error message

Update rect.pyi
@Matiiss Matiiss force-pushed the matiiss-add-rect-move_to branch from 85c0d6e to 004e89e Compare July 29, 2023 09:00
Starbuck5
Starbuck5 reviewed Aug 27, 2023
View reviewed changes
docs/reST/ref/rect.rst Outdated Show resolved Hide resolved
MyreMylar
MyreMylar approved these changes Oct 7, 2023
View reviewed changes
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

This all seems fine to me 👍

@MyreMylar MyreMylar merged commit 1719bcc into pygame-community:main Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itzpr3d4t0r itzpr3d4t0r itzpr3d4t0r approved these changes

@Starbuck5 Starbuck5 Starbuck5 left review comments

@yunline yunline yunline approved these changes

@MyreMylar MyreMylar MyreMylar approved these changes

Assignees
No one assigned
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame rect pygame.rect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement a Rect.move_to() and FRect.move_to() function
5 participants
@Matiiss @yunline @MyreMylar @Starbuck5 @itzpr3d4t0r