-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
Adds vibrancy effect for macos #7898
Conversation
Thanks for this feature! I hope it's merged soon. |
|
||
#### `win.setVibrancy(type)` _macOS_ | ||
|
||
* `type` String - Values include `appearance-based`, `light`, `dark`, `titlebar`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the style Can be thing1, thing2 or thing3
@@ -265,6 +265,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`. | |||
* `offscreen` Boolean - Whether to enable offscreen rendering for the browser | |||
window. Defaults to `false`. | |||
* `sandbox` Boolean - Whether to enable Chromium OS-level sandbox. | |||
* `vibrancy` String - Add a type of vibrancy effect to the window, only on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See supported types at the
setVibrancy
method.
It might seem silly but can we duplicate the list of value up here as well purely so the JSON api is accurate
@MarshallOfSound thanks! good thing you pointed it out, because I also realized I put the option under |
I wonder if it works with frameless window |
@YurySolovyov it does! |
@@ -901,6 +909,8 @@ void Window::BuildPrototype(v8::Isolate* isolate, | |||
&Window::SetVisibleOnAllWorkspaces) | |||
.SetMethod("isVisibleOnAllWorkspaces", | |||
&Window::IsVisibleOnAllWorkspaces) | |||
.SetMethod("setVibrancy", &Window::SetVibrancy) | |||
.SetMethod("removeVibrancy", &Window::RemoveVibrancy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like other methods in BrowserWindow
like setMenu
, setParentWindow
, setThumbnailClip
support clearing things via the setter, instead of a second remove*
method.
For consistency it might be good to follow that pattern here, maybe setVibrancy
with null
removes it?
void NativeWindowMac::SetVibrancy(const std::string& type) { | ||
if (!(base::mac::IsOSMavericks() || base::mac::IsOSYosemiteOrLater())) return; | ||
|
||
NSVisualEffectView *vview = (NSVisualEffectView *)vibrant_view_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be NSVisualEffectView* vview
and (NSVisualEffectView*)
, I don't think the linter runs on Objective-C files.
@@ -1188,3 +1191,17 @@ Returns `BrowserWindow[]` - All child windows. | |||
|
|||
[window-levels]: https://developer.apple.com/reference/appkit/nswindow/1664726-window_levels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we leave these footer links at the very bottom of the docs so the new docs can go above here.
|
||
NSVisualEffectView *vview = (NSVisualEffectView *)vibrant_view_; | ||
if (vview == nil) { | ||
vview = [[NSVisualEffectView alloc] initWithFrame: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vview
name is a little vague, maybe effect_view
instead?
@@ -161,6 +161,10 @@ class NativeWindow : public base::SupportsUserData, | |||
virtual void SetVisibleOnAllWorkspaces(bool visible) = 0; | |||
virtual bool IsVisibleOnAllWorkspaces() = 0; | |||
|
|||
// Vibrancy API | |||
virtual void SetVibrancy(const std::string& type) = 0; | |||
virtual void RemoveVibrancy() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be overridden in atom/browser/native_window_views.h
and atom/browser/native_window_views.cc
as well, currently failing on Linux CI with:
./../atom/browser/native_window_views.cc -o obj/atom/browser/electron_lib.native_window_views.o
../../atom/browser/native_window_views.cc:1289:14: error: allocating an object of abstract class type 'atom::NativeWindowViews'
return new NativeWindowViews(inspectable_web_contents, options, parent);
^
../../atom/browser/native_window.h:165:16: note: unimplemented pure virtual method 'SetVibrancy' in 'NativeWindowViews'
virtual void SetVibrancy(const std::string& type) = 0;
^
../../atom/browser/native_window.h:166:16: note: unimplemented pure virtual method 'RemoveVibrancy' in 'NativeWindowViews'
virtual void RemoveVibrancy() = 0;
This is really cool, thanks for adding it, left a few minor comments. |
@kevinsawicki, hope I fixed everything, the only thing is that because |
Will this be merged with the main branch? |
@@ -201,6 +201,13 @@ void NativeWindow::InitFromOptions(const mate::Dictionary& options) { | |||
options.Get(options::kShow, &show); | |||
if (show) | |||
Show(); | |||
|
|||
#if defined(OS_MACOSX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor, this should be fine to call on all platforms since it is a no-op on Windows/Linux, but some of the mac-specific options are set directly in the NativeWindowMac::NativeWindowMac
constructor, do you think this should move there?
@@ -786,6 +786,18 @@ bool Window::IsVisibleOnAllWorkspaces() { | |||
return window_->IsVisibleOnAllWorkspaces(); | |||
} | |||
|
|||
void Window::SetVibrancy(v8::Local<v8::Value> value, mate::Arguments* args) { | |||
std::string type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this just took an args
param then you could support null
and undefined
with:
std::string type;
args->GetNext(&type);
window_->SetVibrancy(type);
type
will be empty if the first argument is missing or is not a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but that means that it would remove the vibrancy to any type, other than string. Is that an acceptable behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that an acceptable behavior?
Yeah, many other Electron APIs behave like this, I think this is something to improve upon and provide more consistent errors/checks but we'd do that module by module for Electron 2.0.
Can you add a small test for this that verifies this does not crash, just to guard against regressions going forward and to get some coverage on this code, something like: describe('setVibrancy', function () {
it('allows setting, changing, and removing the vibrancy', function () {
assert.doesNotThrow(function () {
window.setVibrancy('light')
window.setVibrancy('dark')
window.setVibrancy(null)
window.setVibrancy('ultra-dark')
window.setVibrancy('')
})
})
}) |
Tested this out locally, I noticed the vibrancy effect goes away when the docked dev tools are opened, is this expected? |
macOS apps only have vibrancy when they are focused, try it with any other macOS app. Does the app maintain focus when the docked devtools are focused? |
@kevinsawicki didn't noticed it, will look into it as this should not happen I think. my first guess that the With detached devTools it doesn't happen though. |
@kevinsawicki It's not related to |
I'm new to Electron and trying to implement vibrancy for my window. I create it like this: mainWindow = new BrowserWindow({
titleBarStyle: 'hidden',
transparent: true,
hasShadow: true,
vibrancy: 'dark'
}); But no matter the CSS I use, the window a) has no shadow anymore unless inactive, b) shows no translucency. Is there anything I'm missing out on? |
@Radiergummi electron version? Are you building from |
version is 1.4.6, pulled from npm. Blames on me though, I didn't notice the date on the PR... It'd be nice if features not present in the releases yet wouldn't be included in the docs, or at least received a small notice saying experimental or something... 😄 |
I was really happy when reading that this is in electron right now. After testing it today, I soon came to a point where I wanted to control the strength of the effect. |
@T-Specht this is the implementation of the |
@gerhardberger Do you know how this is done by iTerm? |
@scherii no, I haven't looked at their code yet. |
Any plans for a feature to add the vibrancy to just a section of the window? It looks like |
@elronalds interested - is there anything in particular that support for that would add over and above just blocking out areas with a solid background in css? |
@gerhardberger Hi, sorry to bother you. There's a bug with vibrancy in 1.8.0 as I describe in #10420, could you please have a look? Thank you very much |
Please Enable It For Windows 7 Please |
@omarshehab221 Windows 7 doesn't have this effect natively, unlike macOS. |
Note quite the same effect but Windows 7 does have «Aero». From @arkenthera's electron-vibrancy repository:
|
This PR adds the native macOS vibrancy effect to
BrowserWindow
. #3002