Add pygame.display.get/set_window_position() #2816

Damus666 · 2024-04-22T19:59:18Z

I implemented what is said in the title of the pull request plus tests, stubs and documentation, I tested locally the functions.

Why?

The window API should not have this feature restricted to it, without it seems incomplete
The SDL code to achieve this is minimal so I see no reason to exclude this functionality
The environment variable won't allow position changes after initialization and is nor pythonic nor clear
The window API currently doesn't support OpenGL among the many features, so adding this feature to the display API is important
Really, why not? It's not bloating nor complicating rather implementing existing and important functions provided by SDL

I'm very open to feedback and critics, I got inspired by the rest of display.c to write the implementation :)
Sorry for the double pull request, I messed up the previous branch, the current was forked directly from pygame-community/pygame-ce:main

…ocumentation

src_c/display.c

buildconfig/stubs/pygame/display.pyi

oddbookworm · 2024-04-22T22:41:15Z

src_c/display.c

+        return NULL;
+
+    if (win)
+        SDL_SetWindowPosition(win, x, y);


In SDL3, this will return something based on whether it was successful or not (SDL3 docs). @Starbuck5 @ankith26 @MyreMylar what do y'all think about this? Should there be some version checks built into new code that can support SDL3 more directly in the future, or should that be added in with other SDL3-compat stuff?

The same also applies to SDL_GetWindowPosition

I think we should just put a comment here for now saying that this function will start raising errors, and deal with it during the porting

docs/reST/ref/display.rst

Damus666 · 2024-04-24T10:50:08Z

Would re-running checks magically fix the Pypy exception (which was not raised by code edited by me)? I remember that happening with another pull request of mine. (how would I do that?)

oddbookworm · 2024-04-26T02:53:12Z

Would re-running checks magically fix the Pypy exception (which was not raised by code edited by me)? I remember that happening with another pull request of mine. (how would I do that?)

