Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update traffic lights on macos when title bar zooms #155906

Merged
merged 5 commits into from
Aug 4, 2022
Merged

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Jul 21, 2022

fixes #155557

This adopts the same technique as Windows because we need to know about the titlebar's counter zoom behavior. however, it removes the workaround for electron/electron#34822 which should be fixed in 18.3.6 when released and adopted

@sbatten sbatten enabled auto-merge (squash) July 21, 2022 22:34
@sbatten sbatten self-assigned this Jul 21, 2022
@sbatten sbatten requested a review from bpasero July 21, 2022 22:34
@VSCodeTriageBot VSCodeTriageBot added this to the July 2022 milestone Jul 21, 2022
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is generally an elegant solution but:

  • we cannot merge this before we updated Electron and I am not sure that happens soon, I am not even seeing that new Electron version released yet, maybe lobby in with Deepak
  • I see a flicker on startup, and I think this is because the traffic lights now are only updated from the workbench which can load slow:
    Recording 2022-07-22 at 06 24 10

PS: I find updateTitleBarOverlay a bit misleading, this is not an overlay, this is updating the native window controls (maybe then call it "window control overlay").

- update api name
- cached window controls information
@sbatten
Copy link
Member Author

sbatten commented Jul 22, 2022

@bpasero pushed some changes based on the feedback. most of the startup flicker is caused by the electron bug, but I've also added caching to improve the new window experience. With the fix from electron and the caching, there should be minimal to no jumping.

@sbatten sbatten requested a review from bpasero July 22, 2022 18:30
@sbatten sbatten modified the milestones: July 2022, August 2022 Jul 28, 2022
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rather use the IStateMainService if you must cache something in electron-main, given:

/**
* Important: unlike other storage services in the renderer, the
* main process does not await the storage to be ready, rather
* storage is being initialized while a window opens to reduce
* pressure on startup.
*
* As such, any client wanting to access application storage from the
* main process needs to wait for `whenReady`, otherwise there is
* a chance that the service operates on an in-memory store that
* is not backed by any persistent DB.
*/
readonly whenReady: Promise<void>;

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a change to address this and gave it some testing on macOS, not Windows though. I am not sure if there is any downside in calling updateWindowControls on every startup even when the title area is not specially configured in any way.

@sbatten sbatten merged commit 35819b2 into main Aug 4, 2022
@sbatten sbatten deleted the sbatten/brave-lamprey branch August 4, 2022 10:37
@sbatten
Copy link
Member Author

sbatten commented Aug 4, 2022

@bpasero thank you for that, i just test with the electron 19 build and it seems to work well

joyceerhl pushed a commit that referenced this pull request Aug 10, 2022
* update traffic lights on macos when title bar zooms
fixes #155557

* address PR feedback
- update api name
- cached window controls information

* address feedback

Co-authored-by: Benjamin Pasero <benjamin.pasero@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoomed title needs to update traffic lights position
3 participants