windows: Fix regression introduced by a previous PR

[JunkuiZhang]

In the current main branch, Zed refuses to open any windows on Windows. After performing a quick git bisect, I found that PR #18161 introduced this regression.

I'll provide more detailed information about this PR after I return from dinner.

Release Notes:

• N/A

[huacnlee]

zed/crates/gpui/src/platform/windows/window.rs

Lines 542 to 550 in 68d6177

	fn activate(&self) {
	let hwnd = self.0.hwnd;
	unsafe { SetActiveWindow(hwnd).log_err() };
	unsafe { SetFocus(hwnd).log_err() };
	// todo(windows)
	// crate `windows 0.56` reports true as Err
	unsafe { SetForegroundWindow(hwnd).as_bool() };
	}

Maybe the problem is this:

The SetActiveWindow function activates a window, but not if the application is in the background. The window will be brought into the foreground (top of Z-Order) if its application is in the foreground when the system activates the window.

https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setactivewindow#remarks

It seems that we need to add a line to this function:

unsafe { ShowWindow(hwnd, SW_SHOW).ok().log_err() };

But the Window still not show in the front.

[JunkuiZhang]

@huacnlee The issue is much more complicated than that, please see my new comment.

[JunkuiZhang]

This bug itself is relatively straightforward to fix, but the associated quality-of-life issues it brings are a bit trickier to address. Here are the approaches I’ve tried for resolving it:

Approach 1: Add ShowWindow in activate_window
This is the simplest solution, but the downside is that it changes the window opening animation in Zed from the default Windows "open window" animation to the "restore from minimized" animation.

Approach 2: Use winit’s method of setting additional exstyle or style
This method fixes the issue but causes the window opening animation to disappear entirely in Zed, although other animations remain functional.

Approach 3: Use AnimateWindow to manually set the opening animation
However, the animation displayed by this function is, in my opinion, one of the least visually appealing options.

I’ve also experimented with other approaches, but they all seem to run into animation-related issues. From the tests I performed, it seems that the window-opening animation can only be triggered by the initial ShowWindow call after CreateWindow.

Given that Zed currently creates windows in a hidden state and then makes them visible by calling activate_window, the changes proposed in this PR would decouple the "show window" logic from the window creation process and move it into a separate function. The impact of these changes would be as follows (please see the table below):

Previous Behavior	New Behavior	Result
WindowParams.show is true	WindowParams.visibility set to Show	Same behavior as before
WindowParams.show is false	WindowParams.visibility set to DeferredShow	Zed currently behaves this way: the window is initially hidden, but requires an explicit show_window call to become visible. Otherwise, it won’t display.
WindowParams.show is false	WindowParams.visibility set to Invisible	Same behavior as before

Alternatively, a simpler solution would be to set WindowParams.show = true when Zed opens a window. I would appreciate Zed team's thoughts on which approach you believe is best.

cc @mikayla-maki

[mikayla-maki]

I think moving the boolean into the window params is our preferred solution. If windows needs it to feel right, then that’s what we’ll do :)

[huacnlee]

I have tried different ways, but still can't find a suitable solution. It seems that the trouble is that when CreateWindow is called, it cannot be hidden from the beginning.

I also tried to remove WS_VISIBLE from the CreateWindow parameter, but it didn't work.

[JunkuiZhang]

My Mac is currently at school, and I’m in my dorm, so I’m unable to test these changes on macOS. I’m not sure if the current modifications will have any impact on macOS. Should I use cfg to mark the changes? Say:

#[cfg(windows)]
show: true,
#[cfg(not(windows))]
show: false,

Let me know your thoughts @mikayla-maki

[JunkuiZhang]

I have tried different ways, but still can't find a suitable solution. It seems that the trouble is that when CreateWindow is called, it cannot be hidden from the beginning.

I also tried to remove WS_VISIBLE from the CreateWindow parameter, but it didn't work.

Yes, if we do want to keep the current deferred show behavior of Zed, there is no easy solution.

[mikayla-maki]

I did a bit of a review of the code for what this code would change, and it seems like it would cause our window opening to have a lot more pop-in. See https://github.com/zed-industries/zed/blob/main/crates/workspace/src/workspace.rs#L1192-L1223, for how we create the window, then deserialize the state, then activate the window.

Given this issue, I think I'd actually prefer switching to option #2 of the ones you've given @JunkuiZhang.

That said, could we fake this in GPUI and get the best of all worlds? Essentially, don't create the underlying windows-window until the first 'activate' call, so that we get the proper animation and also don't have issues with workspace code that relies on a WindowContext. That said... I could see this solution getting really messy and unpleasant as there's now a bunch of platform APIs that do nothing or produce fake results until the window is shown.

Let me know what y'all think. Going with the winit style solution is probably the right choice :D

[JunkuiZhang]

I did a bit of a review of the code for what this code would change, and it seems like it would cause our window opening to have a lot more pop-in. See https://github.com/zed-industries/zed/blob/main/crates/workspace/src/workspace.rs#L1192-L1223, for how we create the window, then deserialize the state, then activate the window.

