Improve in-game window positioning

[falbrechtskirchinger]

Improve window positioning in two ways:

1. Remember the window position set by a move and try to restore this position after every resize.
2. If the window is moved to a screen edge, save the width or height of the screen – depending on the edge – instead. This makes the window "stick" to the edge when resizing.

The images below depict the window position between clicking the resize button.

Before:

After:

To-do:

• Add unit test.
• If a window contacts a screen edge, always make it stick to that edge. (=> Window behaves correctly when shaded.)
• Rebase after Refactor PersistentWindowSettings and persist window minimized state #1609.
• Find a better name than savedPos.

[Flamefire]

Looks good but upon a final look it isn't immediately obvious why we have lastPos and restorePos, i.e. their difference.

1. Isn't the map window the only one which can be resized, i.e. for every other window the 2 positions are always the same?
2. Can't we fold restorePos into lastPos? I mean the main change here is the edge-handling when storing the position, isn't it?

So wouldn't we get the same effect by storing the value stored into restorePos into lastPos instead and removing restorePos?

[Spikeone]

1. Isn't the map window the only one which can be resized, i.e. for every other window the 2 positions are always the same?

You can resize the observation window as well

[falbrechtskirchinger]

So wouldn't we get the same effect by storing the value stored into restorePos into lastPos instead and removing restorePos?

The code evolved quite a bit, so let me think about that a bit. I know there were edge cases that didn't work initially.

Edit: Yes. The observation window with its multiple sizes was the issue. restorePos and lastPos can diverge. I just checked and I managed to re-break the code. It's not working as intended right now.

[falbrechtskirchinger]

I removed the restorePos member forgetting that the observation window doesn't persist. The fix is to bring that member back, then we can remove the setting and update lastPos accordingly instead.

Hopefully, there isn't another edge case I'm forgetting. This seemingly trivial feature isn't quite so easy to get right.

[falbrechtskirchinger]

1. Isn't the map window the only one which can be resized, i.e. for every other window the 2 positions are always the same?

In addition to the map and observation window, every non-modal window can be shaded/minimized, which is also a resize.

[Flamefire]

In addition to the map and observation window, every non-modal window can be shaded/minimized, which is also a resize.

So currently moving a window to a border and minimizing it (I prefer that word as it seems more conventional for windows) would move the minimized window even closer to the border?
Does this really make sense?

In any case: The actual behavior change intended here is clipping the window to borders if they were clipped before, correct? I.e. the change in SetIwSize

Can't we have it simpler: we get an adjusted position there before the resize which will clip the window correctly again after the resize and not store anything in the config? And/or to persist the clipping over restarts do the modification to newRestorePos to lastPos instead? We could even use values of 0 and INT_MAX for the left/top and right/bottom positions to ensure that the restored window after resolution change is still clipped.

Or am I misunderstanding anything?

[falbrechtskirchinger]

So currently moving a window to a border and

minimizing it (I prefer that word as it seems more conventional for windows)

Windows® (capital W, registered trademark), sure. Mac and Linux users – depending on the desktop environment – will disagree with you. I have known the function of rolling the window into its title bar as "shade" since the late 90s when I was using MacOS (pre-MacOS X, so, may not be a thing today). It's still available on KDE, though.
And being a pedantic a**hole, I take great offense at calling it "minimize" when there's a more accurate term. 😉
The BlueByte devs – going by the icon name – seem to have called the feature "iconify". They didn't know either…

would move the minimized window even closer to the border? Does this really make sense?

I think so. But I concede that this is a less straightforward and more opinionated feature than I assumed.

In any case: The actual behavior change intended here is clipping the window to borders if they were clipped before, correct? I.e. the change in SetIwSize

Not quite. Critically, it's about restoring the position the user last dragged the window to. Which is subtly different for windows with more than two resize states (map and observation window).

If a window is placed close to an edge, but not yet in clipping distance, following a resize that puts it in clipping distance lastPos and restorePos diverge. On the next resize information is lost when only keeping one value.

Can't we have it simpler: we get an adjusted position there before the resize which will clip the window correctly again after the resize and not store anything in the config? And/or to persist the clipping over restarts do the modification to newRestorePos to lastPos instead? We could even use values of 0 and INT_MAX for the left/top and right/bottom positions to ensure that the restored window after resolution change is still clipped.

INT_MAX sounds like a good idea and I have assigned newRestorePos to lastPos, as you suggested, which results in (IMO) incorrect behavior.

Or am I misunderstanding anything?

In summary, yes. Even though it's only relevant for one window (the observation window isn't persistent and storing restorePos as a member works), to get the desired behavior, we'd need to save restorePos to the settings.

Let's hear some more opinions. Do we want windows to snap to edges – including when "minimized" (ugh) – and do we care about the very subtle difference I've explained?

[Flamefire]

I have known the function of rolling the window into its title bar as "shade" since the late 90s when I was using MacOS (pre-MacOS X, so, may not be a thing today). It's still available on KDE, though.

Didn't know that, only went by the only name I've read for this and what was used throughout this project before. So thanks for the explanation where this name comes from. Makes sense as "minimized" may mean "smaller" which is more general than the folding into the title bar.

Or am I misunderstanding anything?

In summary, yes. Even though it's only relevant for one window (the observation window isn't persistent and storing restorePos as a member works), to get the desired behavior, we'd need to save restorePos to the settings.

I see. Quite a lot of edge cases here, so thanks for tackling that!

[falbrechtskirchinger]

Alright. I've implemented the "correct" behavior as envisioned and greatly expanded the unit test to cover the edge cases.

Bonus: Point gained two members Min/MaxElementValue to simplify 9 uses of std::numeric_limits<Point::ElementType>.

I'll do one more round of manual testing and mark this PR as ready for review if all goes well. *knock on wood*

[Flamefire]

Good job with the tests! I added a few improvement suggestions using contexts instead of unconditional messages which may make the test log harder to read

[falbrechtskirchinger]

I had to add braces for formatting (IfMacros is a clang-format 13 addition). Also, BOOST_TEST_CONTEXT expands to an if. So your initial suggestion didn't quite work, but thanks for pointing me in the right direction. I'm a Catch2/doctest user myself.

Improved output with BOOST_TEST(false):

tests/s25Main/UI/testIngameWindow.cpp(398): info: check wnd.GetSize() == Extent(wndSizeS.x, minHeight) has passed
Assertion occurred in a following context:
    Non-persisted window, posAtMouse
    Decrease size, window no longer connects with screen edges and should move to restorePos
    Minimized
tests/s25Main/UI/testIngameWindow.cpp(399): error: in "IngameWindows/WindowPositioning": check false has failed
Failure occurred in a following context:
    Non-persisted window, posAtMouse
    Decrease size, window no longer connects with screen edges and should move to restorePos
    Minimized

Edit: And clang-format-17 formats differently than v10. :-(

[Flamefire]

I had to add braces for formatting (IfMacros is a clang-format 13 addition). Also, BOOST_TEST_CONTEXT expands to an if. So your initial suggestion didn't quite work, but thanks for pointing me in the right direction. I'm a Catch2/doctest user myself.

A true. I meant to omit the semicolon too: BOOST_TEST_CONTEXT("Before minimize/un-minimize") main(); would also work (best with a newline but that might be confusing), but let's not get too detailed.

Edit: And clang-format-17 formats differently than v10. :-(

Note that there is a build target "clangFormat" which formats modified files (e.g. make clangFormat -j) which is created if the correct clang-format version is found. Maybe that helps.

What is the BOOST_INFO in front of the SetMinimized for? There is no assertion inside the function and the next check is guarded by BOOST_TEST_CONTEXT. So I'd rather remove that

[falbrechtskirchinger]

Edit: And clang-format-17 formats differently than v10. :-(

Note that there is a build target "clangFormat" which formats modified files (e.g. make clangFormat -j) which is created if the correct clang-format version is found. Maybe that helps.

I'm using that already, thanks! My default is just to format from the editor using clang-format in PATH and it usually produces compatible results.

What is the BOOST_INFO in front of the SetMinimized for? There is no assertion inside the function and the next check is guarded by BOOST_TEST_CONTEXT. So I'd rather remove that

I think I meant to use BOOST_TEST_MESSAGE there. I just wanted something in the log about these key steps.

Edit: Oh, and CI fails now because BOOST_TEST_INFO_SCOPE is a 1.70.0 addition. Let me see how best to resolve that.

[falbrechtskirchinger]

• Removed the braces. (clang-format-17 seems to like that.)
• Added a compatibility macro for Boost <1.70. Replaced with BOOST_TEST_CONTEXT. 🤦‍♂️
• Removed BOOST_TEST_INFO and didn't replace it with BOOST_TEST_MESSAGE. Didn't like the clutter.

[Flamefire]

Woops, I wanted to enable auto-merge as Appveyor is so slow. Well, I guess/hope it is fine ;-)

Thanks for your efforts!

[falbrechtskirchinger]

Woops, I wanted to enable auto-merge as Appveyor is so slow. Well, I guess/hope it is fine ;-)

I'm here to fix things if anything breaks. (Found one bug in a prior PR already.)

Thanks for your efforts!

You're very welcome! Thanks for reviewing!