Refactor getting the usable parent screen #107459
Conversation
| parent_rect = DisplayServer::get_singleton()->screen_get_usable_rect(DisplayServer::get_singleton()->window_get_current_screen(w->get_window_id())); | ||
| // The DisplayServer doesn't allow resizing so there is no parent that we could resize into. | ||
| if (window_id != DisplayServer::INVALID_WINDOW_ID && DisplayServer::get_singleton()->has_feature(DisplayServer::FEATURE_SELF_FITTING_WINDOWS)) { | ||
| return Rect2i(position, size); |
There was a problem hiding this comment.
I'm surprised that it works :O
That said I'm not 100% convinced of this approach. Like, right now it gets stuck in place pretty much but I can see whatever future user of this method using it for other reasons and getting pretty confused about it.
IMO we should avoid calling this method in the first place when possible.
There was a problem hiding this comment.
I suppose it all depends on what this method is meant to return. I have interpreted it to mean the size of the rect which we can expand into which if SELF_FITTING_WINDOWS is implemented is that we can't expand at all.
If that's not what this method is meant to mean then even if we reject the PR we should add documentation that explains what this is actually meant to return
There was a problem hiding this comment.
I mean it's called get_usable_parent_rect. My interpretation is that it should return the whole space in which the child window can be placed. This would include the middle of the parent for example, so I can see this being useful for more than simple window fitting to the screen.
Dunno, again that's just my interpretation. It definitely feels like changing behavior a bit too much to me.
There was a problem hiding this comment.
I though that was my interpretation too, that if SELF_FITTING_WINDOWS is true then we can't actually move/resize the window so it's current size is already the entire size it can fit in.
In other DisplayServers I expected it would be used to place the window (like say centering the window), to do that we would need to have the space it can be placed in. If the window has already been created and can't move then we can't reposition but if the native window hasn't need created yet (window_id == INVALID_WINDOW_ID) then we still have a chance of setting the position to anywhere in the containing parent even under Wayland.
There was a problem hiding this comment.
I guess since when SELF_FITTING_WINDOWS is implemented fitting/positioning would just always fail we could just make calling the method when the displayserver implements SELF_FITTING_WINDOWS an error and fix all instances of it being used.
Would that be an approach that sits better with you?
There was a problem hiding this comment.
that if SELF_FITTING_WINDOWS is true then we can't actually move/resize
That's the thing though, it can. It simply can't reliably try to fit it to the screen itself but it can position it freely everywhere.
we could just make calling the method when the displayserver implements SELF_FITTING_WINDOWS an error and fix all instances of it being used.
Yeah it would be a nice assertion I suppose, if the relevant maintainers are ok with that.
to be completely honest I have a slight feeling that this whole INVALID_SCREEN thing might make the whole feature flag redundant. I'd like to take a stab myself if you don't mind.
There was a problem hiding this comment.
Completely reasonable, this was just my first idea as to how to fix it and you're in a better position to know what the problem is and how it should be fixed
| // If there is no native window we don't know what screen it will be on or what its size is. | ||
| // Just hope that the popup will appear on the same screen as the main one. |
There was a problem hiding this comment.
If we have position set, we can get screen from it (except Wayland).
| } | ||
| return parent_rect; | ||
|
|
||
| // Internal popups should not overflow from the parent window. |
There was a problem hiding this comment.
There's no reason for this limitation, popups can and do overflow parent all the time. If they could not, there would be zero point of supporting native window popups at all.
|
It seems the added checks aren't actually useful and are just harmful then. Unless some of the added checks really are useful/could be reworked to be useful I think just closing and looking for a better way of fixing #107438 would be better. |
Fixes #107438
Reworked
get_usable_parent_rectto do more checks and try a bit harder before just getting the screen rect.Now all window popup methods rely on this method instead of duplicating the work to find their parent rect.
The popup check is intended for dropdowns and such so if this isn't the correct check I'll need to fix that.