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

x11: don't unconditionally move the window on geometry changes #14938

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

Dudemanguy
Copy link
Member

9e81fc9 started doing this and then later when force update notifications were added it became even more apparent with issues like #14911. In retrospect, this behavior change is not really intuitive (nobody would reasonably expect --geometry=50% to also center the window) and actually completely avoidable. vo_calc_window_geometry3 already flags for us if the window should be moved either via the opts->force_window_position or if there is a valid xy specified in the geometry option. All we need to do is check the flags for VO_WIN_FORCE_POS and use that for determining whether or not an actual window move should be done.

@Dudemanguy Dudemanguy changed the title x11: don't unconditionally move the window geometry changes x11: don't unconditionally move the window on geometry changes Sep 27, 2024
@kasper93
Copy link
Contributor

For w32 it can be done too

diff --git a/video/out/w32_common.c b/video/out/w32_common.c
index 5c9cd2cb93..a899222d18 100644
--- a/video/out/w32_common.c
+++ b/video/out/w32_common.c
@@ -1171,7 +1171,7 @@ static void update_window_state(struct vo_w32_state *w32)

     SetWindowPos(w32->window, w32->opts->ontop ? HWND_TOPMOST : HWND_NOTOPMOST,
                  wr.left, wr.top, rect_w(wr), rect_h(wr),
-                 SWP_FRAMECHANGED | SWP_NOACTIVATE | SWP_NOOWNERZORDER);
+                 SWP_FRAMECHANGED | SWP_NOACTIVATE | SWP_NOOWNERZORDER | (!w32->win_force_pos ? SWP_NOMOVE : 0));

     // Unmaximize the window if a size change is requested because SetWindowPos
     // doesn't change the window maximized state.

Though maybe it would be better to actually calculate the position in geometry? This would allow to keep window center fixed, instead of top-left corner.

This is what changing window-scale does. I don't think there is an issue in VO. It should be updated in geometry code to not center.

@Dudemanguy
Copy link
Member Author

Dudemanguy commented Sep 27, 2024

Everything on x11 and wayland resizes with respect to the top left corner including window-scale. Not really sure trying to change that would be worth it.

Edit: Actually I forgot. On x11 you can set --force-window-position and that will keep it centered while resizing. Except it seems it doesn't calculate the right coordinates when used with --geometry and always goes right to the middle. Let me see if I can fix that.

@na-na-hi
Copy link
Contributor

na-na-hi commented Sep 27, 2024

Moving the window to the center of the screen when x/y aren't specified is caused by #3165, commit c4e8c36.

Apparently this behavior is wanted by #2397.

@Dudemanguy
Copy link
Member Author

Seems weird. imo this should just be controlled by --force-window-position or you can simply pass +50%+50% to your geometry command.

There's really no reason to have all these extra variants. It's not like
this is public API. Collapse it all into one vo_calc_window_geometry
function and callers can simply just pass the appropriate parameters to
get the same behavior as before. I'm about to edit this function again
in a future commit and I really don't want to make a
vo_calc_window_geometry4 for this so let's clean it up.
c4e8c36 made any usage of --geometry
implicitly center the window on the screen after a resize even if the
user did not pass any x/y arguments to the option. At the time, this was
probably wanted since --geometry was primarily a startup option and
likely the window wouldn't be centered on x11 without moving
coordinates. Times have changed since then and we support full runtime
--geometry changes on all the relevant platforms but it comes with this
automatic window centering behavior (except on wayland of course hah).

It's better to make such window centering optional and user controllable
since it is entirely reasonable that someone wants --geometry=50% to
just resize and not move anything. It's already trivial for a person
that does want to move the window to just add their coordinates to the
--geometry command so there's no reason to continue to force this
behavior since it is less flexible.

Instead, move the window centering stuff out of m_geometry_apply into
vo_calc_window_geometry. We give the power to the caller to whether or
not to force centering the window here and all usage of the function is
updated to simply call it with false for now. Additionally,
--force-window-position being set will also center the window like
before. All that is left is for the windowing code to take advantage of
this. See subsequent commits.
With the pieces set in place with the previous changes, we can implement
the desired behavior. The trick here is that we do want to force
centering the window when mpv first initially starts and the window size
is not known yet. This can be done by simply checking
x11->pseudo_mapped. The other change is to just look if we have the
VO_WIN_FORCE_POS flag and feed that to highlevel resize.
@Dudemanguy
Copy link
Member Author

I ended up reworking how this whole centering mechanism worked and put it in the hands of the caller of vo_calc_window_geometry. So the windowing code is now in charge of whether or not it wants centered coordinates and if they should be used. Only implemented it in x11 of course.

@kasper93 and @Akemi: feel free to push whatever small changes are needed for windows and mac to this branch. It's a behavior change but I think a strictly better one.

Copy link

github-actions bot commented Sep 28, 2024

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy
Copy link
Member Author

Apparently windows does not need any further changes so will merge later today.

@kasper93
Copy link
Contributor

kasper93 commented Oct 5, 2024

Apparently windows does not need any further changes so will merge later today.

Fine with me. Indeed on Windows it was working ok last time I tried. There are some unrelated to this PR outstanding issues there, but I plan to tackle them separately.

@Dudemanguy Dudemanguy merged commit 9791c6d into mpv-player:master Oct 5, 2024
25 checks passed
@Dudemanguy Dudemanguy deleted the x11-geometry-move branch October 5, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants