Closed falbrechtskirchinger closed 1 year ago
Looks good but upon a final look it isn't immediately obvious why we have lastPos
and restorePos
, i.e. their difference.
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
?
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
So wouldn't we get the same effect by storing the value stored into
restorePos
intolastPos
instead and removingrestorePos
?
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.
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.
- 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.
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?
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. :wink: 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
tolastPos
instead? We could even use values of0
andINT_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?
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 saverestorePos
to the settings.
I see. Quite a lot of edge cases here, so thanks for tackling that!
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*
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. :-(
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 theSetMinimized
for? There is no assertion inside the function and the next check is guarded byBOOST_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.
BOOST_TEST_CONTEXT
. :man_facepalming: BOOST_TEST_INFO
and didn't replace it with BOOST_TEST_MESSAGE
. Didn't like the clutter.Woops, I wanted to enable auto-merge as Appveyor is so slow. Well, I guess/hope it is fine ;-)
Thanks for your efforts!
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!
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:
savedPos
.