Add Windows implementation of setTitlebarVisible (#75)

[Quezion]

Opening as Windows impl of #75. First commit has working setTitlebarVisible in JWM's demo.

I was going to implement more methods but setTitle(title) is already implemented & works on my machine. The rest of the methods Tonsky mentions in #75 (comment) are Mac specific & may not have clear Windows analogues.

For my own usage I only hide the titlebar. I'm happy to leave Windows style extension to a future PR, but thoughts @tonsky ?

TODOs

• Additional methods? (might be left to future WinStyle PR)
• Request review from https://github.com/littleli once PR is complete

Bugs

• Window contents are blurry after resize
  • HumbleUI is not recognizing increase in app pane's draw area that occurs after removing titlebar
  • Need to dispatch or process event in HumbleUI to update render resolution
    Can probably solve this by handling WM_STYLECHANGED event in jwm::WindowWin32::processEvent switch
    see https://learn.microsoft.com/en-us/windows/win32/winmsg/wm-stylechanged
  • Fixed by running setContentSize in WM_STYLECHANGED, but has logical side-effect of growing the window by the titlebar-height on every other toggle
    • Should we try to calculate the Windows titlebar height & remove it from the content size when the titlebar is restored? @tonsky

Test Instructions

Assuming you have Windows local build setup & you're in the VS x64 command prompt:

1. python script/build.py && python script/run.py
2. Input keyboard shortcut Ctrl+T in JWM's demo window to Toggle Titlebar. The titlebar should disappear from the top of the window.
3. Run Toggle Titlebar again & the default titlebar should reappear

Note that I've only tested this on Windows 10.

[littleli]

It took me quite a while to successfully setup the dev environment and compile the project. Now I can confirm that after Ctrl+T Toggle title bar function hides (and show) window title bar.

This works both in normal application mode and full screen mode (after Ctrl+F). I can also confirm that size of the window after toggle twice increase the window size with the size of title bar. I recommend to consider compensate height as suggested by @Quezion.

[tonsky]

I also checked the PR and it works great!

Should we try to calculate the Windows titlebar height & remove it from the content size when the titlebar is restored? @tonsky

I think so, yes. As much as it’s possible.

Reasistically people will also probably set this once after creating a window, but the problem is, it happens on the very first toggle (create -> hide title bar). I’d prefer that it doesn’t change size here

[Quezion]

Fixed the "window size increasing" behavior, updated windows/build.md with some findings from @littleli , & force pushed a single latest commit to this PR.

I ended up deleting handler in processEvent WM_STYLECHANGED and instead doing resize inside of setTitlebarVisible --
the problem is that you must know whether the titlebar was added or removed.

• If titlebar was removed, you must resize window to current content size
• If titlebar was added, you must resize content to old window size

Test Instructions

1. Execute original test instructions but with repeated Toggle Titlebar via Ctrl+T
2. Check that the content size should remained fixed (1264, 681) & rendering doesn't degrade into "blurry mode"

Hopefully this is the mergeable commit! 🤞

[tonsky]

This does work better! I do think that correcting the size right in the setTitlebarVisible is ultimately the right move.

Can I ask you to change one more thing, though?

The way I feel this should work is that

setWindowSize(w, h)
setTitlebarVisible(false)

and

setTitlebarVisible(false)
setWindowSize(w, h)

should produce the same result. In other words, you tell your window to occupy certain amount of space on a screen. For example, 1/2 on the left. Then, no matter whether you turn titlebar on or off, the amount of space it occupies should stay the same. The external border of the window should not move. Content area will expand/compact a little when removing titlebar, but that’s about it.

I feel this is the most natural behavior. What do you think?

If you agree, then the order you have your conditions is reversed: e.g. if I remove titlebar, I should take window size, hide titlebar, and then set content size to previous coordinates/dimensions.

I tried it myself and it didn’t produce desirable results. Looks like this is because we actually measure window boundary wrong! See #289

So this is what I suggest. Can you fix #289 first and then we come back here and swap the windowSize/contentSize in the setTitlebarVisible and it will all work flawlessly?

[Quezion]

RE: I'm trying to fix #289 but ran into an issue where Windows doesn't report the dropshadow width/height until the first render of the window.

This means that the shadow width/height can't be immediately accounted for when restoring the titlebar. To fix this, maybe we can react to a WM_ event somewhere in processEvent once the shadow is available (WM_STYLECHANGED, perhaps?)

Alternatively, we can "remember" the dropshadow size from the first time we hid the window and use those values when restoring the titlebar+border styles.

I'll work on this some more in a few days.

[tonsky]

I think remembering is fine? They don’t change anyway, right?

[Quezion]

It shouldn't change unless the user changes the Windows theming (removing drop shadow), but that seems like a fine compromise for not having the intermediate possibly-bugged state while we wait for a WM_ event callback to fire.

[Quezion]

I was able to workaround the bugged local behavior but the code is nasty.

Turns out that setContentSize uses winapi AdjustWindowRectEx -- this also reports incorrect sizes when the titlebar has just been added/removed and the window hasn't received a new render from Windows. This is untrue, I was unaware that the library was returning static hardcoding values for getWindowStyle. AdjustWindowRectEx does a pure calculation given any window style & isn't dependent on the hWnd's state.

To work around this, we also need to manually track the window's titlebar + borderwidth between hide/set titlebar. These values then get used to account for unknowns in setWindowSize, setWindowPosition, and setContentSize.

The "cleaner" solution would involve some state machine that waits for Window's first render after a window's style is changed, thus allowing AdjustWindowRectEx to actually work, but this introduces other types of complexities.

[Quezion]

@tonsky Are you trying to support Windows XP?
To pull in DwmGetWindowAttribute, I needed to add both lines:

#include <dwmapi.h>
#pragma comment(lib,"dwmapi.lib")

This .h itself is only available on Vista or later. To support XP, this needs to somehow be dynamically loaded without breaking the linker on Vista+. Note that the #pragma comment is required for it to work on my Win10 machine.

If we drop Windows XP support then I can submit this PR immediately without needing to add Windows version detection.

---

I've cleaned up the code (as much as is possible) but I need to solve "Windows version detection" before I can submit it. You're supposed to use the below snippet to get the bounds of the application without the dropshadow, but it's only available on Vista or later (majorVersion > 6)

DwmGetWindowAttribute(_hWnd, DWMWA_EXTENDED_FRAME_BOUNDS, &rect, sizeof(rect));

It turns out that detecting the OS version is nontrivial, the methods have been deprecated multiple times, and Windows would prefer you test for the presence of features. Except there's no universally available feature that tells you if DwmGetWindowAttributes exists or if you're on Vista+.

There is https://learn.microsoft.com/en-us/windows/win32/api/versionhelpers/nf-versionhelpers-iswindowsvistaorgreater , but this requires a correct Windows manifest to be set & I suspect we can't rely on that. This could use more investigation.

https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1

So I'm working on a shim to determine OS version for the sake of getWindowRect() & will submit the PR once this is complete.

[tonsky]

That’s fine I think, we probably won’t run on Vista any time soon. Maybe we should care about Win 10 but not lowerOn 26 Feb 2024, at 20:42, Quest Yarbrough ***@***.***> wrote: I've cleaned up the code (as much as is possible) but I need to solve "Windows version detection" before I can submit it. You're supposed to use the below snippet to get the bounds of the application without the dropshadow, but it's only available on Vista or later (majorVersion > 6) DwmGetWindowAttribute(_hWnd, DWMWA_EXTENDED_FRAME_BOUNDS, &rect, sizeof(rect)); It turns out that detecting the OS version is nontrivial, the methods have been deprecated multiple times, and Windows would prefer you test for the presence of features. Except there's no supported feature that tells you if DwmGetWindowAttributes exists or if you're on Vista+. https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1 So I'm working on a shim to determine OS version for the sake of getWindowRect() & will submit the PR once this is complete. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[Quezion]

Added latest commit. I misspoke earlier -- this should work on Vista but won't on Windows XP.

The code seems nasty to me. I suspect @tonsky will have some comments to address before the final commit. For the sake of diff, I've just added a 2nd commit instead of squashing both commits down.

[tonsky]

Works better, but still not quite there yet. Hiding titlebar keeps window in place (great!) but showing title bar moves window 5 px up (on 200% scaling). If I change scaling to 100%, it only moves 3px up. Is there a DPI-aware calculation lost here somewhere? Perhaps this bit is important?

[tonsky]

Re: code itself. I see you change _windowShadowWidth/Height back and forth. Wouldn’t it be better if you just identify them after window is first shown and remember these values?

As I understand, you need to account for shadow if titlebar is visible and not account for it if it’s not. Might this be just a condition in setWindowSize? Something along the lines of

if (titlebarVisible) {
  setSize(width + shadowWidth, height + shadowHeight);
} else {
  setSize(width, height);
}

I find it easier to work with when state is immutable

[Quezion]

I see what you mean with the changes. I'll try to remove the windowShadow vars on next commit & fix the DPI calculation. I remember something in the MS docs about DPI sensitivity & AdjustWindowRectEx, which is used in WindowWin32::SetContentSize() to calculate some of the window bounds.

I'm sure there's a way to work around it but it'll take some testing to figure it out. I'll do this as soon as I can but I'm unexpectedly buying & moving to a new place. It may be a few months before I get my programming time back but I'm looking forward to landing this PR and thanks for the feedback along the way. 🙏