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

feat: enable BrowserWindow level placement on Windows #18982

Merged
merged 1 commit into from Jul 10, 2019

Conversation

@CapOM
Copy link
Contributor

CapOM commented Jun 25, 2019

Description of Change

For now it only adds the ability to place the window below
the task bar while still being always on top.
Previous behaviour was always showing the window above the task
bar when top is true. We keep this default behaviour, i.e. when
the 'level' parameter is omitted.

#18933

Checklist

Release Notes

Notes: Added the ability to set a window always on top but behind the taskbar on Windows.

@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jun 25, 2019

@codebytere codebytere changed the title feat: Can a window always on top but behind the taskbar on Win32 feat: enable BrowserWindow level placement on Windows Jun 26, 2019
docs/api/browser-window.md Outdated Show resolved Hide resolved
shell/browser/native_window_views.cc Outdated Show resolved Hide resolved
@CapOM CapOM force-pushed the CapOM:set_always_on_top_win32 branch from 5e4ce25 to 132e90e Jun 26, 2019
@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jun 26, 2019

The test failures seem unrelated to this change. I addressed the remarks, please take a look, thx!

@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 26, 2019
Copy link
Member

codebytere left a comment

From the implementation side this looks good to me, but i'd like a secondary review from @brenca or someone else with a Windows machine before merging!

Copy link
Member

brenca left a comment

From my testing on Windows, the behind taskbar property of the window is lost when the user activates another window and then reactivates the one that's always-on-top. I suggest looking at AtomDesktopNativeWidgetAura to detect when the window is activated, and then you could set the window to be behind the taskbar again. We should probably test other scenarios too, like minimizing and restoring and such (a lot of things happen in chromium internally that's hard to track down).

@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jul 2, 2019

Hi, thx for the testing and suggestions. One could argue that the user of setAlwaysOnTop can still watch for the focus event and call this api again. But I am ok to internally ensure the behind taskbar property upon NativeWindowViews::OnWidgetActivationChanged(when active is true), i.e. focus. I will also do it for restore, resize and show. That should cover most of the cases. Remaining corner cases will have to be handle for the user of this api. How does this plan sound ?

@CapOM CapOM force-pushed the CapOM:set_always_on_top_win32 branch 2 times, most recently from 415797b to c1a27ef Jul 3, 2019
@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jul 3, 2019

Hi @brenca , I addressed your remarks. Actually OnWidgetActivationChanged is internally called upon activate, restore and show so no need to ensure the placement else where.
Please take a look thx!

@CapOM CapOM force-pushed the CapOM:set_always_on_top_win32 branch from c1a27ef to 2f75cc0 Jul 5, 2019
For now it only adds the ability to place the window below
the task bar while still being always on top.
Previous behaviour was always showing the window above the task
bar when top is true. We keep this default behaviour, i.e. when
the 'level' parameter is omitted.

#18933

Notes: Can set a window always on top but behind the taskbar on Windows
@CapOM CapOM force-pushed the CapOM:set_always_on_top_win32 branch from 2f75cc0 to 78bec37 Jul 5, 2019
@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jul 5, 2019

Hi @miniak , thx for the review, I addressed the remarks, please take a look, thx!

@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jul 9, 2019

Hi @miniak , hi @brenca , gentle ping ?

@brenca
brenca approved these changes Jul 9, 2019
Copy link
Member

brenca left a comment

Nice, good work! I do sometimes see a flicker when the window is reactivated (flickers from above to behind the taskbar), but we can address that in a follow-up PR if needed.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Jul 10, 2019

autoUpdater behavior "before each" hook for "should have a valid code signing identity" - "before each" hook for "should have a valid code signing identity"

is an expected failure for fork PRs and does not block merge.

@codebytere codebytere merged commit 8b31953 into electron:master Jul 10, 2019
8 of 9 checks passed
8 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 10, 2019

Release Notes Persisted

Added the ability to set a window always on top but behind the taskbar on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.