feat: remember position of game window #9853
Conversation
usidedown
force-pushed
the
feature/remember-window-pos
branch
3 times, most recently
from
15d83e0 to
7af36b3
Compare
usidedown
force-pushed
the
feature/remember-window-pos
branch
2 times, most recently
from
016dcfd to
8f8b6cf
Compare
usidedown
force-pushed
the
feature/remember-window-pos
branch
2 times, most recently
from
0e4e728 to
fbf8742
Compare
usidedown
force-pushed
the
feature/remember-window-pos
branch
from
fbf8742 to
0e0448b
Compare
oleg-derevenetz
added
improvement
There was a problem hiding this comment.
Hi @usidedown , I left few comments in this pull request. Would you mind to take a look at them when you have time?
| if ( pos == fheroes2::Point{ -1, -1 } ) { | ||
| _prevWindowPos = { SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED }; | ||
| } | ||
| else { | ||
| _prevWindowPos = pos; | ||
| } | ||
| } |
There was a problem hiding this comment.
We must have a check whether the position is within the screen area. Moreover, the position plus the size of the windows must be within the screen.
There was a problem hiding this comment.
I'm on the fence regarding these checks. They seem to be mostly useful if the user messes with fheroes2.cfg.
My main issue is that when setWindowPos is called _currentScreenResolution isn't set yet. Even if I postpone the check to engine::allocate, _currentScreenResolution is still not set.
Moreover, it's legal to be in a position where pos + size exceeds the screen size. Probably not useful even if you have multiple displays, but still valid.
What do you think?
There was a problem hiding this comment.
Assuming the following situation:
- make the resolution 1920x1080
- move the window to the left part of the display
- change resolution to 640x480
As a result, the whole window is going to be invisible and outside the display. At very least we need to have a mechanism if a user messes up and they would always blame us :)
Or I might be wrong as SDL handles such cases so we don't need to do anything.
There was a problem hiding this comment.
You're right!
I've implemented the check.
I believe it is better to check when the window is created rather then when the position is set to avoid race conditions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
usidedown
force-pushed
the
feature/remember-window-pos
branch
from
ecf3cb6 to
d2fa303
Compare
|
We will merge this pull request after the upcoming release. |
There was a problem hiding this comment.
Hi @usidedown , I put just few more minor comment. Could you please check them and see if they make sense?
src/engine/screen.cpp
Outdated
| @@ -825,6 +825,9 @@ namespace | |||
| flags |= SDL_WINDOW_FULLSCREEN_DESKTOP; | |||
| #endif | |||
|
|
|||
| // update the window position, in case we get queried for it | |||
There was a problem hiding this comment.
[Very minor] Since every other comment starts from a capital letter I suggest to do the same here.
| if ( pos == fheroes2::Point{ -1, -1 } ) { | ||
| _prevWindowPos = { SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED }; | ||
| } | ||
| else { | ||
| _prevWindowPos = pos; | ||
| } | ||
| } |
There was a problem hiding this comment.
Assuming the following situation:
- make the resolution 1920x1080
- move the window to the left part of the display
- change resolution to 640x480
As a result, the whole window is going to be invisible and outside the display. At very least we need to have a mechanism if a user messes up and they would always blame us :)
Or I might be wrong as SDL handles such cases so we don't need to do anything.
src/fheroes2/system/settings.cpp
Outdated
| @@ -398,6 +403,9 @@ std::string Settings::String() const | |||
| os << std::endl << "# video mode: in-game width x in-game height : on-screen width x on-screen height" << std::endl; | |||
| os << "videomode = " << display.width() << "x" << display.height() << ":" << display.screenSize().width << "x" << display.screenSize().height << std::endl; | |||
|
|
|||
| os << std::endl << "# position of the game's window" << std::endl; | |||
| os << "main window position = [ " << _windowPos.x << ", " << _windowPos.y << " ]" << std::endl; | |||
There was a problem hiding this comment.
Should we name the parameter as game window position as well?
Solves #1228
Saves the position of the window in fheroes2.conf.
The position is captured every time the window is moved, and saved to settings when the game exits.
It's possible to capture only when the game exits, but then we might not save the correct position if the user swaps to Fullscreen mode before quitting.