This is exactly my concern here, that is the possibility of unintended side effects here if we directly change show from false to true.

Given this issue, I think I'd actually prefer switching to option #2 of the ones you've given @JunkuiZhang.

In fact, this approach doesn't work as expected. As I mentioned before, using this method causes the window's opening animation to disappear, meaning the window just appears abruptly, which I find quite undesirable.

Allow me to explain method #2 in more detail:

Under PR #18161, the process for creating an invisible window in Zed is as follows:

• The style parameter is set to WS_MINIMIZED during creation.
• After creating the window, ShowWindow(SW_HIDE) is called immediately.

While fixing this issue, I noticed that winit creates an invisible window with the following process:

• The exstyle parameter is set to WS_EX_NOACTIVATE | WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_TOOLWINDOW during creation. See the details here.
• And that's it, there’s no call to ShowWindow().

In Zed’s case, the window creation flow is:

• Create an invisible window (by setting show = false, see here.)
• Perform some actions
• Make the window visible by calling activate_window

Under method #2, to fix this issue, when calling activate_window, we would first need to:

• Remove the WS_EX_NOACTIVATE | WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_TOOLWINDOW flags from the exstyle parameter and set it back to WS_EX_APPWINDOW
• Call ShowWindow(...)

However, this causes the issue I mentioned earlier—the window’s opening animation disappears.

That said, could we fake this in GPUI and get the best of all worlds? Essentially, don't create the underlying windows-window until the first 'activate' call, so that we get the proper animation and also don't have issues with workspace code that relies on a WindowContext. That said... I could see this solution getting really messy and unpleasant as there's now a bunch of platform APIs that do nothing or produce fake results until the window is shown.

If we proceed with this approach, it might turn out to be a dead end:

1. If we are creating a window that should be shown immediately (show = true), we need to ensure the window is created during open_window.

2. If we want to create a truly invisible window (in Zed's case, it first creates an invisible window and then shows it later, which I’ll refer to as DeferredShow), we must actually create the window during open_window because in this scenario, activate_window might never be called, so the creation needs to happen upfront.

3. For deferred-show window creation (as Zed currently does), where the actual window creation happens only at activate_window.

As you can see, this solution introduces some conflicts. For both Case 2 and Case 3, the window creation parameter is show = false, yet the process of creating the window occurs in two different places: the former occurs during the open_window function call, while the latter happens during activate_window.

Thus, we need to differentiate between these two cases. However, if we make this distinction, there may actually be a simpler solution, as explained below.

Let me know what y'all think. Going with the winit style solution is probably the right choice :D

After extensive testing, I think the following solutions are feasible:

• Modify how Zed opens windows. Specifically, change the show = false at this line to show = true. Of course, this approach might introduce unforeseen bugs as discussed early.
• Introduce WindowVisibility. For Zed's current window creation flow, we don’t actually intend to create an invisible window but rather a window with delayed visibility. So, this approach would seperate the show = false situation into two cases:
  1. Creating a window that should remain invisible indefinitely.
  2. Creating a window that remains hidden initially, but becomes visible only after the first activate_window call.

At present, the latest commit (a8f5e87) in this PR implements the first solution. The next step will be to implement the second approach.

[JunkuiZhang]

In the latest changes, I introduced a WindowDisplayState struct. Since my English is not my native language, please feel free to suggest any improvements or better names for the struct and its member variables if you have any ideas.

[mikayla-maki]

Rather than add this additional optional, could we just remove this minimized state entirely? I'd prefer to keep the two options, Hidden and Visible or true and false, as this third option doesn't feel quite right yet.

[JunkuiZhang]

Rather than add this additional optional, could we just remove this minimized state entirely?

The minimized state here is quite necessary; otherwise, when creating an invisible window, there will be a brief window flash, as shown in the video below.

Screen.Recording.2024-10-03.102803.mp4

I'd prefer to keep the two options, Hidden and Visible or true and false, as this third option doesn't feel quite right yet.

The purpose of the DeferredVisible option is to distinguish between the following two cases when show = false is set:

• Creating a hidden window, as demonstrated by clicking the "invisible" button in the video.
• Creating a window that remains hidden until activate_window is called, which is how Zed currently handles window creation.

The reason for this distinction is that if we don't make it, when Zed creates a window:

• Create a hidden window
• Zed configures the window
• Show the hidden window

Regardless of how the hidden window is created, in the third step, the window's opening animation will be problematic—it will either suddenly appear on the screen (with no opening animation) or play the animation for restoring from a minimized state.

Therefore, under the DeferredVisible option, we avoid creating a hidden window, which preserves the window's opening animation. The process becomes:

• Create a normal window
• Zed configures the window
• Show the window

[JunkuiZhang]

If you do want to keep just two options, true or false, we may need to create a new window within the first call of activate_window.

[JunkuiZhang]

I've created another PR #18705 which is a better solution I think

[SomeoneToIgnore]

#18705 closes this one, presumably, as the most tested and working one so far.