[WS-31] windows integrated titlebar #102

szymonkaliski · 2023-08-15T13:42:24Z

Why

Integrated titlebar on Windows like on macOS:

Needs https://github.com/replit/repl-it-web/pull/35147 to be merged.

What changed

added custom platform styling for Windows
had to grab foreground color too
var(--background-root) in platform styling is not working on Windows, so changed to use getLastSeenBackgroundColor (and new getLastSeenForegroundColor)

Test plan

Check out the screenshots.

linear · 2023-08-15T13:42:27Z

WS-31 integrated titlebar/menubar UI for Windows, like on macOS

see what we can do, Windows looks ugly with the default title bar design (see screenshot)

notes from Faris: https://replit.slack.com/archives/C0509G0FJNL/p1683158097198949

Screenshot 2023-05-31 at 4.53.58 PM.png

sergeichestakov · 2023-08-15T16:03:58Z

will check this in a bit with the web PR checked out - in the meantime looks like you have some conflicts here from the other PR

szymonkaliski · 2023-08-15T16:12:15Z

thanks; conflicts fixed :)

sergeichestakov · 2023-08-15T17:14:30Z

src/createWindow.ts

+      titleBarOverlay: {
+        color: backgroundColor,
+        symbolColor: foregroundColor,
+        height: 44,


can we make this the same height as the above for consistency? i.e. 48px esp since that's more consistent with RUI values and matches some of our other header heights

if we go with 48 then the hover state of the windows native button touches the top of the pane below it, which looks unintentional/wrong, considering we have 4px padding in most places

really? I feel like this looks better. what makes it look unintentional to you? feel like the later does for me:

44px

48px

yea we have paddings everywhere so feels weird to me that it suddenly touches the pane... maybe worth circulating with #design for another opinion 🤷

yeah agree worth asking. feel like at the OS level it's ok to be flush with the pane

asked here looks like 48px is the preferred

sure, will update together with the other changes!

sergeichestakov

this largely looks good (except for the comment above) but can you make the background translucent? it looks like it's currently hardcoded to match the dark mode color so it looks off if you're using a light theme or anything other than our official dark theme:

szymonkaliski · 2023-08-16T07:57:54Z

this largely looks good (except for the comment above) but can you make the background translucent? it looks like it's currently hardcoded to match the dark mode color so it looks off if you're using a light theme or anything other than our official dark theme

It's not hardcoded, it just defaults to the background/foreground from the dark theme, but then uses the colors recorded when we close the window:

desktop/src/createWindow.ts

Lines 171 to 184 in e6a0243

    
           window.on("close", async () => { 
        
             // We're capturing the background and foreground colors to use as main 
        
             // browser window background color, and style the traffic lights/window 
        
             // buttons. 
        
             const backgroundColor = await window.webContents.executeJavaScript( 
        
               `getComputedStyle(document.body).getPropertyValue('--background-root');`, 
        
             ); 
        
             const foregroundColor = await window.webContents.executeJavaScript( 
        
               `getComputedStyle(document.body).getPropertyValue('--foreground-default');`, 
        
             ); 
        
             store.setLastSeenBackgroundColor(backgroundColor); 
        
             store.setLastSeenForegroundColor(foregroundColor); 
        
             store.setWindowBounds(window.getBounds()); 
        
           });

This obviously will break if we have a mismatch between stored/current colors, like if we change the theme without restarting the app.

One possible way forward would be to expose some version of https://www.electronjs.org/docs/latest/api/browser-window#winsettitlebaroverlayoptions-windows through the electron API, and allow the client to change the color when we change the theme. What do you think @sergeichestakov?

sergeichestakov · 2023-08-16T14:47:59Z

hmm it doesn't seem to restore for me in light mode (in other words, I still see the dark mode colors regardless). and yeah we're going to want to go with an approach that is resilient to switching themes

szymonkaliski · 2023-08-16T15:03:49Z

ok cool, I'll switch it to draft and will update soon

sergeichestakov · 2023-08-16T15:05:01Z

nvm looks like it does when I close and re-open a window (think I was just opening a window before without closing after switching themes)

szymonkaliski · 2023-08-16T15:08:49Z

nvm looks like it does when I close and re-open a window (think I was just opening a window before without closing after switching themes)

Yea that's what I was trying to say -- we only update when we close the window, so it's easy to get into a stale state, I think we should expose this through the API so we can switch colors dynamically.

sergeichestakov · 2023-08-16T15:54:15Z

src/createWindow.ts

+        color: backgroundColor,
+        symbolColor: foregroundColor,


can we not affect Mac styling in the context of this PR? I think using CSS vars is the ideal solution if it works on MacOS. Looks like we'll have to find a different approach on Windows but don't see a reason to switch on Mac

these actually don't do anything on macOS, I'll drop them completely

szymonkaliski · 2023-08-21T09:16:14Z

@sergeichestakov I updated the code with comments, and added an event to notify on theme change, so we get this:

Screenshot.2023-08-21.at.11.01.17.mov

I tested on macOS and Windows, but would be good for you to test as well.

Since this depends on a new desktop app API event, you should test together with recent changes in https://github.com/replit/repl-it-web/pull/35147

szymonkaliski · 2023-08-21T09:20:03Z

src/createWindow.ts

-        titleBarStyle: "hidden",
-        titleBarOverlay: {
-          color: "var(--background-root)",
-          symbolColor: "var(--foreground-default)",


color/symbolColor does nothing on macOS -- it's a Windows thing only

sergeichestakov

exciting! let's just pass the theme values in directly as part of the IPC call? otherwise I see a flash in the transparent header when switching themes because the old values persist for a split second

src/createWindow.ts

src/preload.ts

sergeichestakov

looks good. let's just make it an object arg rather than positional. also some merge conflicts to fix. I can take over

sergeichestakov · 2023-08-23T13:47:25Z

src/preload.ts

@@ -48,6 +48,8 @@ contextBridge.exposeInMainWorld('replitDesktop', {
    ipcRenderer.invoke(events.SHOW_MESSAGE_BOX, params),
  onEnterFullscreen: makeEventHandler(events.ON_ENTER_FULLSCREEN),
  onLeaveFullscreen: makeEventHandler(events.ON_LEAVE_FULLSCREEN),
+  themeChanged: (backgroundColor: string, foregroundColor: string) =>


I think we're going to want to an object param here rather than multiple positional params since we may add more values down the line (for instance, if we want to send the full theme over).

also: we should just be explicit here what theme values these correspond to. e.g. backgroundRoot and foregroundDefault rather than just backgroundColor, foregroundColor since, again, we may want to send more at some point and this will be easier to pass down directly from the theme values on the client

src/createWindow.ts

sergeichestakov · 2023-08-23T13:51:44Z

src/createWindow.ts

@@ -76,13 +76,17 @@ function isInBounds(rect: Rectangle) {
  });
 }

-async function getLastSeenBackgroundColor(
+export async function getColorsFromWindow(


don't think we need to export as this isn't used anywhere else

sergeichestakov · 2023-08-23T13:54:44Z

src/preload.ts

@@ -48,6 +48,8 @@ contextBridge.exposeInMainWorld('replitDesktop', {
    ipcRenderer.invoke(events.SHOW_MESSAGE_BOX, params),
  onEnterFullscreen: makeEventHandler(events.ON_ENTER_FULLSCREEN),
  onLeaveFullscreen: makeEventHandler(events.ON_LEAVE_FULLSCREEN),
+  themeChanged: (backgroundColor: string, foregroundColor: string) =>


also: we should just be explicit here what theme values these correspond to. e.g. backgroundRoot and foregroundDefault rather than just backgroundColor, foregroundColor since, again, we may want to send more at some point and this will be easier to pass down directly from the theme values on the client

src/createWindow.ts

sergeichestakov

updated the API a bit and did some minor refactoring. thanks for pushing this forward!

windows integrated titlebar

e16da62

szymonkaliski requested a review from sergeichestakov August 15, 2023 13:42

Merge branch 'main' into sk/windows-integrated-titlebar

e6a0243

sergeichestakov reviewed Aug 15, 2023

View reviewed changes

sergeichestakov suggested changes Aug 15, 2023

View reviewed changes

szymonkaliski marked this pull request as draft August 16, 2023 15:03

sergeichestakov reviewed Aug 16, 2023

View reviewed changes

szymonkaliski added 2 commits August 21, 2023 10:01

Merge branch 'main' into sk/windows-integrated-titlebar

b098d22

keep theme in sync with the web app

854a700

szymonkaliski marked this pull request as ready for review August 21, 2023 09:16

szymonkaliski commented Aug 21, 2023

View reviewed changes

szymonkaliski requested a review from sergeichestakov August 21, 2023 10:53

sergeichestakov reviewed Aug 21, 2023

View reviewed changes

src/createWindow.ts Show resolved Hide resolved

src/preload.ts Outdated Show resolved Hide resolved

szymonkaliski added 3 commits August 23, 2023 13:49

receive background and foreground color with theme changed event

b9144e3

Merge branch 'main' into sk/windows-integrated-titlebar

1866cfe

setTitleBarOverlay is windows-only thing

02c1c97

sergeichestakov reviewed Aug 23, 2023

View reviewed changes

szymonkaliski and others added 3 commits August 23, 2023 15:55

Merge branch 'main' into sk/windows-integrated-titlebar

6ff0f76

drop duplicated log import

f533084

Object param instead of positional args, update API naming

8e76737

Move platform specific styling into helper

a39e2a6

sergeichestakov approved these changes Aug 23, 2023

View reviewed changes

Reorder, add comments

8458ec5

sergeichestakov merged commit fd35af9 into main Aug 23, 2023
3 checks passed

sergeichestakov deleted the sk/windows-integrated-titlebar branch August 23, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WS-31] windows integrated titlebar #102

[WS-31] windows integrated titlebar #102

szymonkaliski commented Aug 15, 2023 •

edited

Loading

linear bot commented Aug 15, 2023

sergeichestakov commented Aug 15, 2023

szymonkaliski commented Aug 15, 2023

sergeichestakov Aug 15, 2023

szymonkaliski Aug 16, 2023 •

edited

Loading

sergeichestakov Aug 16, 2023

szymonkaliski Aug 16, 2023

sergeichestakov Aug 16, 2023

sergeichestakov Aug 16, 2023

szymonkaliski Aug 17, 2023

sergeichestakov left a comment

szymonkaliski commented Aug 16, 2023

sergeichestakov commented Aug 16, 2023

szymonkaliski commented Aug 16, 2023

sergeichestakov commented Aug 16, 2023

szymonkaliski commented Aug 16, 2023

sergeichestakov Aug 16, 2023

szymonkaliski Aug 17, 2023

szymonkaliski commented Aug 21, 2023

szymonkaliski Aug 21, 2023

sergeichestakov left a comment

sergeichestakov left a comment

sergeichestakov Aug 23, 2023

sergeichestakov Aug 23, 2023

sergeichestakov Aug 23, 2023

sergeichestakov Aug 23, 2023

sergeichestakov left a comment

[WS-31] windows integrated titlebar #102

[WS-31] windows integrated titlebar #102

Conversation

szymonkaliski commented Aug 15, 2023 • edited Loading

Why

What changed

Test plan

linear bot commented Aug 15, 2023

sergeichestakov commented Aug 15, 2023

szymonkaliski commented Aug 15, 2023

Choose a reason for hiding this comment

szymonkaliski Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

44px

48px

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeichestakov left a comment

Choose a reason for hiding this comment

szymonkaliski commented Aug 16, 2023

sergeichestakov commented Aug 16, 2023

szymonkaliski commented Aug 16, 2023

sergeichestakov commented Aug 16, 2023

szymonkaliski commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymonkaliski commented Aug 21, 2023

Choose a reason for hiding this comment

sergeichestakov left a comment

Choose a reason for hiding this comment

sergeichestakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeichestakov left a comment

Choose a reason for hiding this comment

szymonkaliski commented Aug 15, 2023 •

edited

Loading

szymonkaliski Aug 16, 2023 •

edited

Loading