Conversation
|
If you're ok with it, I might just add a |
amilich
left a comment
amilich
left a comment
There was a problem hiding this comment.
Will be sending this:
try {
const message = {
type: 'theme',
data: { theme }
};
// Windows WebView2 injects a global `window.chrome` object
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
chrome.webview.postMessage(JSON.stringify(message));
} catch (error) {
console.error('Failed to send theme to Windows app', error);
}
This comment was marked as outdated.
This comment was marked as outdated.
| [DllImport("dwmapi.dll", PreserveSig = true)] | ||
| static extern int DwmSetWindowAttribute(IntPtr hwnd, int attr, ref int attrValue, int attrSize); | ||
| const int DWMWA_CAPTION_COLOR = 35; |
There was a problem hiding this comment.
Opinion: Ideally, rather than write PInvokes by hand, we would wire up CsWin32 here.
|
Ready for review again. I can modify it to use CsWin32 for |
There was a problem hiding this comment.
Nice!
To minimize dependencies, at some point we can probably swap out the use of WMI here for some standard Win32 APIs. (Microsoft is very slowly removing WMI from Windows.) Not something we need to worry about for this PR in my opinion.
I made a note to investigate that uxtheme API failure; will start making some noise internally if needed. The registry approach here is solid though. I also made a note to talk to some folks on WebView2--having an API to only change the theme across all WebView2 instances is nuts to me.
Disclaimer: I don't work for Skiff and am just a stranger on the Internet. Take whatever I say here with a grain of salt.
Noting this is out in production. |
|
Now the messaging is out in production, is there anything left blocking this PR from being merged or are we just waiting for review? I'm happy to make changes if any are needed. |
Awesome. Should I go ahead and test anything in particular? I can build/release in the next day |
Not really, just that the titlebar matches the current theme and that "auto" respects the user's theme preference. |
3 participants
Pretty essential feature. Should fail gracefully on Windows versions that are too old for dark theme.
Currently this hooks into localStorage, but if you want to send the message manually from the web app code you can revert 550d987.