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

[UI] On Windows open the window in the same position as last time #2146

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

togenn
Copy link

@togenn togenn commented Apr 6, 2023

@@ -26,6 +26,11 @@
#include <ShellScalingApi.h>
#include <dwmapi.h>

DEFINE_uint32(window_position_x, 0,
"In windows the position to create the window to", "Display");
Copy link
Member

Choose a reason for hiding this comment

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

"Windows" must be capitalized here as it's a trademark starting with a capital letter, but overall this variable can be platform-independent, so there's no need to add a specific platform to the description. Possibly it may even be better to move it to the common Window code (declaration in window.h, definition in window.cc).

@@ -26,6 +26,11 @@
#include <ShellScalingApi.h>
#include <dwmapi.h>

DEFINE_uint32(window_position_x, 0,
"In windows the position to create the window to", "Display");
Copy link
Member

Choose a reason for hiding this comment

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

Please make a distinction between horizontal and vertical coordinates in the descriptions.


int window_x = cvars::window_position_x;
int window_y = cvars::window_position_y;
if (window_x == 0 && window_y == 0) {
Copy link
Member

@Triang3l Triang3l Apr 7, 2023

Choose a reason for hiding this comment

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

This comparison makes it impossible to save the 0, 0 position (top-left corner) for the next launch. I'd suggest some extreme values like INT_MIN or INT_MAX by default, and thus an || comparison rather than an && also, as the first launch will write the real values to the config anyway.

int window_x = cvars::window_position_x;
int window_y = cvars::window_position_y;
if (window_x == 0 && window_y == 0) {
window_x = CW_USEDEFAULT;
Copy link
Member

Choose a reason for hiding this comment

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

Is the behavior presented by this change desirable on Windows at all? I think Xenia shouldn't behave any different than other windowed apps as it's just yet another Windows windowed app essentially.

However, I see a mismatch between how Xenia works now (cycles between 4 stacked positions in top-left of the screen — interestingly, not even relative to the Explorer window it was launched from) and how other apps work. There aren't that many apps with legacy UI nowadays preinstalled on Windows so I don't have that many examples on my PC, but I've checked the Task Manager, as well as modern UI (yet not UWP) apps such as Paint, and old non-preinstalled apps like Microsoft Word 2007, and they preserve their positions.

I don't think all those apps save their window position explicitly, though I'm not entirely sure. But it's possible that we're not doing something necessary to use Windows's default behavior which looks like it's supposed to save the position?

Or does Windows not like that we're specifying the default size directly in the CreateWindowExW call, while it tries to save not only the position, but the size as well?… I don't know, this needs investigation.

Maybe letting the OS save the size would be nice too if we add a reset option to the menu, by the way. Though maybe it'd even be better to save the size via the config, so Windows doesn't reset it behind the scenes somehow, as Xenia is an emulator for running games, and you usually don't resize games at all once you've chosen your preferred resolution — games are not viewports for a region of some document, and you don't tile them with other windows on the screen for multitasking, instead, resizing changes pixel density and letterbox amount.

Copy link
Author

Choose a reason for hiding this comment

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

It seems that Windows doesn't save window position automatically. I tried to find some examples and I found out that at least notepad++ and classic notepad save their window position explicitly.

Notepad++ saves its position to config.xml:
<GUIConfig name="AppPosition" x="12" y="18" width="1100" height="700" isMaximized="no" />

Classic notepad saves its window position to registry Computer\HKEY_CURRENT_USER\Software\Microsoft\Notepad:
iWindowPosX, iWindowPosY

Copy link

Choose a reason for hiding this comment

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

Correct, windows does not save window position. But there is a thing you need to be careful about. If the position you are opening the window in, is outside of the current window boundary, you need to revert to the old behavior and save that. Like if someone changed their resolution. But even checking functions like GetSystemMetrics has it's caveats since you can't just check a point because the top left corner of the window could be out of the boundary but the rest of the screen could be ok.

Again, for 99% of users this doesn't apply, but the quirks can matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants