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.

Sign up for GitHub

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

Jump to bottom

[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
Open

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d838bf
[UI] On Windows open the window in the same position as last time
togenn Apr 6, 2023
d3fcde2
Merge branch 'xenia-project:master' into master
togenn Apr 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 31 additions & 2 deletions src/xenia/ui/window_win.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

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.

DEFINE_uint32(window_position_y, 0,
"In windows the position to create the window to", "Display");

namespace xe {
namespace ui {

Expand Down Expand Up @@ -125,20 +130,35 @@ bool Win32Window::OpenImpl() {
dpi_, USER_DEFAULT_SCREEN_DPI));
AdjustWindowRectangle(window_size_rect, window_style,
BOOL(main_menu != nullptr), window_ex_style, dpi_);

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 •
edited

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.

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.

window_y = CW_USEDEFAULT;
}

// Create the window. Though WM_NCCREATE will assign to `hwnd_` too, still do
// the assignment here to handle the case of a failure after WM_NCCREATE, for
// instance.
hwnd_ = CreateWindowExW(
window_ex_style, L"XeniaWindowClass",
reinterpret_cast<LPCWSTR>(xe::to_utf16(GetTitle()).c_str()), window_style,
CW_USEDEFAULT, CW_USEDEFAULT,
window_size_rect.right - window_size_rect.left,
window_x, window_y, window_size_rect.right - window_size_rect.left,
window_size_rect.bottom - window_size_rect.top, nullptr, nullptr,
hinstance, this);
if (!hwnd_) {
XELOGE("CreateWindowExW failed");
return false;
}

// If saved window position is not in any monitor anymore, move window to
// the primary display
if (!MonitorFromWindow(hwnd_, MONITOR_DEFAULTTONULL)) {
MoveWindow(hwnd_, 0, 0, window_size_rect.right - window_size_rect.left,
window_size_rect.bottom - window_size_rect.top, false);
}

// For per-monitor DPI, obtain the DPI of the monitor the window was created
// on, and adjust the initial normal size for it. If as a result of this
// resizing, the window is moved to a different monitor, the WM_DPICHANGED
Expand Down Expand Up @@ -1000,6 +1020,15 @@ LRESULT Win32Window::WndProc(HWND hWnd, UINT message, WPARAM wParam,
DeleteTimerQueueTimer(nullptr, cursor_auto_hide_timer_, nullptr);
cursor_auto_hide_timer_ = nullptr;
}
{
// Save window position
WINDOWINFO window_info{};
window_info.cbSize = sizeof(window_info);
if (GetWindowInfo(hwnd_, &window_info)) {
OVERRIDE_uint32(window_position_x, window_info.rcWindow.left);
OVERRIDE_uint32(window_position_y, window_info.rcWindow.top);
}
}
{
WindowDestructionReceiver destruction_receiver(this);
OnBeforeClose(destruction_receiver);
Expand Down