Potentially, a rerun would have worked. Sometimes the fails are intermittent and unrelated to the changes. A member can rerun (most) failed checks (I haven't figured out how to rerun circleci yet). You can force rerun all CI by closing and reopening the PR, but I don't like doing that if I don't have to

docs/reST/ref/display.rst

oddbookworm

LGTM! Tested locally on windows 11, and it works.

ankith26

I'm not strongly in favour or strongly against this PR, but I have left a few reviews for your consideration.

ankith26 · 2024-05-12T13:23:16Z

buildconfig/stubs/pygame/display.pyi

@@ -82,6 +82,8 @@ def get_caption() -> Tuple[str, str]: ...
 def set_palette(palette: Sequence[ColorValue], /) -> None: ...
 def get_num_displays() -> int: ...
 def get_window_size() -> Tuple[int, int]: ...
+def get_window_pos() -> Tuple[int, int]:...
+def set_window_pos(pos: Sequence[int]) -> None:...


Suggested change

def set_window_pos(pos: Sequence[int]) -> None:...

def set_window_pos(pos: Coordinate) -> None:...

typing it as a Coordinate is more generic and accurate

ankith26 · 2024-05-12T13:24:46Z

docs/reST/ref/display.rst

+   This differs from updating environment variables as this function can be called after the display has been initialised.
+   The position is expected to be relative to the topleft of the primary monitor.
+   The y coordinate will ignore the window frame (y = 0 means the frame is hidden).
+   The user will still be able to move the window after this call. See also :func:`pygame.display.get_window_pos()`.


I'd like to have it noted here that there is a chance that this function can just fail on some platforms/configurations (example: direct wayland on linux)
So we shouldn't encourage users to rely on this function.

I saw that the similar functions in window.c call the same SDL functions, doesn't that mean that they could also fail on those platforms/configurations?
Also when you say fail, what kind of exceptions are you talking about?

ankith26 · 2024-05-12T13:29:59Z

src_c/display.c

+        return NULL;
+
+    if (win)
+        SDL_SetWindowPosition(win, x, y);


I think we should just put a comment here for now saying that this function will start raising errors, and deal with it during the porting

Damus666 · 2024-05-12T13:48:05Z

@ankith26 I know it makes no sense, but I could be against my pull request. I made it because I knew the display API was gonna stay and I didn't understand why it was missing those functions. But then we started talking quite a lot about the window module.

From what I understood, the window API will be THE way of making the window. The display module will only contain monitor/screen related functions, meaning that set_mode, get_window_size, get_surface (I might be missing something) will be deprecated and only available with the Window object.
Feel free to correct me if what I understood is not planned, but if it was, having get/set_window_pos added and then immediately deprecated doesn't feel like a good move.

I use this comment as an occasion to ask you what's the future of the display/window API. If display will always remain as an option to create a singleton window, then I'm strongly in favor of my pull request (who would have guessed) but otherwise, then we should finish the window API and deprecate display functions related to the window.

NOTE: I'm still gonna modify it like you suggested as it makes a lot of sense, but I think this are important considerations.

ankith26 · 2024-05-12T13:52:44Z

I use this comment as an occasion to ask you what's the future of the display/window API. If display will always remain as an option to create a singleton window, then I'm strongly in favor of my pull request (who would have guessed) but otherwise, then we should finish the window API and deprecate display functions related to the window.

The display module will probably never be removed. Almost all pygame code written relies on it. What we will most likely do once the window API is finalized and published, is recommend users to write any new code in it. The old code that uses display will continue to work.

Damus666 · 2024-05-12T13:58:26Z

The display module will probably never be removed. Almost all pygame code written relies on it. What we will most likely do once the window API is finalized and published, is recommend users to write any new code in it. The old code that uses display will continue to work.

Thanks for the information.

novialriptide · 2024-05-18T02:09:16Z

buildconfig/stubs/pygame/display.pyi

@@ -82,6 +82,8 @@ def get_caption() -> Tuple[str, str]: ...
 def set_palette(palette: Sequence[ColorValue], /) -> None: ...
 def get_num_displays() -> int: ...
 def get_window_size() -> Tuple[int, int]: ...
+def get_window_pos() -> Tuple[int, int]:...


Why window_pos instead window_position?

When I wrote it I gave it that name because it was shorter but Window has the position attribute and not pos so it makes sense to have position. I'll edit this when I'm back in Italy in 14 hours

novialriptide · 2024-05-19T00:20:55Z

src_c/display.c

+
+    if (pos != NULL) {
+        if (!pg_TwoIntsFromObj(pos, &x, &y))
+            return RAISE(PyExc_TypeError, "pos must be two numbers");


Please change this as well.

My bad I didn't see it thanks

oddbookworm

Still looks good

MyreMylar

OK, I don't mind this going in.

It doesn't really create more work for the eventual plan of Window supplanting display because Window already supports this functionality. I guess it slightly takes a little shine of Window for having features that display doesn't. But eh, that's not a great reason not to do it at the minute.

If display eventually becomes a python shim around the new Window module then this will only add a tiny bit more code to that shim.

Add pygame.display.get/set_window_pos() functions, tests, stubs and d…

3409755

…ocumentation

Damus666 requested a review from a team as a code owner April 22, 2024 19:59

oddbookworm requested changes Apr 22, 2024

View reviewed changes

Damus666 added 2 commits April 23, 2024 08:31

implement review comments (fix + docs update + parameter generalization)

9f823d5

fix format and stubs

cbe3f1a

Use faster tuple[int, int] builder

785497e

oddbookworm requested changes Apr 27, 2024

View reviewed changes

docs/reST/ref/display.rst Outdated Show resolved Hide resolved

Make docs slightly clearer

270727b

oddbookworm approved these changes Apr 27, 2024

View reviewed changes

yunline added New API This pull request may need extra debate as it adds a new class or function to pygame display pygame.display labels May 9, 2024

ankith26 reviewed May 12, 2024

View reviewed changes

Comment and stubs update

675e0dc

novialriptide reviewed May 18, 2024

View reviewed changes

Damus666 changed the title ~~Add pygame.display.get/set_window_pos() functions, tests, stubs and documentation~~ Add pygame.display.get/set_window_position() May 18, 2024

Rename to window_position

8cf4cf3

novialriptide reviewed May 19, 2024

View reviewed changes

Rename pos in exception message

cc4f6bb

oddbookworm approved these changes May 20, 2024

View reviewed changes

MyreMylar approved these changes May 24, 2024

View reviewed changes

oddbookworm added this to the 2.5.0 milestone May 25, 2024

oddbookworm merged commit 582b639 into pygame-community:main May 25, 2024
39 checks passed

Damus666 deleted the display-getset-window-pos branch May 26, 2024 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pygame.display.get/set_window_position() #2816

Add pygame.display.get/set_window_position() #2816

Damus666 commented Apr 22, 2024 •

edited

oddbookworm Apr 22, 2024

ankith26 May 12, 2024

Damus666 commented Apr 24, 2024

oddbookworm commented Apr 26, 2024

oddbookworm left a comment

ankith26 left a comment

ankith26 May 12, 2024

ankith26 May 12, 2024

Damus666 May 12, 2024 •

edited

ankith26 May 12, 2024

Damus666 commented May 12, 2024

ankith26 commented May 12, 2024

Damus666 commented May 12, 2024

novialriptide May 18, 2024

Damus666 May 18, 2024

novialriptide May 19, 2024

Damus666 May 19, 2024

oddbookworm left a comment

MyreMylar left a comment

	def set_window_pos(pos: Sequence[int]) -> None:...
	def set_window_pos(pos: Coordinate) -> None:...

Add pygame.display.get/set_window_position() #2816

Add pygame.display.get/set_window_position() #2816

Conversation

Damus666 commented Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Damus666 commented Apr 24, 2024

oddbookworm commented Apr 26, 2024

oddbookworm left a comment

Choose a reason for hiding this comment

ankith26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Damus666 May 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Damus666 commented May 12, 2024

ankith26 commented May 12, 2024

Damus666 commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oddbookworm left a comment

Choose a reason for hiding this comment

MyreMylar left a comment

Choose a reason for hiding this comment

Damus666 commented Apr 22, 2024 •

edited

Damus666 May 12, 2024 •

edited