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

Blur-Behind / Glassy Windows implementation #568

Closed
wants to merge 11 commits into from

Conversation

haudan
Copy link

@haudan haudan commented Jun 15, 2018

Implements #538.

Backend progress:

  • macOS
  • Windows
  • X
  • Wayland

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality

src/os/macos.rs Outdated
@@ -16,6 +16,9 @@ pub trait WindowExt {
///
/// The pointer will become invalid when the `Window` is destroyed.
fn get_nsview(&self) -> *mut c_void;

/// Just for testing purposes.
Copy link
Author

Choose a reason for hiding this comment

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

Should probably change that comment.

@haudan
Copy link
Author

haudan commented Jun 15, 2018

Once the changes are merged, I will submit a PR to glutin for compability with these changes. For the time being, in order to follow along, clone glutin and apply these changes. Also check for newer commits.

Copy link
Member

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

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

I won't get around to actually testing this until tomorrow, but I've left you plenty of super fun nitpicks.

IdRef::new(msg_send![view, initWithWinit:state_ptr])
let view_class = if win_attribs.blur { VISUAL_EFFECT_VIEW_CLASS.0 } else { VIEW_CLASS.0 };
let view: id = msg_send![view_class, alloc];
msg_send![view, initWithWinit:state_ptr];
Copy link
Member

Choose a reason for hiding this comment

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

Proper Obj-C style dictates that we rebind view to the return of the init method. While it's typically the same pointer, it's not idiomatic to assume that.

src/os/macos.rs Outdated
#[repr(i64)]
// Applies to MacOS Mojave.
pub enum BlurMaterial {
AppearanceBased = 0, // Deperecated
Copy link
Member

Choose a reason for hiding this comment

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

Since this enum is public, the deprecation notices should be doc comments above the variant.

@@ -13,11 +13,12 @@ use cocoa::foundation::{NSPoint, NSRect, NSSize, NSString, NSUInteger};
use objc::declare::ClassDecl;
use objc::runtime::{Class, Object, Protocol, Sel, BOOL};

use {ElementState, Event, KeyboardInput, MouseButton, WindowEvent, WindowId};
use {ElementState, Event, KeyboardInput, MouseButton, WindowEvent, WindowId, WindowAttributes};
Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to keep imports sorted alphabetically, so WindowAttributes should go before WindowEvent.

use platform::platform::events_loop::{DEVICE_ID, event_mods, Shared, to_virtual_key_code};
use platform::platform::util;
use platform::platform::ffi::*;
use platform::platform::window::{get_window_id, IdRef};
use os::macos::BlurMaterial;
Copy link
Member

Choose a reason for hiding this comment

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

likewise, os imports go above platform imports.

Though, I might be more comfortable with you defining the BlurMaterial within platform, as IIRC no other types define themselves in os.

Copy link
Author

Choose a reason for hiding this comment

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

I tried defining BlurMaterial inside platform, but that creates a problem with module visibility. os needs access to this enum, but platform is inaccessible from os.

I don't know if we even should keep the enum, see my other comments.

@@ -157,3 +165,27 @@ impl MonitorIdExt for MonitorId {
self.inner.get_nsscreen().map(|s| s as *mut c_void)
}
}

#[repr(i64)]
Copy link
Member

Choose a reason for hiding this comment

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

Are these actually supposed to be i64, or are they an architecture-dependent size? I'd appreciate a comment including the NSWhatever name of the enum.

Copy link
Author

Choose a reason for hiding this comment

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

It's an i64 based on the current NSVisualEffectView.setMaterial method signature. The method takes a c-style enum, which is represented as an i64. I don't think this is set in stone but I doubt it will ever change.

I have added some valuable documentation in my new commit.

let view: id = msg_send![view_class, alloc];
msg_send![view, initWithWinit:state_ptr];
if win_attribs.blur {
msg_send![view, setMaterial: BlurMaterial::Light as i64];
Copy link
Member

Choose a reason for hiding this comment

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

You have to do let _: () = msg_send!, as otherwise segsegv/sigill will be triggered at runtime when the ! (never) type is stabilized.

@@ -3,6 +3,7 @@ use std::ops::Deref;
use std::os::raw::c_void;
use std::sync::Weak;
use std::cell::{Cell, RefCell};
use os::macos::BlurMaterial;
Copy link
Member

Choose a reason for hiding this comment

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

I've been grouping imports into blocks, ordered:

  1. std
  2. Crates
  3. Internal

With a newline between each block.

(I know this isn't consistently applied throughout the repo yet, but we're getting there.)

#[inline]
fn set_blur_material(&self, material: BlurMaterial) {
let view = self.get_nsview() as *mut objc::runtime::Object;
unsafe { msg_send![view, setMaterial: material as i64]; }
Copy link
Member

Choose a reason for hiding this comment

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

In every other usage, we call msg_send! on *self.view. Doing that is much cleaner (and has less potential overhead) than calling get_nsview like this.

Additionally, with setMaterial: material, from what I've seen Obj-C style has us writing setMaterial:material. This applies to the other occurrences as well, naturally.

src/lib.rs Outdated
@@ -482,6 +482,11 @@ pub struct WindowAttributes {
/// [iOS only] Enable multitouch,
/// see [multipleTouchEnabled](https://developer.apple.com/documentation/uikit/uiview/1622519-multipletouchenabled)
pub multitouch: bool,

/// Whether the window should be blurry. This is similar to transparency.
Copy link
Member

Choose a reason for hiding this comment

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

"blurry" has a pretty strong negative connotation. "blurred" sounds more neutral, but that has associations with input focus... "the window should have a blur effect" probably sounds the coolest, and then the second sentence should go on to explain things more.

src/window.rs Outdated
@@ -96,6 +96,15 @@ impl WindowBuilder {
#[inline]
pub fn with_transparency(mut self, transparent: bool) -> WindowBuilder {
self.window.transparent = transparent;
self.window.blur = !transparent;
Copy link
Member

Choose a reason for hiding this comment

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

What's the intention with the negation here?

Copy link
Author

Choose a reason for hiding this comment

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

Transparency and blur are mutually exclusive. The negation ensures that transprency and blur are always set to the opposite of each other, so that both can never be true at the same time. This avoids running code paths for transparency in backends when blur is enabled (and vice versa), which in turn avoids potential bugs and improves performance.

Writing this comment, I noticed that this obviously creates a bug. When for example with_blur(false) is called, transparency will be enabled, which is wrong. I'm going to fix this.

@elinorbgr
Copy link
Contributor

For the record, there is currently no way to have this implemented for Wayland, and I doubt there will be in any short-term timescale.

- Improve documentation for BlurMaterial, methods and members.
- Reorganize imports
- Annotate return type of msg_send! usages
- Rebind view to the return value of init, as is idiomatic in Objective-C
- Mark set_blur_material as unsafe
- Change some code styling for consistency
Copy link
Author

@haudan haudan left a comment

Choose a reason for hiding this comment

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

I checked the changes and implemented them accordingly. Commits are already pushed. Please review again!


#[repr(i64)]
// Applies to MacOS Mojave.
pub enum BlurMaterial {
Copy link
Author

Choose a reason for hiding this comment

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

So this enum is problematic. I created it based on the documentation Apple updated a while ago, based on the upcoming SDK version for macOS Mojave. Most of these variants are not supported on High Sierra, and will crash the application when attempted to be used. I'm unsure how to go about this. Should we remove the enum entirely and make the set_blur_material method take an i64 instead? That would more or less remove the OS version dependency from winit.

use platform::platform::events_loop::{DEVICE_ID, event_mods, Shared, to_virtual_key_code};
use platform::platform::util;
use platform::platform::ffi::*;
use platform::platform::window::{get_window_id, IdRef};
use os::macos::BlurMaterial;
Copy link
Author

Choose a reason for hiding this comment

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

I tried defining BlurMaterial inside platform, but that creates a problem with module visibility. os needs access to this enum, but platform is inaccessible from os.

I don't know if we even should keep the enum, see my other comments.

@@ -157,3 +165,27 @@ impl MonitorIdExt for MonitorId {
self.inner.get_nsscreen().map(|s| s as *mut c_void)
}
}

#[repr(i64)]
Copy link
Author

Choose a reason for hiding this comment

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

It's an i64 based on the current NSVisualEffectView.setMaterial method signature. The method takes a c-style enum, which is represented as an i64. I don't think this is set in stone but I doubt it will ever change.

I have added some valuable documentation in my new commit.

src/window.rs Outdated
@@ -96,6 +96,15 @@ impl WindowBuilder {
#[inline]
pub fn with_transparency(mut self, transparent: bool) -> WindowBuilder {
self.window.transparent = transparent;
self.window.blur = !transparent;
Copy link
Author

Choose a reason for hiding this comment

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

Transparency and blur are mutually exclusive. The negation ensures that transprency and blur are always set to the opposite of each other, so that both can never be true at the same time. This avoids running code paths for transparency in backends when blur is enabled (and vice versa), which in turn avoids potential bugs and improves performance.

Writing this comment, I noticed that this obviously creates a bug. When for example with_blur(false) is called, transparency will be enabled, which is wrong. I'm going to fix this.

/// this controls the appearance of the blur effect.
///
/// Marked as unsafe because depending on the version of macOS and the `BlurMaterial` variant passed,
/// this might cause a crash.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a lowest common denominator of supported enum variants? I don't think people will be comfortable using a method that can crash based on criteria that are only vaguely explained.

Copy link
Author

@haudan haudan Jun 17, 2018

Choose a reason for hiding this comment

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

The lowest common denominator appears to be AppearanceBased, Light, Dark, MediumLight and UltraDark (source), but those are now marked as deprecated. There are some marked as "modified", I think that means their underlying integer value has changed (validation needed), which would disqualify them. This is all quite problematic since this swift enum is our only source of truth and it appears Apple is comfortable changing the order of constants willy nilly - they only keep the swift code consistent, but apparently not the enum's binary representation.

Honestly I don't know what to do here. This is why I was originally unsure about the BlurMaterial enum in the first place.
The only alternative I see is exposing the raw i64 and removing the enum, but that won't solve the crash / macOS-version dependency problem either.

Copy link
Member

Choose a reason for hiding this comment

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

How impractical would it be for us to translate the values depending on the version of macOS?

Also, shouldn't it be isize instead of i64?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that would be too much work, definitely doable.

Yeah you could be right with isize over i64. The right size is whatever the size of the swift enum is. isize should be a good bet.

@OvermindDL1
Copy link

Just for note, KDE5's wayland client supports blue-behind/glassy windows as long as the proper transparency is set for any given pixel.

@elinorbgr
Copy link
Contributor

Does it actually blur the contents under it ? This PR is not about windows with a (partially) transparent background, but about asking the display server to blur the contents behind the transparent window.

@OvermindDL1
Copy link

It is proper blur, came out in Plasma 5.13, example video (this is apparently an early version of it, bit more refined in the release version):
https://www.youtube.com/watch?v=xroL6ZWg2Gs

The settings are global, but any properly transparent window (or parts there-of) will have the system-set blur. I'm unsure if there are per-window settings that are controllable, though knowing KDE Plasma that wouldn't surprise me if there is.

@OvermindDL1
Copy link

Ah, correction, apparently it's enabled on a window when the window has the _KDE_NET_WM_BLUR_BEHIND_REGION property enabled on it (potentially other options?).

@elinorbgr
Copy link
Contributor

Ah I see. Though, it seems that this _KDE_NET_WM_BLUR_BEHIND_REGION property is an x11 mechanism, not a wayland one. So it would not be applicable to native wayland clients.

@OvermindDL1
Copy link

OvermindDL1 commented Jun 18, 2018

Using the API itself blur is enabled by KWindowEffects::enableBlurBehind(winId, true);, and this would enable it on both X11 and Wayland.

For note, KDE is only performing bug fixes for X11 currently, new development is exclusively Wayland. The Blur uses the existing compositor functionality so it works on both.

@OvermindDL1
Copy link

It seems you can even force blur on when it is default disabled via setting the WindowForceBlurRole attribute to true via the API.

@OvermindDL1
Copy link

Documention I found this at starts with:
https://api.kde.org/4.x-api/kde-workspace-apidocs/kwin/libkwineffects/html/group__kwineffects.html
and
https://api.kde.org/4.x-api/kde-workspace-apidocs/kwin/libkwineffects/html/namespaceKWin.html

So at least KDE should be covered it seems once the calls are made. Just remember to feature detect for Plasma API 5.13 or higher to use it.

@elinorbgr
Copy link
Contributor

Ah so this would need to link winit against Qt/KDE... Not the kind of solution I was hoping for :/

@OvermindDL1
Copy link

Dynamically at most, though KDE does have a habit of exposing most things over DBus as well, so it might be available there.

What is winit linking to as it stands? Is it just direct X11/Wayland interfaces? If so then I'm unsure how you would be able to control effects such as blur as those are handled by the compositor on each, of which Plasma is KDE's compositor (I think there is another DE or two that uses plasma as well) so compositor interfaces would have to be called regardless. Plasma's Compositor is significantly the most complete on Wayland (it also has backwards compat with X11) and it even has a testing framework that does not require an active framebuffer (it has a virtual framebuffer, along the lines xvfb for X11 but one that works on Wayland as well) that is fantastic for performing automated testing (of which KDE has a substantial amount of), useful if you don't already have automated wayland testing.

@elinorbgr
Copy link
Contributor

What is winit linking to as it stands? Is it just direct X11/Wayland interfaces?

Currently yes. Ideally we would like to keep it that way as much as possible I believe.

For wayland, the ideal situation would be if Plasma exposed some KDE-specific protocol extension, I think. Though if not we'll need to work around that.

I think it may be worth opening an other issue to discuss how we should integrate WM-specific features in winit in general for X11/Wayland. This is getting out of scope for this PR, with only touches MacOS specific code.

@OvermindDL1
Copy link

For wayland, the ideal situation would be if Plasma exposed some KDE-specific protocol extension, I think. Though if not we'll need to work around that.

I'm like 90% certain it does as I'm pretty sure I ran across it a few months ago on their Wayland interfaces...

I think it may be worth opening an other issue to discuss how we should integrate WM-specific features in winit in general for X11/Wayland. This is getting out of scope for this PR, with only touches MacOS specific code.

Perhaps the title should be changed to reflect it then? The title implies nothing about Apple-specific interfaces at all? ^.^;

👍

@haudan
Copy link
Author

haudan commented Jun 18, 2018

I think it may be worth opening an other issue to discuss how we should integrate WM-specific features
in winit in general for X11/Wayland. This is getting out of scope for this PR, with only touches MacOS
specific code.

Perhaps the title should be changed to reflect it then? The title implies nothing about Apple-specific
Interfaces at all? ^.^;

This PR is for the implementation on all platforms. I'm currently working on the windows backend. The code is written, but it doesn't work... doh!

Thanks for the discussions, by the way. That will definitely help me on the Linux side.

@OvermindDL1
Copy link

OvermindDL1 commented Jun 18, 2018

Thanks for the discussions, by the way. That will definitely help me on the Linux side.

Sure. :-)

I'm mostly in the KDE world so that is what I primarily know (it's the only setup that does things like HiDPI and such 'mostly' right and so forth), but yeah it is the compositor that handles all that, and as far as I know there are 2 such compositors out, KDE's KWin is one, and Compiz is the other. I'm unsure if there are others but those are the two big ones, and only KWin has full production support for Wayland at the current time.

@haudan
Copy link
Author

haudan commented Jun 18, 2018

I just pushed a raw Windows implementation. If anyone want's to try it, just remember to resize the window. The client are needs to be redrawn for the blur effect to become visible.

By the way I accidentally rustfmted some files, which might have killed some manual formatting.

@francesca64
Copy link
Member

By the way I accidentally rustfmted some files, which might have killed some manual formatting.

I won't review this until you revert that.

@elinorbgr
Copy link
Contributor

@Lisoph

This PR is for the implementation on all platforms. [...]
Thanks for the discussions, by the way. That will definitely help me on the Linux side.

Alright, my bad.

If you plan to do the wayland implementation as well, I expect this will require some digging and possible PRs to wayland-protocols and/or sctk. I right now am quite overwhelmed but a lot of stuff and don't have the time to do all this digging around, but if someone manages to fond out what needs to be done, actually doing it should be pretty quick and straightforward.

@sp3d
Copy link

sp3d commented Jun 20, 2018

@vberger, digging suggests that https://github.com/KDE/kwayland/blob/master/src/client/protocols/blur.xml is the source of truth for the KDE blur region protocol, so it should just be a matter of binding it.

@fschutt
Copy link

fschutt commented Nov 11, 2018

Regarding KDE on X11 at least, _KDE_NET_WM_BLUR_BEHIND_REGION might be worth a look. As far as I am aware, it is a regular X11 Atom property like so many others.

Using xprop:

xprop -f _KDE_NET_WM_BLUR_BEHIND_REGION 32c -set _KDE_NET_WM_BLUR_BEHIND_REGION 0 -id $x11_window_id

@friktor
Copy link

friktor commented Jan 28, 2019

Hey. Can it be better to accept the ready changes for blur in mac os? I don`t think that in the near future anyone would want to be engaged in the implementation of blur for windows. But if you do not actualized this PR and do not accept it - over time, this work will depreciate, and someone will have to write it from scratch. @francesca64 @Lisoph

@haudan
Copy link
Author

haudan commented Jan 28, 2019

@friktor That's fine with me. Waiting for @francesca64's opinion.

@friktor
Copy link

friktor commented Jan 28, 2019

@Lisoph Good) It's just that this PR has been active for almost a year now, and would it be better to decide the fate of this feature.

@jansol
Copy link

jansol commented Mar 24, 2019

@Lisoph did you ever revert the formatting you mentioned? GitHub isn't very good at showing forced pushes sometimes...

@Osspial
Copy link
Contributor

Osspial commented Apr 24, 2019

@Lisoph just fyi - @francesca64 is no longer working on Winit, I should be able to test this out on Windows, though.

@@ -473,7 +473,8 @@ pub unsafe extern "system" fn callback(
window_id: SuperWindowId(WindowId(window)),
event: Refresh,
});
winuser::DefWindowProcW(window, msg, wparam, lparam)
//winuser::DefWindowProcW(window, msg, wparam, lparam) // TODO: Why was this here?
Copy link
Author

Choose a reason for hiding this comment

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

Like the comment says - DefWindowProc doesn't appear to make much sense for WM_PAINT. In fact it broke the redrawing code in the example. Can we remove this without breaking anything or am I missing something?

This line is also present in the most recent commit from upstream.

@haudan
Copy link
Author

haudan commented May 6, 2019

@Lisoph just fyi - @francesca64 is no longer working on Winit, I should be able to test this out on Windows, though.

@Osspial Thanks for the info! I'm still actively working on this (just fixed the modern Windows 10 implementation). Can we change to a different reviewer if @francesca64 is no longer working on winit? I'll try to merge with master and then take a look at the formatting.

@goddessfreya
Copy link
Contributor

I can test this for linux, once it's impl'd. Can this be rebased against eventloops v2?

@Osspial Osspial changed the base branch from master to winit-legacy June 13, 2019 05:36
@goddessfreya
Copy link
Contributor

Can someone rebase this PR against master and reopen it? It seams to have not aged well, so I'm closing it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

10 participants