-
Notifications
You must be signed in to change notification settings - Fork 920
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
Add support for wlr_layer_shell windows #2832
base: master
Are you sure you want to change the base?
Conversation
@@ -122,6 +175,22 @@ pub trait WindowBuilderExtWayland { | |||
/// For details about application ID conventions, see the | |||
/// [Desktop Entry Spec](https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#desktop-file-id) | |||
fn with_name(self, general: impl Into<String>, instance: impl Into<String>) -> Self; | |||
|
|||
// TODO(theonlymrcat): Users shouldn't need to pull sctk in. Reexport? or Redefine? |
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 the types from sctk are used in the API (without or without a re-export) that makes updating sctk a breaking change to winit. But that might not be a problem?
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.
That's a good point. Currently, the only public dependency in winit (except for cursor_icon, which is under the rust-windowing org) is on the android-activity
crate, and it's a simple re-export of the entire crate for the android platform.
In this case, a re-export seems to have negligible benefit, and is very easily avoidable; so I'll redefine the types.
I definitely think it's great for winit to support layer-shell in some form or another. A few things to consider here:
Would it make sense if layer shell used a different type, instead of being a It may be easier in various ways not to define a different type, but it's a bit odd to have a Though We should also consider how I also wonder about higher level abstractions to use layer-shell, or different mechanisms on X, etc. But that could be built on top of winit once this functional is here. |
&queue_handle, | ||
surface.clone(), | ||
layer, | ||
// TODO(theonlymrcat): Is this where app id should go? |
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.
Layer shell surfaces don't have app ids. They have a namespace
, which unlike app ids doesn't have any defined meaning or use.
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.
That's true, but I was wondering whether with_name
should be used to define the namespace
.
I don't think so. Yes, the API surface is very different between layer shells and xdg shells, but their purpose is mostly the same: a top-level container for whatever app is being made. As you pointed out, Also, there's a few more APIs than just (Tangent: I wonder if there is a compositor that meaningfully responds to the xdg activation protocol on a layer shell?)
I don't know much about popups, but depending on how much of their API they share with normal windows, the duplication might be something to consider in this case too. |
Thanks for your patch, but I'm not that found the way it's integrated into the current code base, given that it becomes really messy and way harder to maintain. If you want to continue on that, I could provide a possible high level design on how to integrate it in a maintainable way. The issue though, that I'm a bit busy right now, so I'm also not found of the fact that we'll have lots of requests on a window not working anymore or changing the semantics. I'm not sure I'd be able to provide a good answer for it, unless we For the future, I'd suggest to ask active maintainers before sending massive features (in general), so it could be coordinated and you don't waste your time. |
@@ -148,6 +152,8 @@ impl WinitState { | |||
xdg_shell: XdgShell::bind(globals, queue_handle)?, | |||
xdg_activation: XdgActivationState::bind(globals, queue_handle).ok(), | |||
|
|||
layer_shell: LayerShell::bind(globals, queue_handle)?, |
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.
Does this make winit fail on compositors without layer-shell support? Like Gnome.
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 way it's written -> yes.
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.
So definitely something that would need to be fixed here.
An application using this will want some way to recognize when X is being used instead of Wayland, and when Wayland is in use but the layer-shell protocol isn't available.
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.
Yeah, but I mean, the entire approach present here I don't like, because it complicates internal code without a reason for that. I'd also like to have a way to bind stuff lazily.
I am still very interested in making this feature work, even if my initial solution is less than ideal! My thoughts for a different approach would be to have another variant of |
@TheOnlyMrCat I think we should at least have separation internally, I'll be busy for a couple of weeks, but come back to it eventually. I also want to try simplify some internals in wayland backend, so adding layer shell could be simple afterwards. |
Hello! I'm quite interested in this feature, and I would like to kindly ask what is gonna happen to it :) |
Ping 👐 |
Maybe one day. Don't get me wrong, the layer-shell stuff is just very different to normal desktop use. And you can use |
It would be great to have a helper library that adds some groundwork like raw-window-handle (and maybe an event loop?) Having to do that is is a bit tedious and a bit hard for some so a standard-ish way for layer-shell apps is the way to go I think. I tried that once but came to a halt because sctk was before a rewrite or something |
@RegenJacob have you seen SCTK's example to make a layer-shell window? It has all of that, and that's what winit is using. Winit just builds cross platform api on top of that. |
Yes I edited my response didn't reload the page so I'm just now seeing this |
Or if a Rust toolkit wants to support normal windows (across platforms, using winit), and layer shell surfaces. In theory wlr-layer-shell is also going to be replaced by ext-layer-shell at some point, though not necessarily any time soon: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/28 |
That would be my use case, I'd like to use a widget library, that has support for winit. As it uses winit types for everything, also input etc. I'd need to manually map sctk to those. |
CHANGELOG.md
if knowledge of this change could be valuable to userssoftbuffer
from it.Layer shell windows are very different to normal windows, so there is quite a bit of weirdness that results from handling both XDG and Layer shells with the same system. (
let...else
from Rust 1.65 would make the code a lot nicer).On the other hand, it would be nice to have winit's input handling for layer_shell apps.
Fixes #2582
Unresolved design questions:
Anchor
,KeyboardInteractivity
, andLayer
should be reexported inwinit::platform::wayland
.closed
event sending aCloseRequested
event to the event loop. A panic will likely result if the window is attempted to be used after this is received. I am not sure when the compositor would send this event, nor what the library should do in response to it.