-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
Add SurfaceWindow subclass, remove borrowed window logic #2830
base: main
Are you sure you want to change the base?
Conversation
If you remove the borrowing logic, you have to remove from_window from Renderer as well. |
Yeah I though of doing it but didn't because I didn't understand its purpose well enough. Thanks for letting me know I will remove it |
6d6e5f3
to
18795cf
Compare
cdef Renderer self = cls.__new__(cls) | ||
self._win = window | ||
if self._win._is_borrowed: | ||
self._is_borrowed=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This old code was incorrect anyway, because theoretically you might have a Window that isn't borrowed and end up with a borrowed Renderer. The question is why don't you use your existing reference to that renderer, but oh well.
If you can't borrow a Window though, borrowing a Renderer makes even less sense.
.tp_doc = DOC_WINDOW, | ||
.tp_methods = surfacewindow_methods, | ||
.tp_init = (initproc)surfacewindow_init, | ||
.tp_new = PyType_GenericNew, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like my comment got lost. This .tp_new is correct, but the superclass must be abstract now
So @ankith26 you first brought up the "window subclass" idea a few months ago and I've been tossing it around in my head since. You brought it up back in #2632 when thinking about a .surface property. I've been doing some thinking on it, and they are very interesting ideas, but I don't think we should implement either of them at this time. Reasoning:
Also if we ever implement this I think it would be good to do the subclasses in Python, rather than C. Makes it easier to write and maintain. (I think blubber brought this up at some point too, to give him some credit for this idea as well) |
I have a few things to say about Starbuck's comment, about the reasoning
About python subclassing, that's up to whoever implements it but for now it doesn't seem like it's too messy implemented in C. |
People are aware in theory, but I've already seen complaining about tiny things changing in between releases with _sdl2. But the users are not my only concern, we want to push the Window API sooner rather than later, and a big change to the API like this shouldn't be happening at the stage I would like us to be in its development.
Yes, I agree that pygame-ce is not just a wrapper, as people sometimes say. However, if we're not confident about doing something differently, it's not a good idea to do so from a maintenance perspective. Sticking with SDL concepts and SDL API design is a good insurance policy to make sure our stuff keeps working the way we expect. |
@Starbuck5 To conclude this, I basically agree and i think we should keep developing this and eventually release it with pygame 3.0 among other OOP related ideas like python event types. |
I basically agree with Starbuck. I don't think doing it like this would be that bad, but it probably will be worse than what we have. |
I have put this PR up for the 3.0.0 milestone. We don't have a consensus to do a change like this at this point so I'm fine with this PR being stalled for the foreseeable future, though I will leave this PR open so we can decide in the long term. For now, let's not halt |
This is a draft PR to get feedback on the API and idea, so I have not updated docs/tests/types
Related to issue #2603
How to test this PR?
Use the example written by @Starbuck5 in #2626, but have to just replace the two lines that construct
Window
and instead useSurfaceWindow